Bug 188307 - Regression(r233865): Causes synchronous IPC in the middle of layout
Summary: Regression(r233865): Causes synchronous IPC in the middle of layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks: 186226
  Show dependency treegraph
 
Reported: 2018-08-03 08:42 PDT by Chris Dumez
Modified: 2018-11-17 07:13 PST (History)
13 users (show)

See Also:


Attachments
Patch (7.45 KB, patch)
2018-08-03 08:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.35 MB, application/zip)
2018-08-03 10:35 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (13.03 MB, application/zip)
2018-08-03 15:54 PDT, EWS Watchlist
no flags Details
Patch (25.72 KB, patch)
2018-11-16 11:23 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.74 MB, application/zip)
2018-11-16 12:06 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.54 MB, application/zip)
2018-11-16 12:36 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (2.23 MB, application/zip)
2018-11-16 12:38 PST, EWS Watchlist
no flags Details
Patch (15.40 KB, patch)
2018-11-16 14:24 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Follow-up test fix (4.80 KB, patch)
2018-11-16 23:57 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 Chris Dumez 2018-08-03 08:42:59 PDT
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)
Comment 1 Chris Dumez 2018-08-03 08:43:13 PDT
<rdar://problem/42807306>
Comment 2 Chris Dumez 2018-08-03 08:52:51 PDT
Created attachment 346489 [details]
Patch
Comment 3 Chris Dumez 2018-08-03 09:02:49 PDT
@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.
Comment 4 Chris Dumez 2018-08-03 09:27:56 PDT
(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.
Comment 5 Geoffrey Garen 2018-08-03 09:53:54 PDT
Comment on attachment 346489 [details]
Patch

iOS simulator failure looks like a real failure?

  media/no-fullscreen-when-hidden.html [ Failure ]
Comment 6 Chris Dumez 2018-08-03 10:14:46 PDT
(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.
Comment 7 EWS Watchlist 2018-08-03 10:35:20 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-08-03 10:35:22 PDT Comment hidden (obsolete)
Comment 9 Chris Dumez 2018-08-03 10:53:26 PDT
To Jer as there is apparently a security aspect to this and will requires a little bit more work.
Comment 10 Darin Adler 2018-08-03 13:47:55 PDT
(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.
Comment 11 EWS Watchlist 2018-08-03 15:54:34 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-08-03 15:54:45 PDT Comment hidden (obsolete)
Comment 13 Jer Noble 2018-11-16 11:23:05 PST
Created attachment 355082 [details]
Patch
Comment 14 EWS Watchlist 2018-11-16 12:06:54 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-11-16 12:06:56 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-11-16 12:36:34 PST Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-11-16 12:36:36 PST Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-11-16 12:38:11 PST Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-11-16 12:38:13 PST Comment hidden (obsolete)
Comment 20 Jer Noble 2018-11-16 14:24:58 PST
Created attachment 355123 [details]
Patch
Comment 21 WebKit Commit Bot 2018-11-16 16:16:02 PST
Comment on attachment 355123 [details]
Patch

Clearing flags on attachment: 355123

Committed r238322: <https://trac.webkit.org/changeset/238322>
Comment 22 WebKit Commit Bot 2018-11-16 16:16:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Chris Dumez 2018-11-16 20:38:43 PST
I think this may have caused the following API test time outs:
Timeout

    TestWebKitAPI.FullscreenZoomInitialFrame.WebKit
    TestWebKitAPI.ExitFullscreenOnEnterPiP.ElementFullscreen
Comment 24 Jer Noble 2018-11-16 22:35:19 PST
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.
Comment 25 Jer Noble 2018-11-16 23:51:52 PST
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.
Comment 26 Jer Noble 2018-11-16 23:57:55 PST
Reopening to attach new patch.
Comment 27 Jer Noble 2018-11-16 23:57:56 PST
Created attachment 355186 [details]
Follow-up test fix
Comment 28 WebKit Commit Bot 2018-11-17 07:13:14 PST
Comment on attachment 355186 [details]
Follow-up test fix

Clearing flags on attachment: 355186

Committed r238348: <https://trac.webkit.org/changeset/238348>
Comment 29 WebKit Commit Bot 2018-11-17 07:13:16 PST
All reviewed patches have been landed.  Closing bug.