Bug 157068 – imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html is crashing on debug builds

• Component: Bindings
• Status: NEW

[youenn fablet]

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.

[Daniel Bates]

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.

[Daniel Bates]

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.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 329570 [details]
Patch to reduce test

[Alexey Proskuryakov]

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.

[Chris Dumez]

Where is the full stack trace?

[Ms2ger (he/him; ⌚ UTC+1/+2)]

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]

[youenn fablet]

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

[youenn fablet]

Created attachment 330374 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 330403 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 340181 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Comment on attachment 340181 [details]
Patch

win error is unrelated

[Ms2ger (he/him; ⌚ UTC+1/+2)]

*** Bug 173432 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

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?

[youenn fablet]

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.

[Alexey Proskuryakov]

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.

[youenn fablet]

Comment on attachment 340181 [details]
Patch

OK, will try to come up with something more aggressive.

[youenn fablet]

Created attachment 340699 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

> New failing tests:
> transitions/created-while-suspended.html

Failure seems unrelated

[Daniel Bates]

*** Bug 186978 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/41439184>

[Alexey Proskuryakov]

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.

[youenn fablet]

Created attachment 344632 [details]
Patch

[youenn fablet]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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.

[Alex Christensen]

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.

[Fujii Hironori]

No failure these days.
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Ffetch%2Fnosniff%2Fimportscripts.html
I'm going to unmark.

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/33375

[EWS]

Test gardening commit 283395@main (bb02b2a4a272): <https://commits.webkit.org/283395@main>

Reviewed commits have been landed. Closing PR #33375 and removing active labels.