Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2454293004:
Printing: Fix undefined behavior for near 0 scaling (Closed)
DescriptionPrinting: Fix undefined behavior for near 0 scaling.
Sometimes casting 0 integer value to a float results in a very
small number (on the order of 1e-312). This results in integer
overflows when this scaling is used. Check scaling against
0.01f instead of 0 since we know it has to be an integer/100.
If it is less than .01 the value must have been 0 (not set),
so set it to the default value (1.0).
Undefined behavior was introduced in:
https://crrev.com/427556
BUG=660135
Committed: https://crrev.com/2f5080ac8257bc1dacb6045bdf2d2a3fedb95531
Cr-Commit-Position: refs/heads/master@{#428597}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add constant #
Total comments: 2
Patch Set 3 : Move declaration #
Total comments: 2
Patch Set 4 : Fix declaration #
Messages
Total messages: 29 (19 generated)
Description was changed from ========== Change to gte Revert line deletes Fix undefined behavior for near 0 scaling BUG= ========== to ========== Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). BUG=660135 ==========
rbpotter@chromium.org changed reviewers: + thestig@chromium.org
Description was changed from ========== Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). BUG=660135 ========== to ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). BUG=660135 ==========
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). BUG=660135 ========== to ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). Undefined behavior was introduced in: https://chromium.googlesource.com/chromium/src/+/769ffdf779d83399a6f219e2637c... BUG=660135 ==========
BTW, in the commit message, https://crrev.com/427556 is equivalent and more human readable. https://codereview.chromium.org/2454293004/diff/1/components/printing/rendere... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2454293004/diff/1/components/printing/rendere... components/printing/renderer/print_web_view_helper.cc:237: if (scale_factor >= 0.01f && (fit_to_page || params.print_to_pdf)) { Add a constexpr double kEpsilon constant and use that everywhere.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). Undefined behavior was introduced in: https://chromium.googlesource.com/chromium/src/+/769ffdf779d83399a6f219e2637c... BUG=660135 ========== to ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). Undefined behavior was introduced in: https://crrev.com/427556 BUG=660135 ==========
https://codereview.chromium.org/2454293004/diff/1/components/printing/rendere... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2454293004/diff/1/components/printing/rendere... components/printing/renderer/print_web_view_helper.cc:237: if (scale_factor >= 0.01f && (fit_to_page || params.print_to_pdf)) { On 2016/10/29 00:26:53, Lei Zhang wrote: > Add a constexpr double kEpsilon constant and use that everywhere. Done.
https://codereview.chromium.org/2454293004/diff/20001/components/printing/ren... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2454293004/diff/20001/components/printing/ren... components/printing/renderer/print_web_view_helper.h:54: constexpr double kEpsilon = .01f; Mmm, this adds printing::kEpsilon. Can we declare it within PrintWebViewHelper? i.e. static constexpr double kEpsilon = 0.01f; around line 115.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2454293004/diff/20001/components/printing/ren... File components/printing/renderer/print_web_view_helper.h (right): https://codereview.chromium.org/2454293004/diff/20001/components/printing/ren... components/printing/renderer/print_web_view_helper.h:54: constexpr double kEpsilon = .01f; On 2016/10/29 00:59:27, Lei Zhang wrote: > Mmm, this adds printing::kEpsilon. Can we declare it within PrintWebViewHelper? > i.e. static constexpr double kEpsilon = 0.01f; around line 115. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with one more round of shuffling. https://codereview.chromium.org/2454293004/diff/40001/components/printing/ren... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2454293004/diff/40001/components/printing/ren... components/printing/renderer/print_web_view_helper.cc:117: constexpr double kEpsilon = 0.01f; Sorry, I meant line 115 in the header file, that way you can still use the constant in print_web_view_helper_mac.mm. CalculatePageLayoutFromPrintParams() will have to change to refer to it as PrintWebViewHelper::kEpsilon.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2454293004/#ps60001 (title: "Fix declaration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). Undefined behavior was introduced in: https://crrev.com/427556 BUG=660135 ========== to ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). Undefined behavior was introduced in: https://crrev.com/427556 BUG=660135 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). Undefined behavior was introduced in: https://crrev.com/427556 BUG=660135 ========== to ========== Printing: Fix undefined behavior for near 0 scaling. Sometimes casting 0 integer value to a float results in a very small number (on the order of 1e-312). This results in integer overflows when this scaling is used. Check scaling against 0.01f instead of 0 since we know it has to be an integer/100. If it is less than .01 the value must have been 0 (not set), so set it to the default value (1.0). Undefined behavior was introduced in: https://crrev.com/427556 BUG=660135 Committed: https://crrev.com/2f5080ac8257bc1dacb6045bdf2d2a3fedb95531 Cr-Commit-Position: refs/heads/master@{#428597} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2f5080ac8257bc1dacb6045bdf2d2a3fedb95531 Cr-Commit-Position: refs/heads/master@{#428597}
Message was sent while issue was closed.
https://codereview.chromium.org/2454293004/diff/40001/components/printing/ren... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2454293004/diff/40001/components/printing/ren... components/printing/renderer/print_web_view_helper.cc:117: constexpr double kEpsilon = 0.01f; On 2016/10/29 01:19:24, Lei Zhang wrote: > Sorry, I meant line 115 in the header file, that way you can still use the > constant in print_web_view_helper_mac.mm. > > CalculatePageLayoutFromPrintParams() will have to change to refer to it as > PrintWebViewHelper::kEpsilon. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
