705178 - Crash on Shutdown when triggering a ChromeWorker via xpcshell

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P3)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I don't have a firefox build, so I can't create a reduced testcase, but here is what I was doing:

I have an xpcom component implemented in js, that creates a ChromeWorker. The worker script is loaded via resource:// uri. The worker did not explicitly call close() so it seems it was left running.

Then I wrote an xpcshell unit test that calls the method on the xpcom component. Merely calling the worker causes xpcshell to crash on shutdown with the following backtrace:

#0  0x00002b02c69bde3c in mozilla::dom::workers::RuntimeService::AutoSafeJSContext::GetSafeContext () at /home/kewisch/mozilla/comm-central/mozilla/dom/workers/RuntimeService.cpp:1235
#1  0x00002b02c69c2d3b in mozilla::dom::workers::WorkerRunnable::Run (this=0x2b02d95cdd00) at /home/kewisch/mozilla/comm-central/mozilla/dom/workers/WorkerPrivate.cpp:1587
#2  0x00002b02c7125804 in nsThread::ProcessNextEvent (this=0x2b02d160ccc0, mayWait=true, result=0x7fff59847caf) at /home/kewisch/mozilla/comm-central/mozilla/xpcom/threads/nsThread.cpp:631
#3  0x00002b02c70f90c3 in NS_ProcessNextEvent_P (thread=<value optimized out>, mayWait=true) at /home/kewisch/mozilla/comm-central/obj-x86_64-unknown-linux-gnu/mozilla/xpcom/build/nsThreadUtils.cpp:245
#4  0x00002b02c7125adc in nsThread::Shutdown (this=0x2b02d16a9c10) at /home/kewisch/mozilla/comm-central/mozilla/xpcom/threads/nsThread.cpp:494
#5  0x00002b02c6557c2b in nsSocketTransportService::Shutdown (this=0x2b02d16a9ab0) at /home/kewisch/mozilla/comm-central/mozilla/netwerk/base/src/nsSocketTransportService2.cpp:526
#6  0x00002b02c6543cd3 in nsIOService::SetOffline (this=0x2b02d1ffb210, offline=<value optimized out>) at /home/kewisch/mozilla/comm-central/mozilla/netwerk/base/src/nsIOService.cpp:802
#7  0x00002b02c6545c78 in nsIOService::Observe (this=0x2b02d1ffb210, subject=0x2b02d16661c8, topic=0x2b02c74e4805 "xpcom-shutdown", data=0x0) at /home/kewisch/mozilla/comm-central/mozilla/netwerk/base/src/nsIOService.cpp:1019
#8  0x00002b02c7106aba in nsObserverList::NotifyObservers (this=<value optimized out>, aSubject=0x2b02d16661c8, aTopic=0x2b02c74e4805 "xpcom-shutdown", someData=0x0)
    at /home/kewisch/mozilla/comm-central/mozilla/xpcom/ds/nsObserverList.cpp:130
#9  0x00002b02c7106dd0 in nsObserverService::NotifyObservers (this=0x2b02d16b0200, aSubject=0x2b02d16661c8, aTopic=0x2b02c74e4805 "xpcom-shutdown", someData=0x0)
    at /home/kewisch/mozilla/comm-central/mozilla/xpcom/ds/nsObserverService.cpp:182
#10 0x00002b02c70fcfa8 in mozilla::ShutdownXPCOM (servMgr=0x0) at /home/kewisch/mozilla/comm-central/mozilla/xpcom/build/nsXPComInit.cpp:598
#11 0x0000000000406d03 in main (argc=1, argv=0x7fff59848340, envp=<value optimized out>) at /home/kewisch/mozilla/comm-central/mozilla/js/xpconnect/shell/xpcshell.cpp:2038

Comment 1

[Thinker Li [:sinker]]

For Firefox or browsers, work queues of all workers associated with web pages or chrome are cancel before shutdown. But, for xpcshell, there is no chrome and web page, so workers are not cancel following pages.  The solution is to cancel all workers before context being free.

Comment 2

[Thinker Li [:sinker]]

For applications with windows, workers are canceled in nsGlobalWindow::FreeInnerObjects().

Comment 3

[Thinker Li [:sinker]]

I think mozilla::dom::worker::RuntimeService should expose an interface that can be used by nsGlobalWindow and xpcshell.  nsGlobalWindow calls mozilla::dom::worker::CancelWorkersForWindow() directly.  But, it can not be reached from the application; for example, xpcshell.  so, we need an interface.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The worker runtime service has code to shut down all workers in response to the "xpcom-shutdown-threads" observer topic. It should be unnecessary to expose any further API for this unless I am misunderstanding something.

The crash in comment 0 is actually something different. First, "xpcom-shutdown-threads" is probably incorrect. We really want "xpcom-shutdown". Even that won't solve the problem though. We use nsContentUtils::ThreadJSContextStack which returns a null pointer after layout's LayoutShutdownObserver runs. If worker shutdown happens after that then we're hosed.

Comment 5

[Thinker Li [:sinker]]

Listening to "xpcom-shutdow" introduces some order issues.  Some actions happened when clean-up workers depend on other modules.  They are shutdown in unknown order.  For example, I found scriptloader depends nsDirectoryService, it would run into a problem if we clear-up workers before the completion of loading.  It may be better to move "xpcom-shutdown-threads" to between "xpcom-shutdown" and "xpcom-will-shutdown".

Comment 6

[Thinker Li [:sinker]]

Attachment: Part 1: Testcase for shutdown crash with ChromeWorker

Comment 7

[Thinker Li [:sinker]]

Attachment: Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown"

Comment 8

[Thinker Li [:sinker]]

Attachment: Part 3: handle local file path as script URIs of workers

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't think changing the order of the "xpcom-shutdown-threads" and "xpcom-shutdown" is a good idea. Both topics have been around for a long time and there could be lots of code that depends on that order.

For now, why not make workers shut down at "xpcom-will-shutdown" instead?

Comment 10

[Thinker Li [:sinker]]

Attachment: Part 1: Testcase for shutdown crash with ChromeWorker, v2

--
Attachment #625007 [details] [diff] - Attachment is obsolete: true

Comment 11

[Thinker Li [:sinker]]

Attachment: Part 2: Cleanup at xpcom-will-shutdown, v2

--
Attachment #625008 [details] [diff] - Attachment is obsolete: true

Comment 12

[Thinker Li [:sinker]]

Comment on attachment 634749 [details] [diff] [review]
Part 2: Cleanup at xpcom-will-shutdown, v2

Cleanup at xpcom-will-shutdown to avoid potential ordering issue of xpcom-shutdown.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 634748 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker, v2

Review of attachment 634748 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, can we put this test in dom/workers/test/unit instead of dom/workers/tests?

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 634748 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker, v2

Review of attachment 634748 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/tests/test_shutdown_crash.js
@@ +1,2 @@
> +function run_test() {
> +    let testworker = new ChromeWorker("shtudown_crash_worker.js");

There's a typo here ("shtudown_crash_worker")

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 625009 [details] [diff] [review]
Part 3: handle local file path as script URIs of workers

Review of attachment 625009 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't really related to this bug, and I'm not sure we should do this. If you feel that it's important please file a new bug about this.

Comment 16

[Thinker Li [:sinker]]

(In reply to ben turner [:bent] from comment #15)
> Comment on attachment 625009 [details] [diff] [review]
> Part 3: handle local file path as script URIs of workers
> 
> Review of attachment 625009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This isn't really related to this bug, and I'm not sure we should do this.
> If you feel that it's important please file a new bug about this.

I have move the patch to bug 767875.

Comment 17

[Thinker Li [:sinker]]

Attachment: Part 1: Testcase for shutdown crash with ChromeWorker, v2

--
Attachment #634748 [details] [diff] - Attachment is obsolete: true

Comment 18

[Thinker Li [:sinker]]

Attachment: Part 1: Testcase for shutdown crash with ChromeWorker, v3

--
Attachment #634748 [details] [diff] - Attachment is obsolete: true

Comment 19

[Thinker Li [:sinker]]

Attachment: Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown", v3

--
Attachment #634749 [details] [diff] - Attachment is obsolete: true

Comment 20

[Thinker Li [:sinker]]

Comment on attachment 625009 [details] [diff] [review]
Part 3: handle local file path as script URIs of workers

have moved to bug 767875.

Comment 21

[Thinker Li [:sinker]]

Attachment: Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown", v3

--
Attachment #636254 [details] [diff] - Attachment is obsolete: true

Comment 22

[Thinker Li [:sinker]]

Comment on attachment 636256 [details] [diff] [review]
Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown", v3

bent had r+ it.

Comment 23

[Thinker Li [:sinker]]

Comment on attachment 636253 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker, v3

I had fixed the typo.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 636253 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker, v3

Review of attachment 636253 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this one fix.

::: dom/workers/test/Makefile.in
@@ +15,5 @@
>    extensions \
>    $(NULL)
>  
> +ifdef ENABLE_TESTS
> +XPCSHELL_TESTS = unit/

Just "unit", not "unit/"

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.