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
John Wilander
2018-08-20 14:12:48 PDT
This is categorized as a WebKit bug because it requires test infrastructure in the WebKit layer. This bug also covers <rdar://problem/38713823>. Created attachment 347545 [details]
Patch
Will skip the new test case on non-WK2 bots. iOS-sim is green. That's what I was waiting for. New patch with updated expectations coming. Created attachment 347550 [details]
Patch
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. 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 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 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
Discussed with Alex and new patch is coming up. Created attachment 347578 [details]
Patch
The WPE build fail doesn’t look related to me. On the WPE fail: g++-6: internal compiler error: Killed (program cc1plus) 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() Created attachment 347654 [details]
Patch for landing
Thanks, Alex! Fixed the isolated copy issue. Comment on attachment 347654 [details] Patch for landing Clearing flags on attachment: 347654 Committed r235124: <https://trac.webkit.org/changeset/235124> All reviewed patches have been landed. Closing bug. 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 (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. (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. Reopening to attach new patch. Created attachment 347818 [details]
Patch for landing
Comment on attachment 347818 [details]
Patch for landing
Wrong bug number.
Moving back to fixed. Addressing test flakiness in https://bugs.webkit.org/show_bug.cgi?id=188856. |