Bug 188757 – Make ResourceLoadObserver::logWebSocketLoading() handle websockets in detached frames

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We should make WebCore::ResourceLoadObserver::logWebSocketLoading() handle websockets in detached frames (commented as a fixme in the code).

[John Wilander]

<rdar://problem/38713390>

[John Wilander]

This is categorized as a WebKit bug because it requires test infrastructure in the WebKit layer.

[John Wilander]

This bug also covers <rdar://problem/38713823>.

[John Wilander]

Created attachment 347545 [details]
Patch

[John Wilander]

Will skip the new test case on non-WK2 bots.

[John Wilander]

iOS-sim is green. That's what I was waiting for. New patch with updated expectations coming.

[John Wilander]

Created attachment 347550 [details]
Patch

[Alex Christensen]

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.

[John Wilander]

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()?

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

Discussed with Alex and new patch is coming up.

[John Wilander]

Created attachment 347578 [details]
Patch

[John Wilander]

The WPE build fail doesn’t look related to me.

[John Wilander]

On the WPE fail:
g++-6: internal compiler error: Killed (program cc1plus)

[Alex Christensen]

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()

[John Wilander]

Created attachment 347654 [details]
Patch for landing

[John Wilander]

Thanks, Alex! Fixed the isolated copy issue.

[WebKit Commit Bot]

Comment on attachment 347654 [details]
Patch for landing

Clearing flags on attachment: 347654

Committed r235124: <https://trac.webkit.org/changeset/235124>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[John Wilander]

(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.

[Ryan Haddad]

(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.

[John Wilander]

Reopening to attach new patch.

[John Wilander]

Created attachment 347818 [details]
Patch for landing

[John Wilander]

Comment on attachment 347818 [details]
Patch for landing

Wrong bug number.

[John Wilander]

Moving back to fixed. Addressing test flakiness in https://bugs.webkit.org/show_bug.cgi?id=188856.