Bug 186383 – Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn()

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

#0	0x00000002cf969cd0 in ::WTFCrash() at /Users/antoine/Code/safari/OpenSource/Source/WTF/wtf/Assertions.cpp:267
#1	0x00000002cf969ce9 in ::WTFCrashWithSecurityImplication() at /Users/antoine/Code/safari/OpenSource/Source/WTF/wtf/Assertions.cpp:288
#2	0x00000002c1dc6926 in WebCore::Document::updateLayout() at /Users/antoine/Code/safari/OpenSource/Source/WebCore/dom/Document.cpp:1981
#3	0x00000002c30afa16 in WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestLocation const&, WebCore::HitTestResult&) at /Users/antoine/Code/safari/OpenSource/Source/WebCore/rendering/RenderView.cpp:147
#4	0x00000002c30af9d4 in WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestResult&) at /Users/antoine/Code/safari/OpenSource/Source/WebCore/rendering/RenderView.cpp:142
#5	0x0000000101e03ec1 in WebKit::WebPage::determinePrimarySnapshottedPlugIn() at /Users/antoine/Code/safari/OpenSource/Source/WebKit/WebProcess/WebPage/WebPage.cpp:5407
#6	0x0000000101de27fe in WebKit::WebPage::determinePrimarySnapshottedPlugInTimerFired() at /Users/antoine/Code/safari/OpenSource/Source/WebKit/WebProcess/WebPage/WebPage.cpp:5342
#7	0x0000000101e16656 in WTF::RunLoop::Timer<WebKit::WebPage>::fired() at /Users/antoine/Builds/Debug/usr/local/include/wtf/RunLoop.h:148
#8	0x00000002cf9e2861 in WTF::RunLoop::TimerBase::timerFired(__CFRunLoopTimer*, void*) at /Users/antoine/Code/safari/OpenSource/Source/WTF/wtf/cf/RunLoopCF.cpp:84

[Ryosuke Niwa]

<rdar://problem/40849498>

[Ryosuke Niwa]

Created attachment 342123 [details]
Fixes the bug

[Jon Lee]

Comment on attachment 342123 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=342123&action=review

> LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html:22
> +    }, 500);

Does this make flakiness a risk?

[alan]

Comment on attachment 342123 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=342123&action=review

> Source/WebKit/ChangeLog:11
> +        The release assert was hit because the descendent elemenet iterator, which instantiates ScriptDisallowedScope,
> +        was alive as determinePrimarySnapshottedPlugIn updated the layout via RenderView::hitTest. Avoid this by copying
> +        the list of plugin image elements into a vector first.

I would rephrase this to "potentially updated" or something along those lines.
determinePrimarySnapshottedPlugIn carefully issues a layout starting from the top level document and it expects the hittest's updateLayout() to be a no-op. I understand that with the current script-callbacks-after-layout model, this is not guaranteed at all. We need to invest some time in this area and ensure immutability for cases like this. What determinePrimarySnapshottedPlugIn() has today should be the preferred way of coding against mutation as opposed to nullchecks and refing all the things.

[Ryosuke Niwa]

(In reply to Jon Lee from comment #3)
> Comment on attachment 342123 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342123&action=review
> 
> > LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html:22
> > +    }, 500);
> 
> Does this make flakiness a risk?

All other snapshot tests are doing this, and there doesn't seem to be a way to do this deterministically due to the way snapshot algorithm waits for the movie to get to a snapshottable state.

[Ryosuke Niwa]

(In reply to zalan from comment #4)
> Comment on attachment 342123 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342123&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        The release assert was hit because the descendent elemenet iterator, which instantiates ScriptDisallowedScope,
> > +        was alive as determinePrimarySnapshottedPlugIn updated the layout via RenderView::hitTest. Avoid this by copying
> > +        the list of plugin image elements into a vector first.
> 
> I would rephrase this to "potentially updated" or something along those
> lines.
> determinePrimarySnapshottedPlugIn carefully issues a layout starting from
> the top level document and it expects the hittest's updateLayout() to be a
> no-op. I understand that with the current script-callbacks-after-layout
> model, this is not guaranteed at all. We need to invest some time in this
> area and ensure immutability for cases like this. What
> determinePrimarySnapshottedPlugIn() has today should be the preferred way of
> coding against mutation as opposed to nullchecks and refing all the things.

That's a big misleading since it's true today that invoking updateLayout in this function isn't safe (or at least would hit the release assertion). I'd rephrase as "invoked Document::updateLayout" instead for clarity.

[Ryosuke Niwa]

Committed r232591: <https://trac.webkit.org/changeset/232591>