Bug 157068

Summary: imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html is crashing on debug builds
Product: WebKit Reporter: youenn fablet <youennf>
Component: BindingsAssignee: youenn fablet <youennf>
Status: NEW ---    
Severity: Normal CC: achristensen, ap, cdumez, dbates, esprehn+autocc, ews-watchlist, jlewis3, kondapallykalyan, Ms2ger, 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=171248
https://bugs.webkit.org/show_bug.cgi?id=173432
Attachments:
Description Flags
Patch to reduce test
none
Patch
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews205 for win-future none

Description youenn fablet 2016-04-27 06:08:10 PDT
Assertion: ASSERT(m_mainThreadLoader || m_loadingFinished) is hit in Source/WebCore/loader/WorkerThreadableLoader.cpp.
It seems that returned loader is null in all cases when CachedResource::load fails early.
Comment 1 Daniel Bates 2017-04-25 13:31:43 PDT
Currently worker-imported scripts, as tested by <https://github.com/w3c/web-platform-tests/blob/master/fetch/nosniff/importscripts.html>, do not respect X-Content-Type-Options: nosniff. We will implement such support in bug #171248.
Comment 2 Daniel Bates 2017-04-25 13:34:19 PDT
I am assuming you mean that running the test represented by <https://github.com/w3c/web-platform-tests/blob/master/fetch/nosniff/importscripts.html> causes a crash as opposed to accessing <https://github.com/w3c/web-platform-tests/blob/master/fetch/nosniff/importscripts.html>.

For completeness, the test <https://github.com/w3c/web-platform-tests/blob/master/fetch/nosniff/importscripts.html> has since been imported into LayoutTests/imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html.
Comment 3 Ms2ger (he/him; ⌚ UTC+1/+2) 2017-12-16 01:35:54 PST
This now hits

ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction

I can reproduce quite easily when running in a loop. I'll attach a patch that reduces the test case somewhat.
Comment 4 Ms2ger (he/him; ⌚ UTC+1/+2) 2017-12-16 01:36:41 PST
Created attachment 329570 [details]
Patch to reduce test
Comment 5 Alexey Proskuryakov 2017-12-18 09:30:48 PST
This assertion generally means that an event listener wasn't protected from GC, and got collected. That would be a pretty bad bug for Fetch if so.
Comment 6 Chris Dumez 2017-12-18 09:38:06 PST
Where is the full stack trace?
Comment 7 Ms2ger (he/him; ⌚ UTC+1/+2) 2017-12-19 00:06:10 PST
I don't know that it's a bug in fetch code, particularly. It's the onmessage handler for the Worker that disappears.

STDERR: ../../Source/WebCore/bindings/js/JSEventListener.h(128) : JSC::JSObject* WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext&) const
STDERR: 1   0x7f717617d147 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7f717617d147]
STDERR: 2   0x7f718333e1c3 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext&) const+0x1a5) [0x7f718333e1c3]
STDERR: 3   0x7f718333c5a8 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)+0xf6) [0x7f718333c5a8]
STDERR: 4   0x7f7183705dac /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>)+0x3bc) [0x7f7183705dac]
STDERR: 5   0x7f71837058bb /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::EventTarget::fireEventListeners(WebCore::Event&)+0x11f) [0x7f71837058bb]
STDERR: 6   0x7f718370564b /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::EventTarget::dispatchEvent(WebCore::Event&)+0xf3) [0x7f718370564b]
STDERR: 7   0x7f718471db8f /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xa6bdb8f) [0x7f718471db8f]
STDERR: 8   0x7f718472aa96 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xa6caa96) [0x7f718472aa96]
STDERR: 9   0x7f71831f9848 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WTF::Function<void (WebCore::ScriptExecutionContext&)>::operator()(WebCore::ScriptExecutionContext&) const+0x78) [0x7f71831f9848]
STDERR: 10  0x7f71831f83a7 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::ScriptExecutionContext::Task::performTask(WebCore::ScriptExecutionContext&)+0x23) [0x7f71831f83a7]
STDERR: 11  0x7f718366c6e4 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x960c6e4) [0x7f718366c6e4]
STDERR: 12  0x7f718367e548 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x961e548) [0x7f718367e548]
STDERR: 13  0x7f7181aa0398 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WTF::Function<void ()>::operator()() const+0x5e) [0x7f7181aa0398]
STDERR: 14  0x7f71761980ef /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTF::dispatchFunctionsFromMainThread()+0x105) [0x7f71761980ef]
STDERR: 15  0x7f71761fc8df /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTF::MainThreadDispatcher::fired()+0x11) [0x7f71761fc8df]
STDERR: 16  0x7f71761fca44 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTF::RunLoop::Timer<WTF::MainThreadDispatcher>::fired()+0x66) [0x7f71761fca44]
STDERR: 17  0x7f71761ffb81 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x2d73b81) [0x7f71761ffb81]
STDERR: 18  0x7f71761ffbbd /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x2d73bbd) [0x7f71761ffbbd]
STDERR: 19  0x7f71761ff242 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x2d73242) [0x7f71761ff242]
STDERR: 20  0x7f71761ff271 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x2d73271) [0x7f71761ff271]
STDERR: 21  0x7f7177426a4a /WebKit/GTK/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_context_dispatch+0x15a) [0x7f7177426a4a]
STDERR: 22  0x7f7177426df8 /WebKit/GTK/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x4adf8) [0x7f7177426df8]
STDERR: 23  0x7f7177427122 /WebKit/GTK/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_loop_run+0xc2) [0x7f7177427122]
STDERR: 24  0x7f71761ff7c8 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTF::RunLoop::run()+0xba) [0x7f71761ff7c8]
STDERR: 25  0x7f71823fb0e6 /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**)+0x91) [0x7f71823fb0e6]
STDERR: 26  0x7f71823faf8a /WebKit/GTK/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebProcessMainUnix+0x20) [0x7f71823faf8a]
STDERR: 27  0x4009b1 /WebKit/GTK/WebKit/WebKitBuild/Debug/bin/WebKitWebProcess(main+0x3d) [0x4009b1]
STDERR: 28  0x7f71799a7830 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7f71799a7830]
STDERR: 29  0x400869 /WebKit/GTK/WebKit/WebKitBuild/Debug/bin/WebKitWebProcess(_start+0x29) [0x400869]
Comment 8 youenn fablet 2018-01-03 05:26:01 PST
(In reply to Ms2ger from comment #7)
> I don't know that it's a bug in fetch code, particularly. It's the onmessage
> handler for the Worker that disappears.

Looking at the code, JSWorker is not collectable when loading the script.
After that point, it is again collectable.
I guess we could extend the fact that it is not collectable until being terminated.
Or if it has an on message event handler.
Comment 9 youenn fablet 2018-01-03 05:30:15 PST
Created attachment 330374 [details]
Patch
Comment 10 EWS Watchlist 2018-01-03 07:02:40 PST
Comment on attachment 330374 [details]
Patch

Attachment 330374 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5909433

New failing tests:
fast/workers/dedicated-worker-lifecycle.html
fast/workers/worker-exception-during-navigation.html
Comment 11 EWS Watchlist 2018-01-03 07:02:42 PST
Created attachment 330379 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 12 EWS Watchlist 2018-01-03 10:18:42 PST
Comment on attachment 330374 [details]
Patch

Attachment 330374 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5915689

New failing tests:
inspector/worker/worker-create-and-terminate.html
fast/workers/termination-with-port-messages.html
fast/workers/worker-document-leak.html
fast/workers/worker-terminate.html
Comment 13 EWS Watchlist 2018-01-03 10:18:43 PST
Created attachment 330399 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 14 EWS Watchlist 2018-01-03 10:34:33 PST
Comment on attachment 330374 [details]
Patch

Attachment 330374 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5915958

New failing tests:
fast/workers/termination-with-port-messages.html
html5lib/generated/run-comments01-data.html
fast/workers/dedicated-worker-lifecycle.html
inspector/worker/worker-create-and-terminate.html
fast/workers/worker-terminate.html
inspector/unit-tests/test-harness-expect-functions-async.html
Comment 15 EWS Watchlist 2018-01-03 10:34:34 PST
Created attachment 330402 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 youenn fablet 2018-01-03 10:59:53 PST
Created attachment 330403 [details]
Patch
Comment 17 EWS Watchlist 2018-01-03 11:57:44 PST
Comment on attachment 330403 [details]
Patch

Attachment 330403 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5916834

New failing tests:
fast/workers/worker-document-leak.html
http/wpt/resource-timing/rt-initiatorType-other.html
Comment 18 EWS Watchlist 2018-01-03 11:57:46 PST
Created attachment 330406 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 19 EWS Watchlist 2018-01-03 12:39:30 PST
Comment on attachment 330403 [details]
Patch

Attachment 330403 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5917209

New failing tests:
fast/workers/worker-document-leak.html
Comment 20 EWS Watchlist 2018-01-03 12:39:31 PST
Created attachment 330410 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 EWS Watchlist 2018-01-03 13:09:11 PST
Comment on attachment 330403 [details]
Patch

Attachment 330403 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5917717

New failing tests:
fast/mediastream/MediaStream-MediaElement-setObject-null.html
Comment 22 EWS Watchlist 2018-01-03 13:09:13 PST
Created attachment 330412 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 23 youenn fablet 2018-05-11 02:39:03 PDT
Created attachment 340181 [details]
Patch
Comment 24 EWS Watchlist 2018-05-11 04:49:41 PDT
Comment on attachment 340181 [details]
Patch

Attachment 340181 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7650044

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 25 EWS Watchlist 2018-05-11 04:49:53 PDT
Created attachment 340188 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 26 youenn fablet 2018-05-11 04:51:01 PDT
Comment on attachment 340181 [details]
Patch

win error is unrelated
Comment 27 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-05-11 05:01:38 PDT
*** Bug 173432 has been marked as a duplicate of this bug. ***
Comment 28 Alexey Proskuryakov 2018-05-11 09:31:42 PDT
Comment on attachment 340181 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Make Worker uncollectable if it has an onmessage event handler.

Doesn't this mean that it's essentially never collectable?
Comment 29 youenn fablet 2018-05-11 10:08:09 PDT
If onmessage is set, we cannot let the worker be collected since we do not know if a message will have to be dispatched in the future.
A web page can always stop the worker or unset onmessage.
Comment 30 Alexey Proskuryakov 2018-05-11 10:40:32 PDT
The fact that it's possible for a page to do something explicit to avoid performance problems is not enough for us. What if it doesn't?

Worker lifetime is directly addressed in the spec, <https://w3c.github.io/workers/#worker-lifetime>. Having an onmessage handler on Worker is not the end of the story.
Comment 31 youenn fablet 2018-05-14 07:45:19 PDT
Comment on attachment 340181 [details]
Patch

OK, will try to come up with something more aggressive.
Comment 32 youenn fablet 2018-05-18 08:34:01 PDT
Created attachment 340699 [details]
Patch
Comment 33 EWS Watchlist 2018-05-18 09:49:57 PDT
Comment on attachment 340699 [details]
Patch

Attachment 340699 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7722599

New failing tests:
transitions/created-while-suspended.html
Comment 34 EWS Watchlist 2018-05-18 09:49:58 PDT
Created attachment 340701 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 35 youenn fablet 2018-05-18 10:30:35 PDT
> New failing tests:
> transitions/created-while-suspended.html

Failure seems unrelated
Comment 36 Daniel Bates 2018-06-25 14:00:47 PDT
*** Bug 186978 has been marked as a duplicate of this bug. ***
Comment 37 Radar WebKit Bug Importer 2018-06-25 14:05:37 PDT
<rdar://problem/41439184>
Comment 38 Alexey Proskuryakov 2018-06-26 01:37:19 PDT
Comment on attachment 340699 [details]
Patch

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

> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:87
> +    thread().runLoop().postTaskForMode([](auto& context) {

Can some assertions or comments be added to clarify why m_importScriptCount isn't being accessed from multiple threads concurrently? I'm assuming that's true, but I don't know how to confirm that.

Other that that, leaving for someone else to review.
Comment 39 youenn fablet 2018-07-09 15:52:06 PDT
Created attachment 344632 [details]
Patch
Comment 40 youenn fablet 2018-07-09 16:24:42 PDT
(In reply to Alexey Proskuryakov from comment #38)
> Comment on attachment 340699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340699&action=review
> 
> > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:87
> > +    thread().runLoop().postTaskForMode([](auto& context) {
> 
> Can some assertions or comments be added to clarify why m_importScriptCount
> isn't being accessed from multiple threads concurrently? I'm assuming that's
> true, but I don't know how to confirm that.

Added a comment.
importScripts is called in the worker thread and the lambda as well so m_importScriptCount is only accessed from the worker thread.
Comment 41 EWS Watchlist 2018-07-10 00:26:45 PDT
Comment on attachment 344632 [details]
Patch

Attachment 344632 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8491450

New failing tests:
http/tests/preload/onload_event.html
Comment 42 EWS Watchlist 2018-07-10 00:26:57 PDT
Created attachment 344676 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 43 Chris Dumez 2018-07-17 08:34:42 PDT
Comment on attachment 344632 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Decrement the counter after an even loop iteration to handle the case of importScripts being called one after the other.

s/even/event/

> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:-83
> -    thread().workerObjectProxy().reportPendingActivity(hasPendingActivity());

So hasPendingActivity() has to do with ActiveDOMObjects, not loads. Could you explain to me why moving the call to thread().workerObjectProxy().reportPendingActivity(hasPendingActivity()); before the call to Base::importScripts(urls) work?

> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:87
> +    // We decrement m_importScriptCount asycnhronously in case another importScripts is made synchronously. m_importScriptCount is modified only in Worker thread.

Typo: asycnhronously

> Source/WebCore/workers/DedicatedWorkerGlobalScope.h:73
> +    static bool isType(const WebCore::ScriptExecutionContext& context)

We should add an overload that takes a WorkerGlobalScope& in parameter. Then the overload that takes a ScriptExecutionContext& could do the is<WebCore::WorkerGlobalScope>(context) and call this new one.
Comment 44 Alex Christensen 2021-11-01 12:48:22 PDT
Comment on attachment 344632 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.