Summary: | Make sure WebProcess::ensureNetworkProcessConnection() is always called on the main thread | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, commit-queue, ews-watchlist, joepeck, rniwa, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=170525 | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2018-07-12 10:14:12 PDT
Created attachment 344853 [details]
Patch
Comment on attachment 344853 [details] Patch Attachment 344853 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8517302 New failing tests: inspector/worker/runtime-basic.html http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html inspector/page/searchInResources.html Created attachment 344863 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Well, it is already finding bugs: Thread 7 Crashed:: WebCore: Worker 0 com.apple.WebKit 0x000000010bebb85b WebKit::WebProcess::ensureNetworkProcessConnection() + 265 (WebProcess.cpp:1117) 1 com.apple.WebKit 0x000000010be0e3ad WebKit::WebLoaderStrategy::setCaptureExtraNetworkLoadMetricsEnabled(bool) + 21 (DumbPtrTraits.h:41) 2 com.apple.WebCore 0x000000010de38e27 WebCore::WorkerInspectorController::disconnectFrontend(Inspector::DisconnectReason) + 39 (WorkerInspectorController.cpp:132) 3 com.apple.WebCore 0x000000010e4de8aa WTF::Function<void (WebCore::ScriptExecutionContext&)>::CallableWrapper<WebCore::WorkerInspectorProxy::disconnectFromWorkerInspectorController()::$_6>::call(WebCore::ScriptExecutionContext&) + 26 (WorkerGlobalScope.h:81) 4 com.apple.WebCore 0x000000010e4e0769 WebCore::WorkerRunLoop::runInMode(WebCore::WorkerGlobalScope*, WebCore::ModePredicate const&, WebCore::WorkerRunLoop::WaitMode) + 441 (Function.h:56) 5 com.apple.WebCore 0x000000010e4e0550 WebCore::WorkerRunLoop::run(WebCore::WorkerGlobalScope*) + 96 (WorkerRunLoop.cpp:138) 6 com.apple.WebCore 0x000000010e4e3361 WebCore::WorkerThread::workerThread() + 1025 (RefPtr.h:59) 7 com.apple.JavaScriptCore 0x0000000110f46184 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 212 (memory:2735) 8 com.apple.JavaScriptCore 0x0000000110f47d09 WTF::wtfThreadEntryPoint(void*) + 9 (ThreadingPthreads.cpp:228) 9 libsystem_pthread.dylib 0x00007fffb3ad293b _pthread_body + 180 10 libsystem_pthread.dylib 0x00007fffb3ad2887 _pthread_start + 286 11 libsystem_pthread.dylib 0x00007fffb3ad208d thread_start + 13 +Joe who added this code. (In reply to Chris Dumez from comment #5) > +Joe who added this code. We could callOnMainThread() before using the WebLoaderStrategy. However, the WebInspector code relies on a s_frontendCounter counter which is a static int and is now incremented / decremented from various threads. Cannot we do all these incrementing/decrementing in the main thread? Maybe we need callOnMainThreadAndWait. Created attachment 344875 [details]
Patch
Comment on attachment 344875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344875&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:1466 > + if (!isMainThread()) { > + callOnMainThread([] { > + InspectorInstrumentation::frontendCreated(); > + }); > + return; > + } The s_frontendCounter is consulted for the fast return on most InspectorInstrumentation functions. If this is async, we might end up ignoring some InspectorInstrumentation hooks for a short period of time that we ordinarily would not have ignored. I don't know if I have a concrete example of what would go wrong though. I imagine the only case that could be affected by this is a Service Worker inspector (since Web Inspectors's Dedicated Workers would have had this already happen on the main thread already). I think its reasonable to give this a shot. You could avoid the isMainThreadCheck if you put the callOnMainThread inside of WorkerInspectorController. InspectorController's should always be on the main thread. callOnMainThreadAndWait? Created attachment 344880 [details]
Patch
Comment on attachment 344880 [details] Patch Attachment 344880 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8519486 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html Created attachment 344890 [details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344880 [details] Patch Clearing flags on attachment: 344880 Committed r233784: <https://trac.webkit.org/changeset/233784> All reviewed patches have been landed. Closing bug. |