Bug 186383

Summary: Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn()
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: WebKit2Assignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, graouts, jonlee, koivisto, simon.fraser, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixes the bug jonlee: review+

Description Ryosuke Niwa 2018-06-06 22:56:11 PDT 
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
Comment 1 Ryosuke Niwa 2018-06-06 22:56:23 PDT 
<rdar://problem/40849498>
Comment 2 Ryosuke Niwa 2018-06-06 23:08:27 PDT 
Created attachment 342123 [details]
Fixes the bug
Comment 3 Jon Lee 2018-06-07 09:00:14 PDT 
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?
Comment 4 zalan 2018-06-07 09:39:41 PDT 
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.
Comment 5 Ryosuke Niwa 2018-06-07 11:27:28 PDT 
(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.
Comment 6 Ryosuke Niwa 2018-06-07 11:36:42 PDT 
(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.
Comment 7 Ryosuke Niwa 2018-06-07 11:38:42 PDT 
Committed r232591: <https://trac.webkit.org/changeset/232591>