Bug 181840 - Release ASSERT when reloading Vimeo page @ WebCore: WebCore::Document::updateLayout
Summary: Release ASSERT when reloading Vimeo page @ WebCore: WebCore::Document::update...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-18 23:15 PST by Jer Noble
Modified: 2018-02-05 13:36 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.36 KB, patch)
2018-01-18 23:19 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (8.36 MB, application/zip)
2018-01-19 00:55 PST, EWS Watchlist
no flags Details
Patch for landing (7.03 KB, patch)
2018-01-19 14:53 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (8.40 MB, application/zip)
2018-01-19 16:51 PST, EWS Watchlist
no flags Details
Patch for landing (8.09 KB, patch)
2018-01-19 22:58 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2018-01-18 23:15:00 PST
Release ASSERT when reloading Vimeo page @ WebCore: WebCore::Document::updateLayout
Comment 1 Jer Noble 2018-01-18 23:15:29 PST
<rdar://problem/36186214>
Comment 2 Jer Noble 2018-01-18 23:19:17 PST
Created attachment 331712 [details]
Patch
Comment 3 EWS Watchlist 2018-01-19 00:55:40 PST
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
Comment 4 EWS Watchlist 2018-01-19 00:55:42 PST
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
Comment 5 Simon Fraser (smfr) 2018-01-19 11:01:51 PST
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?
Comment 6 Jer Noble 2018-01-19 11:41:54 PST
(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.
Comment 7 Simon Fraser (smfr) 2018-01-19 11:45:11 PST
That's what (Z)Alan suggests.
Comment 8 Jer Noble 2018-01-19 14:53:04 PST
Created attachment 331792 [details]
Patch for landing
Comment 9 EWS Watchlist 2018-01-19 16:51:22 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-01-19 16:51:23 PST Comment hidden (obsolete)
Comment 11 Jer Noble 2018-01-19 22:58:43 PST
Created attachment 331834 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-01-20 08:55:49 PST
Comment on attachment 331834 [details]
Patch for landing

Clearing flags on attachment: 331834

Committed r227272: <https://trac.webkit.org/changeset/227272>
Comment 13 WebKit Commit Bot 2018-01-20 08:55:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Ryosuke Niwa 2018-02-05 13:34:48 PST
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.
Comment 15 Ryosuke Niwa 2018-02-05 13:36:01 PST
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.