Bug 181840 – Release ASSERT when reloading Vimeo page @ WebCore: WebCore::Document::updateLayout

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Release ASSERT when reloading Vimeo page @ WebCore: WebCore::Document::updateLayout

[Jer Noble]

<rdar://problem/36186214>

[Jer Noble]

Created attachment 331712 [details]
Patch

[EWS Watchlist]

Comment on attachment 331712 [details]
Patch

Attachment 331712 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6131807

New failing tests:
media/video-fullscreen-reload-crash.html

[EWS Watchlist]

Created attachment 331714 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Simon Fraser (smfr)]

Comment on attachment 331712 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:62
> +    if (document.activeDOMObjectsAreSuspended() || document.activeDOMObjectsAreStopped())

It's not obvious why active DOM objects being suspended means that you can't do layout. What does it mean for an "active" DOM object to be suspended? Is that reflecting some higher-level state (being in the page cache?) that we should be checking for?

[Jer Noble]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 331712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331712&action=review
> 
> > Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:62
> > +    if (document.activeDOMObjectsAreSuspended() || document.activeDOMObjectsAreStopped())
> 
> It's not obvious why active DOM objects being suspended means that you can't
> do layout. What does it mean for an "active" DOM object to be suspended? Is
> that reflecting some higher-level state (being in the page cache?) that we
> should be checking for?

It hits "   RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(isSafeToUpdateStyleOrLayout())" in Document::updateLayout().  isSafeToUpdateStyleOrLayout() itself calls ScriptDisallowedScope::InMainThread::isScriptAllowed(), which is set by ScriptExecutionContext() while stopping ActiveDOMObjects.

So I suppose we could check Document:: isSafeToUpdateStyleOrLayout() instead.

[Simon Fraser (smfr)]

That's what (Z)Alan suggests.

[Jer Noble]

Created attachment 331792 [details]
Patch for landing

[EWS Watchlist]

Comment on attachment 331792 [details]
Patch for landing

Attachment 331792 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6142286

New failing tests:
media/video-fullscreen-reload-crash.html

[EWS Watchlist]

Created attachment 331818 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Jer Noble]

Created attachment 331834 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331834 [details]
Patch for landing

Clearing flags on attachment: 331834

Committed r227272: <https://trac.webkit.org/changeset/227272>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 331712 [details]
Patch

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

>>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:62
>>> +    if (document.activeDOMObjectsAreSuspended() || document.activeDOMObjectsAreStopped())
>> 
>> It's not obvious why active DOM objects being suspended means that you can't do layout. What does it mean for an "active" DOM object to be suspended? Is that reflecting some higher-level state (being in the page cache?) that we should be checking for?
> 
> It hits "   RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(isSafeToUpdateStyleOrLayout())" in Document::updateLayout().  isSafeToUpdateStyleOrLayout() itself calls ScriptDisallowedScope::InMainThread::isScriptAllowed(), which is set by ScriptExecutionContext() while stopping ActiveDOMObjects.
> 
> So I suppose we could check Document:: isSafeToUpdateStyleOrLayout() instead.

I think this check would have been better.

[Ryosuke Niwa]

Comment on attachment 331712 [details]
Patch

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

>>>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:62
>>>> +    if (document.activeDOMObjectsAreSuspended() || document.activeDOMObjectsAreStopped())
>>> 
>>> It's not obvious why active DOM objects being suspended means that you can't do layout. What does it mean for an "active" DOM object to be suspended? Is that reflecting some higher-level state (being in the page cache?) that we should be checking for?
>> 
>> It hits "   RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(isSafeToUpdateStyleOrLayout())" in Document::updateLayout().  isSafeToUpdateStyleOrLayout() itself calls ScriptDisallowedScope::InMainThread::isScriptAllowed(), which is set by ScriptExecutionContext() while stopping ActiveDOMObjects.
>> 
>> So I suppose we could check Document:: isSafeToUpdateStyleOrLayout() instead.
> 
> I think this check would have been better.

Another check would have been to check the presence of RenderView. At this point, the render view had been destroyed.