Bug 188307 – Regression(r233865): Causes synchronous IPC in the middle of layout

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

r233865 is causing synchronous IPC in the middle of layout:
Exception Type:  EXC_BREAKPOINT (SIGTRAP)
Exception Codes: 0x0000000000000001, 0x00000001983c5c20
Termination Signal: Trace/BPT trap: 5
Termination Reason: Namespace SIGNAL, Code 0x5
Terminating Process: exc handler [721]
Triggered by Thread:  0

Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   WebKit                        	0x00000001983c5c20 bool IPC::Connection::sendSync<Messages::WebPageProxy::GetIsViewVisible>(Messages::WebPageProxy::GetIsViewVisible&&, Messages::WebPageProxy::GetIsViewVisible::Reply&&, unsigned long long, WTF::Seconds, WTF::OptionSet<IPC::SendSyncOption>) + 296 (Connection.h:430)
1   WebKit                        	0x00000001983c5b2c bool IPC::Connection::sendSync<Messages::WebPageProxy::GetIsViewVisible>(Messages::WebPageProxy::GetIsViewVisible&&, Messages::WebPageProxy::GetIsViewVisible::Reply&&, unsigned long long, WTF::Seconds, WTF::OptionSet<IPC::SendSyncOption>) + 52 (ScriptDisallowedScope.h:56)
2   WebKit                        	0x00000001983c5ae4 WebKit::WebChromeClient::isViewVisible() + 76 (WebChromeClient.cpp:1317)
3   WebCore                       	0x0000000192132ec4 WebCore::HTMLMediaElement::enterFullscreen(unsigned int) + 176 (HTMLMediaElement.cpp:5901)
4   WebCore                       	0x0000000192120a5c WebCore::HTMLMediaElement::updatePlayState(WebCore::HTMLMediaElement::UpdateState) + 264 (HTMLMediaElement.cpp:5306)
5   WebCore                       	0x000000019212b618 WebCore::HTMLMediaElement::playInternal() + 972 (HTMLMediaElement.cpp:3551)
6   WebCore                       	0x000000019212b6cc WebCore::HTMLMediaElement::play() + 176 (HTMLMediaElement.cpp:3479)
7   WebCore                       	0x0000000192136a04 non-virtual thunk to WebCore::HTMLMediaElement::resumeAutoplaying() + 204 (HTMLMediaElement.cpp:7472)
8   WebCore                       	0x00000001924880f8 WebCore::PlatformMediaSession::endInterruption(WebCore::PlatformMediaSession::EndInterruptionFlags) + 512 (PlatformMediaSession.cpp:166)
9   WebCore                       	0x000000019280c320 WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType, WebCore::RenderTreeBuilder&)::$_5::operator()(unsigned int) const + 48 (RenderTreeUpdater.cpp:583)
10  WebCore                       	0x000000019280b46c WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType, WebCore::RenderTreeBuilder&) + 572 (RenderTreeUpdater.cpp:591)
11  WebCore                       	0x000000019280aca4 WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&) + 408 (RenderTreeUpdater.cpp:321)
12  WebCore                       	0x000000019280a290 WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 1188 (RenderTreeUpdater.cpp:200)
13  WebCore                       	0x0000000192809d58 WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 592 (RenderTreeUpdater.cpp:132)
14  WebCore                       	0x0000000191f353c0 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 1036 (Document.cpp:1864)
15  WebCore                       	0x0000000191f35d7c WebCore::Document::updateStyleIfNeeded() + 344 (Document.cpp:1968)
16  WebCore                       	0x0000000191de910c WebCore::updateStyleIfNeededForProperty(WebCore::Element&, WebCore::CSSPropertyID) + 108 (CSSComputedStyleDeclaration.cpp:2470)
17  WebCore                       	0x0000000191de8598 WebCore::ComputedStyleExtractor::propertyValue(WebCore::CSSPropertyID, WebCore::EUpdateLayout) + 144 (CSSComputedStyleDeclaration.cpp:2681)
18  WebCore                       	0x0000000191de7f7c WebCore::CSSComputedStyleDeclaration::getPropertyValue(WebCore::CSSPropertyID) const + 76 (CSSComputedStyleDeclaration.cpp:2419)
19  WebCore                       	0x0000000191494144 WebCore::jsCSSStyleDeclarationPrototypeFunctionGetPropertyValue(JSC::ExecState*) + 316 (JSCSSStyleDeclaration.cpp:431)

[Chris Dumez]

<rdar://problem/42807306>

[Chris Dumez]

Created attachment 346489 [details]
Patch

[Chris Dumez]

@Jer / Jeremy: Note that there is likely another bug here. Seems very odd that destroying a Media element's renderer would cause it to start autoplaying in the first place.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> @Jer / Jeremy: Note that there is likely another bug here. Seems very odd
> that destroying a Media element's renderer would cause it to start
> autoplaying in the first place.

Filed https://bugs.webkit.org/show_bug.cgi?id=188310 for this.

[Geoffrey Garen]

Comment on attachment 346489 [details]
Patch

iOS simulator failure looks like a real failure?

  media/no-fullscreen-when-hidden.html [ Failure ]

[Chris Dumez]

(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 346489 [details]
> Patch
> 
> iOS simulator failure looks like a real failure?
> 
>   media/no-fullscreen-when-hidden.html [ Failure ]

Yes, look related indeed. I verified it was passing on Mac.

[EWS Watchlist]

Comment on attachment 346489 [details]
Patch

Attachment 346489 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8752011

New failing tests:
media/no-fullscreen-when-hidden.html

[EWS Watchlist]

Created attachment 346503 [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.13.4

[Chris Dumez]

To Jer as there is apparently a security aspect to this and will requires a little bit more work.

[Darin Adler]

(In reply to Build Bot from comment #7)
> New failing tests:
> media/no-fullscreen-when-hidden.html

I’m guessing we have introduced a new race.

[EWS Watchlist]

Comment on attachment 346489 [details]
Patch

Attachment 346489 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8755092

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 346556 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Jer Noble]

Created attachment 355082 [details]
Patch

[EWS Watchlist]

Comment on attachment 355082 [details]
Patch

Attachment 355082 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10020628

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355093 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 355082 [details]
Patch

Attachment 355082 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10021010

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355098 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 355082 [details]
Patch

Attachment 355082 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10020766

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355101 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Jer Noble]

Created attachment 355123 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 355123 [details]
Patch

Clearing flags on attachment: 355123

Committed r238322: <https://trac.webkit.org/changeset/238322>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

I think this may have caused the following API test time outs:
Timeout

    TestWebKitAPI.FullscreenZoomInitialFrame.WebKit
    TestWebKitAPI.ExitFullscreenOnEnterPiP.ElementFullscreen

[Jer Noble]

Looks like this changed the order of IPC messages slightly, and the WKFullscreenWindowController doesn't get a chance to register as a VideoFullscreenModelContext client before the context tells everyone we entered PiP mode, breaking the test.

[Jer Noble]

Both of these tests are incorrect in different ways:

In TestWebKitAPI.ExitFullscreenOnEnterPiP.ElementFullscreen, the video element is not the main content; it's too small and it's not playing. We should only cancel fullscreen mode when PiP is entered if the video which was taken into PiP was main content. This worked before by accident; the video element entering PiP makes it the main content, but it wasn't when it was in fullscreen. The new behavior is actually correct; the test needs to be updated to make the video element explicitly the main content, in this case by playing the video during a user gesture.

In TestWebKitAPI.FullscreenZoomInitialFrame.WebKit, the WK1 WebView isn't visible at all; it shouldn't be allowed to go into fullscreen. The reason this worked previously is because the "GetIsViewVisible" UIClient method was not implemented in WK1, and it's default return value is true.

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 355186 [details]
Follow-up test fix

[WebKit Commit Bot]

Comment on attachment 355186 [details]
Follow-up test fix

Clearing flags on attachment: 355186

Committed r238348: <https://trac.webkit.org/changeset/238348>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.