Null-dereference READ in blink::LocalFrameView::UpdateGeometry |
||||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5987490635972608 Fuzzer: ochang_domfuzzer Job Type: windows_asan_content_shell Platform Id: windows Crash Type: Null-dereference READ Crash Address: 0x00000074 Crash State: blink::LocalFrameView::UpdateGeometry blink::RootScrollerController::UpdateIFrameGeometryAndLayoutSize blink::RootScrollerController::ApplyRootScrollerProperties Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=windows_asan_content_shell&range=485825:485889 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5987490635972608 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Sep 18 2017
,
Sep 28 2017
,
Oct 1 2017
Automatically applying components based on information from OWNERS files. If this seems incorrect, please apply the Test-Predator-Wrong-Components label.
,
Oct 3 2017
Reassigning to bokan who knows about this code.
,
Oct 3 2017
,
Oct 3 2017
Yeah, this belongs with me. Stefan/Chris, eae@ mentioned a few days back you might have some insight here: this is occuring because the rootScroller should determine the Document/Canvas background. But we don't know what the rootScroller is until we do a layout. Currently, after a layout, if the rootScroller changes, I'll go back to StyleRecalc and -- because we're at the end of layout -- I synchronously UpdateStyleAndLayout. This "works" but I've been playing whack-a-mole with clusterfuzz since so I think this isn't a great approach. The alternative is to schedule another lifecycle pass at the end of this one, that seems cleaner to me and Emil mentioned IntersectionObserver had to do something similar. Any thoughts? Could you point me to the IntersectionObserver code where this happens?
,
Oct 3 2017
I agree that scheduling a lifecycle update is almost always the best way to do things, because otherwise you have to think really carefully about state management, and the system ends up more complex. Regarding IntersectionObserver, I had to add the ability to force an update even within a throttled frame, but that isn't the case you are hitting. Instead you should be able to just set dirty bits by calling SetNeedsLayout etc (or whichever dirty bits are appropriate). The lifecycle will automatically run in the next frame.
,
Oct 3 2017
Ok, thanks, that does sound easy enough. I'll give that a try...
,
Oct 5 2017
So I'm not sure it's that simple to do from inside the lifecycle. I assign the effective rootScroller when we're in kLayoutClean. Setting SetNeedsStyleRecalc causes the lifecycle to go back to kVisualUpdatePending. Further along in the LocalFrameView updateLifecycle steps it tries to advance the lifecycle to LayoutClean again (which DCHECKs). But even if I ignore that one, I get lifecycle violations later on because other components are now assuming we're past LayoutClean. Should SetNeedsStyleRecalc be changing the lifecycle state?
,
Oct 5 2017
Indeed, you can't call SetNeedsStyleRecalc from within a lifecycle update. Sorry, forgot about that wrinkle. Why does layout determine the rootScroller exactly? Trying to understand better so I can help.
,
Oct 5 2017
We restrict the rootScroller to elements that exactly fill the viewport so we need to know the geometry of it. At that point we promote it to the "effective" rootScroller. This happens inside the lifecycle because when we get to compositing we make some changes based on which Element is the "effective" rootScroller (i.e. disable clipping on iframe rootScroller to make URL bar resizes work correctly).
,
Oct 13 2017
chrishtr@: ping, any suggestions on how this should work? I could hook in to the end of the lifecycle and try to setNeedsStyleRecalc from there, does that sound like it would be ok?
,
Oct 13 2017
Is it just the document background? If so it's just a style recalc, not a layout. However, are you sure this is really a required feature? Why is the document background needed in the presence of a root scroller? The document background rules relating to the HTML/body element are weird and special. Can we avoid propagating that complexity to root scrollers?
,
Oct 16 2017
Yeah, it's just style. It's not critically required but if the rootScroller Element doesn't have scrolling content and the URL bar is hidden, the background of the <html> element will show through at the bottom so if the colors mismatch it'll look weird. (see example screenshot) So authors can manually set the background on <html> whenever they change the rootScroller to fix this but this seems like something they'd always want to do so I think it makes sense to do it with the rootScroller. Another idea I thought about was setting the background style when document.rootScroller is set - regardless of whether it matches viewport size and gets "promoted" to being active. That would avoid these lifecycle issues but complicates the feature since some properties are set while others aren't if the scroller is invalid, it would be confusing to understand and explain to developers.
,
Oct 16 2017
> It's not critically required but if the rootScroller Element doesn't have scrolling content and the URL bar is hidden, the background of the <html> element will show through at the bottom so if the colors mismatch it'll look weird. (see example screenshot) The rootScroller is required to cover the entire screen. Shouldn't that be true even if the URL bar is hidden?
,
Oct 16 2017
It's required to fill the ICB (i.e. height: 100%) but we don't resize the ICB when the URL bar hides. So the rootScroller's LayoutBox is smaller than the viewport but we expand its clip so extra scrolling content can be seen. This is also how regular <html> scrolling works today but in that case, the conceptual "canvas" gets the same background as the <html> Element so you don't notice. (you do notice if the background is a gradient, there's a discontinuity at the screen bottom as we paint both the canvas and the <html>'s background with the gradient but they're different heights.
,
Oct 20 2017
ClusterFuzz has detected this issue as fixed in range 510178:510268. Detailed report: https://clusterfuzz.com/testcase?key=5987490635972608 Fuzzer: ochang_domfuzzer Job Type: windows_asan_content_shell Platform Id: windows Crash Type: Null-dereference READ Crash Address: 0x00000074 Crash State: blink::LocalFrameView::UpdateGeometry blink::RootScrollerController::UpdateIFrameGeometryAndLayoutSize blink::RootScrollerController::ApplyRootScrollerProperties Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=windows_asan_content_shell&range=485825:485889 Fixed: https://clusterfuzz.com/revisions?job=windows_asan_content_shell&range=510178:510268 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5987490635972608 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Oct 20 2017
ClusterFuzz testcase 5987490635972608 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Nov 7 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by kkaluri@chromium.org
, Sep 18 2017Components: Blink
Labels: M-62 Test-Predator-Wrong
Owner: joelhockey@chromium.org
Status: Assigned (was: Untriaged)