Bug 188757

Summary: Make ResourceLoadObserver::logWebSocketLoading() handle websockets in detached frames
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, ews-watchlist, rniwa, ryanhaddad, tsavell
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=188856
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Patch
none
Patch for landing
none
Patch for landing none

Description John Wilander 2018-08-20 14:12:48 PDT
We should make WebCore::ResourceLoadObserver::logWebSocketLoading() handle websockets in detached frames (commented as a fixme in the code).
Comment 1 John Wilander 2018-08-20 14:13:14 PDT
<rdar://problem/38713390>
Comment 2 John Wilander 2018-08-20 14:13:52 PDT
This is categorized as a WebKit bug because it requires test infrastructure in the WebKit layer.
Comment 3 John Wilander 2018-08-20 14:26:56 PDT
This bug also covers <rdar://problem/38713823>.
Comment 4 John Wilander 2018-08-20 14:29:18 PDT
Created attachment 347545 [details]
Patch
Comment 5 John Wilander 2018-08-20 15:13:27 PDT
Will skip the new test case on non-WK2 bots.
Comment 6 John Wilander 2018-08-20 15:29:53 PDT
iOS-sim is green. That's what I was waiting for. New patch with updated expectations coming.
Comment 7 John Wilander 2018-08-20 15:31:19 PDT
Created attachment 347550 [details]
Patch
Comment 8 Alex Christensen 2018-08-20 16:27:05 PDT
Comment on attachment 347550 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:173
>          return;

indent more

> Source/WebCore/loader/ResourceLoadObserver.cpp:184
> +    if (!shouldLog(document.page()->usesEphemeralSession()))

Either move assertion up or check page for null.
Comment 9 John Wilander 2018-08-20 16:48:29 PDT
Thanks, Alex!

I seem to have one obstacle left. For seven existing WebSockets test, I get test runner crashes like this:

ASSERTION FAILED: m_thread.ptr() == &Thread::current()
/Users/john_wilander/dev/WebKitSafari/OpenSource/Source/WebCore/platform/Timer.h(141) : bool WebCore::TimerBase::isActive() const
1   0x3a8a39c89 WTFCrash
2   0x39800db6b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x398099520 WebCore::TimerBase::isActive() const
4   0x39a818fe2 WebCore::ResourceLoadObserver::scheduleNotificationIfNeeded()
5   0x39a8193b7 WebCore::ResourceLoadObserver::logWebSocketLoading(WebCore::ScriptExecutionContext&, WebCore::URL const&)

I believe this never happened before because we skipped those cases, but it is handled in the WebSockets code through this:

#if USE(WEB_THREAD)
            ref();
            dispatch_async(dispatch_get_main_queue(), ^{
                WebThreadRun(^{
                    // Do things here.
                    deref();
                });
            });
#else
            RunLoop::main().dispatch([this, protectedThis = makeRef(*this)]() {
                // Do things here.
            });
#endif

Any suggestions for how I should handle my call to ResourceLoadObserver::logWebSocketLoading()?
Comment 10 EWS Watchlist 2018-08-20 17:07:11 PDT
Comment on attachment 347550 [details]
Patch

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

New failing tests:
http/tests/websocket/tests/hybi/workers/close-in-onmessage-crash.html
http/tests/websocket/tests/hybi/contentextensions/display-none-worker.html
http/tests/websocket/tests/hybi/workers/worker-reload.html
http/tests/websocket/tests/hybi/workers/close-in-worker.html
http/tests/websocket/tests/hybi/workers/worker-simple.html
http/tests/websocket/tests/hybi/contentextensions/block-worker.html
http/tests/websocket/tests/hybi/workers/worker-handshake-challenge-randomness.html
Comment 11 EWS Watchlist 2018-08-20 17:07:13 PDT
Created attachment 347575 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 12 John Wilander 2018-08-20 17:23:59 PDT
Discussed with Alex and new patch is coming up.
Comment 13 John Wilander 2018-08-20 17:25:53 PDT
Created attachment 347578 [details]
Patch
Comment 14 John Wilander 2018-08-21 08:53:22 PDT
The WPE build fail doesn’t look related to me.
Comment 15 John Wilander 2018-08-21 10:07:30 PDT
On the WPE fail:
g++-6: internal compiler error: Killed (program cc1plus)
Comment 16 Alex Christensen 2018-08-21 10:10:03 PDT
Comment on attachment 347578 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:282
> +    RunLoop::main().dispatch([targetURL = m_url, mainFrameURL = context.url(), usesEphemeralSession = context.sessionID().isEphemeral()]() {

.isolaotedCopy()
Comment 17 John Wilander 2018-08-21 10:22:12 PDT
Created attachment 347654 [details]
Patch for landing
Comment 18 John Wilander 2018-08-21 10:22:41 PDT
Thanks, Alex! Fixed the isolated copy issue.
Comment 19 WebKit Commit Bot 2018-08-21 11:02:41 PDT
Comment on attachment 347654 [details]
Patch for landing

Clearing flags on attachment: 347654

Committed r235124: <https://trac.webkit.org/changeset/235124>
Comment 20 WebKit Commit Bot 2018-08-21 11:02:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Truitt Savell 2018-08-21 14:02:02 PDT
The new test: http/tests/websocket/construct-in-detached-frame-resource-load-statistics.html

is flakey from:
https://trac.webkit.org/changeset/235124/webkit

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Fconstruct-in-detached-frame-resource-load-statistics.html
Comment 22 John Wilander 2018-08-21 14:22:01 PDT
(In reply to Truitt Savell from comment #21)
> The new test:
> http/tests/websocket/construct-in-detached-frame-resource-load-statistics.
> html
> 
> is flakey from:
> https://trac.webkit.org/changeset/235124/webkit
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Fconstruct-in-detached-
> frame-resource-load-statistics.html

Got it. I'll look into it. It's OK to mark it as flaky for now. It may be that executing code in detached frames isn't a reliable thing. The test case this was based off of was for a crasher.
Comment 23 Ryan Haddad 2018-08-21 19:26:11 PDT
(In reply to John Wilander from comment #22)
> (In reply to Truitt Savell from comment #21)
> > The new test:
> > http/tests/websocket/construct-in-detached-frame-resource-load-statistics.
> > html
> > 
> > is flakey from:
> > https://trac.webkit.org/changeset/235124/webkit
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Fconstruct-in-detached-
> > frame-resource-load-statistics.html
> 
> Got it. I'll look into it. It's OK to mark it as flaky for now. It may be
> that executing code in detached frames isn't a reliable thing. The test case
> this was based off of was for a crasher.
We shouldn't mark new tests as flaky. We should roll out the change and re-land it when we have a working test.
Comment 24 John Wilander 2018-08-22 10:44:02 PDT
Reopening to attach new patch.
Comment 25 John Wilander 2018-08-22 10:44:03 PDT
Created attachment 347818 [details]
Patch for landing
Comment 26 John Wilander 2018-08-22 10:44:34 PDT
Comment on attachment 347818 [details]
Patch for landing

Wrong bug number.
Comment 27 John Wilander 2018-08-22 10:46:39 PDT
Moving back to fixed. Addressing test flakiness in https://bugs.webkit.org/show_bug.cgi?id=188856.