Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2229753002:
Test that FrameView::incrementVisuallyNonEmptyPixelCount() doesn't overflow (Closed)
DescriptionTest that FrameView::incrementVisuallyNonEmptyPixelCount() doesn't overflow
This CL adds an unit test for https://codereview.chromium.org/2218003003/ that
would have failed before that CL.
BUG=634819
Committed: https://crrev.com/924bbbbfed37fa4c08187bdc137aeed76e8b96ec
Cr-Commit-Position: refs/heads/master@{#422040}
Patch Set 1 #Patch Set 2 : Use max value of 32-bit signed integer #
Total comments: 2
Patch Set 3 : Add SimTest #
Total comments: 4
Patch Set 4 : Reflect comments #
Messages
Total messages: 22 (7 generated)
Description was changed from ========== Add an unit test for FrameView::inIncrementVisuallyNonEmptyPixelCount BUG=634819 ========== to ========== Test that FrameView::inIncrementVisuallyNonEmptyPixelCount doesn't overflow This CL adds an unit test for https://codereview.chromium.org/2218003003/ that would have failed before that CL. BUG=634819 ==========
hiroshige@chromium.org changed reviewers: + bokan@chromium.org, esprehn@chromium.org
PTAL.
How does html content manage to hit this btw?
How does html content manage to hit this btw?
On 2016/08/09 10:16:36, esprehn wrote: > How does html content manage to hit this btw? incrementVisuallyNonEmptyPixelCount() is called from LayoutImage::imageChanged() https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... so showing an quite large image (e.g. a SVG file with large width/height values) is sufficient to hit this code.
Description was changed from ========== Test that FrameView::inIncrementVisuallyNonEmptyPixelCount doesn't overflow This CL adds an unit test for https://codereview.chromium.org/2218003003/ that would have failed before that CL. BUG=634819 ========== to ========== Test that FrameView::incrementVisuallyNonEmptyPixelCount() doesn't overflow This CL adds an unit test for https://codereview.chromium.org/2218003003/ that would have failed before that CL. BUG=634819 ==========
https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:173: document().view()->incrementVisuallyNonEmptyPixelCount(IntSize(2147483647, 2147483647)); This won't actually fail unless run on the sanitizer though, right? Perhaps come up with numbers that, when overflowing, end up less than the visualPixelThreshold in incrementVisuallyNonEmptyPixelCount?
On 2016/08/09 at 14:10:32, bokan wrote: > https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): > > https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameViewTest.cpp:173: document().view()->incrementVisuallyNonEmptyPixelCount(IntSize(2147483647, 2147483647)); > This won't actually fail unless run on the sanitizer though, right? Perhaps come up with numbers that, when overflowing, end up less than the visualPixelThreshold in incrementVisuallyNonEmptyPixelCount? Yeah, I'd much rather a DummyPageHolder or SimTest or something that actually loaded some massive SVG's too (in addition to this test). This test is making sure the code works, but it doesn't explain how this happens
Added a SimTest. https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:173: document().view()->incrementVisuallyNonEmptyPixelCount(IntSize(2147483647, 2147483647)); On 2016/08/09 14:10:32, bokan wrote: > This won't actually fail unless run on the sanitizer though, right? Perhaps come > up with numbers that, when overflowing, end up less than the > visualPixelThreshold in incrementVisuallyNonEmptyPixelCount? 2147483647 * 2147483647 would become 4 if calculated in 32-bit, so this test would fail. However I changed the size to 65536 * 65536 (which clearly would become 0 in 32-bit), partially because there is a limit on sizes of SVGs and we cannot test a SVG file with 2147483647 * 2147483647 pixels.
On 2016/08/12 06:30:16, hiroshige wrote: > Added a SimTest. > > https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): > > https://codereview.chromium.org/2229753002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameViewTest.cpp:173: > document().view()->incrementVisuallyNonEmptyPixelCount(IntSize(2147483647, > 2147483647)); > On 2016/08/09 14:10:32, bokan wrote: > > This won't actually fail unless run on the sanitizer though, right? Perhaps > come > > up with numbers that, when overflowing, end up less than the > > visualPixelThreshold in incrementVisuallyNonEmptyPixelCount? > > 2147483647 * 2147483647 would become 4 if calculated in 32-bit, so this test > would fail. Ah, sorry, didn't realize that. > > However I changed the size to 65536 * 65536 (which clearly would become 0 in > 32-bit), partially because there is a limit on sizes of SVGs and we cannot test > a SVG file with 2147483647 * 2147483647 pixels. Thanks, this looks like a better test. IMO, the SimTest alone should be enough and it looks fine to me but wait for esprehn@ to stamp since I'm not really familiar with SimTests.
Much better! https://codereview.chromium.org/2229753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebMeaningfulLayoutsTest.cpp (right): https://codereview.chromium.org/2229753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebMeaningfulLayoutsTest.cpp:189: testing::runPendingTasks(); What task are you trying to run here? https://codereview.chromium.org/2229753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebMeaningfulLayoutsTest.cpp:199: webView().updateAllLifecyclePhases(); compositor().beginFrame() ?
lgtm w/ the nits fixed :)
Should we land this? :)
https://codereview.chromium.org/2229753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebMeaningfulLayoutsTest.cpp (right): https://codereview.chromium.org/2229753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebMeaningfulLayoutsTest.cpp:189: testing::runPendingTasks(); On 2016/08/15 15:59:39, esprehn wrote: > What task are you trying to run here? Added a comment. https://codereview.chromium.org/2229753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebMeaningfulLayoutsTest.cpp:199: webView().updateAllLifecyclePhases(); On 2016/08/15 15:59:39, esprehn wrote: > compositor().beginFrame() ? Done.
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2229753002/#ps60001 (title: "Reflect comments")
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 ========== Test that FrameView::incrementVisuallyNonEmptyPixelCount() doesn't overflow This CL adds an unit test for https://codereview.chromium.org/2218003003/ that would have failed before that CL. BUG=634819 ========== to ========== Test that FrameView::incrementVisuallyNonEmptyPixelCount() doesn't overflow This CL adds an unit test for https://codereview.chromium.org/2218003003/ that would have failed before that CL. BUG=634819 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Test that FrameView::incrementVisuallyNonEmptyPixelCount() doesn't overflow This CL adds an unit test for https://codereview.chromium.org/2218003003/ that would have failed before that CL. BUG=634819 ========== to ========== Test that FrameView::incrementVisuallyNonEmptyPixelCount() doesn't overflow This CL adds an unit test for https://codereview.chromium.org/2218003003/ that would have failed before that CL. BUG=634819 Committed: https://crrev.com/924bbbbfed37fa4c08187bdc137aeed76e8b96ec Cr-Commit-Position: refs/heads/master@{#422040} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/924bbbbfed37fa4c08187bdc137aeed76e8b96ec Cr-Commit-Position: refs/heads/master@{#422040} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
