Closed
Bug 705178
Opened 13 years ago
Closed 6 years ago
Crash on Shutdown when triggering a ChromeWorker via xpcshell
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: Fallen, Assigned: sinker)
References
Details
(Keywords: crash)
Crash Data
Attachments
(2 files, 7 obsolete files)
2.04 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.04 KB,
patch
|
sinker
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Version: unspecified → 11 Branch
Updated•13 years ago
|
Severity: normal → critical
Crash Signature: [ @ mozilla::dom::workers::RuntimeService::AutoSafeJSContext::GetSafeContext ] → [@ mozilla::dom::workers::RuntimeService::AutoSafeJSContext::GetSafeContext ]
Keywords: crash
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → tlee
Assignee | ||
Comment 1•13 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•13 years ago
|
||
For applications with windows, workers are canceled in nsGlobalWindow::FreeInnerObjects().
Assignee | ||
Comment 3•13 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•13 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•13 years ago
|
||
Assignee | ||
Comment 7•13 years ago
|
||
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
--
Attachment #625007 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 11•13 years ago
|
||
--
Attachment #625008 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #625007 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #625008 -
Attachment is obsolete: true
Assignee | ||
Comment 12•13 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•13 years ago
|
Attachment #625009 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•13 years ago
|
Attachment #634748 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•13 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")
Updated•13 years ago
|
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-
Updated•13 years ago
|
Attachment #634748 -
Flags: review?(bent.mozilla) → review-
Assignee | ||
Comment 16•13 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•13 years ago
|
||
--
Attachment #634748 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 18•13 years ago
|
||
--
Attachment #634748 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 19•13 years ago
|
||
--
Attachment #634749 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #634748 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #634749 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #636252 -
Attachment is obsolete: true
Assignee | ||
Comment 20•13 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•13 years ago
|
||
--
Attachment #636254 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #636254 -
Attachment is obsolete: true
Assignee | ||
Comment 22•13 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•13 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•7 years ago
|
Updated•7 years ago
|
Priority: -- → P3
Comment 25•6 years ago
|
||
Closing because no crash reported since 12 weeks.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Comment 26•6 years ago
|
||
Closing because no crash reported since 12 weeks.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•