Crash on Shutdown when triggering a ChromeWorker via xpcshell

NEW
Assigned to

Status

()

P3
critical
7 years ago
3 months ago

People

(Reporter: Fallen, Assigned: sinker)

Tracking

(Depends on: 1 bug, {crash})

11 Branch
x86_64
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 7 obsolete attachments)

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
(Reporter)

Updated

7 years ago
Version: unspecified → 11 Branch

Updated

7 years ago
Severity: normal → critical
Crash Signature: [ @ mozilla::dom::workers::RuntimeService::AutoSafeJSContext::GetSafeContext ] → [@ mozilla::dom::workers::RuntimeService::AutoSafeJSContext::GetSafeContext ]
Keywords: crash
(Assignee)

Updated

7 years ago
Assignee: nobody → tlee
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

7 years ago
For applications with windows, workers are canceled in nsGlobalWindow::FreeInnerObjects().
(Assignee)

Comment 3

7 years ago
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.
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.
(Assignee)

Comment 5

6 years ago
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".
(Assignee)

Comment 6

6 years ago
Created attachment 625007 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker
(Assignee)

Comment 7

6 years ago
Created attachment 625008 [details] [diff] [review]
Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown"
(Assignee)

Comment 8

6 years ago
Created attachment 625009 [details] [diff] [review]
Part 3: handle local file path as script URIs of workers
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?
(Assignee)

Comment 10

6 years ago
Created attachment 634748 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker, v2

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

Comment 11

6 years ago
Created attachment 634749 [details] [diff] [review]
Part 2: Cleanup at xpcom-will-shutdown, v2

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

Updated

6 years ago
Attachment #625007 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #625008 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
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.
Attachment #634749 - Attachment description: Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown", v2 → Part 2: Cleanup at xpcom-will-shutdown, v2
(Assignee)

Updated

6 years ago
Attachment #625009 - Flags: review?(bent.mozilla)
(Assignee)

Updated

6 years ago
Attachment #634748 - Flags: review?(bent.mozilla)
(Assignee)

Updated

6 years ago
Attachment #634749 - Flags: review?(bent.mozilla)
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 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")
Attachment #634749 - Flags: review?(bent.mozilla) → review+
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.
Attachment #625009 - Flags: review?(bent.mozilla) → review-
Attachment #634748 - Flags: review?(bent.mozilla) → review-
(Assignee)

Updated

6 years ago
Depends on: 767875
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
Created attachment 636252 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker, v2

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

Comment 18

6 years ago
Created attachment 636253 [details] [diff] [review]
Part 1: Testcase for shutdown crash with ChromeWorker, v3

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

Comment 19

6 years ago
Created attachment 636254 [details] [diff] [review]
Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown", v3

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

Updated

6 years ago
Attachment #634748 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #634749 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #636252 - Attachment is obsolete: true
(Assignee)

Comment 20

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

have moved to bug 767875.
Attachment #625009 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 636256 [details] [diff] [review]
Part 2: Notify "xpcom-shutdown-threads" before "xpcom-shutdown", v3

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

Updated

6 years ago
Attachment #636254 - Attachment is obsolete: true
(Assignee)

Comment 22

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

bent had r+ it.
Attachment #636256 - Flags: review+
(Assignee)

Comment 23

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

I had fixed the typo.
Attachment #636253 - Flags: review?(bent.mozilla)
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/"
Attachment #636253 - Flags: review?(bent.mozilla) → review+

Updated

6 months ago
See Also: → bug 988872, bug 1434618

Updated

6 months ago
See Also: → bug 1435343

Updated

3 months ago
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.