If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Nightly crash while executing service workers test suite

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: noemi, Assigned: bkelly)

Tracking

(Blocks: 1 bug)

Trunk
mozilla40
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8589150 [details]
NightlyDebug_Crash_4_7_master_build.rtf

Nightly crash while executing service workers test suite:
"Assertion failure: !mValid, at /Users/noef/Documents/mozilla-central/dom/cache/Manager.cpp:1427" error appears. Please find below the corresponding traces:

++DOMWINDOW == 132 (0x11f13d800) [pid = 75414] [serial = 170] [outer = 0x126eaac00]
[75414] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file /Users/noef/Documents/mozilla-central/docshell/base/nsDocShell.cpp, line 4597
--DOCSHELL 0x11f5c1800 == 11 [pid = 75414] [id = 21]
[75414] WARNING: Called close() before start()!: 'mStarted', file /Users/noef/Documents/mozilla-central/dom/workers/MessagePort.cpp, line 214
[75414] WARNING: Called close() before start()!: 'mStarted', file /Users/noef/Documents/mozilla-central/dom/workers/MessagePort.cpp, line 214
[75414] WARNING: Called close() before start()!: 'mStarted', file /Users/noef/Documents/mozilla-central/dom/workers/MessagePort.cpp, line 214
###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
[75414] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file /Users/noef/Documents/mozilla-central/docshell/base/nsDocShell.cpp, line 4597
[75414] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file /Users/noef/Documents/mozilla-central/docshell/base/nsDocShell.cpp, line 4597
--DOCSHELL 0x113017800 == 10 [pid = 75414] [id = 1]
[75414] WARNING: A runnable was posted to a worker that is already shutting down!: file /Users/noef/Documents/mozilla-central/dom/workers/WorkerPrivate.cpp, line 2782
[75414] WARNING: Failed to dispatch offline status change event!: file /Users/noef/Documents/mozilla-central/dom/workers/WorkerPrivate.cpp, line 3631
--DOCSHELL 0x112ad6800 == 9 [pid = 75414] [id = 22]
--DOCSHELL 0x111b54800 == 8 [pid = 75414] [id = 47]
--DOCSHELL 0x11f2a5800 == 7 [pid = 75414] [id = 3]
--DOCSHELL 0x11c5bb000 == 6 [pid = 75414] [id = 2]
--DOCSHELL 0x11f2ac800 == 5 [pid = 75414] [id = 4]
Assertion failure: !mValid, at /Users/noef/Documents/mozilla-central/dom/cache/Manager.cpp:1427
#01: mozilla::dom::cache::Context::Release() (mozalloc.h:205, in XUL)
#02: mozilla::dom::cache::Manager::Factory::ShutdownAllOnBackgroundThread() (nsRefPtr.h:60, in XUL)
#03: mozilla::dom::cache::Manager::Factory::ShutdownAllRunnable::Run() (Manager.cpp:356, in XUL)
#04: nsThread::ProcessNextEvent(bool, bool*) (nsCOMPtr.h:389, in XUL)
#05: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:265, in XUL)
--DOMWINDOW == 131 (0x12b34e800) [pid = 75414] [serial = 154] [outer = 0x111da8c00] [url = http://mochi.test:8888/tests/dom/workers/test/serviceworkers/message_receiver.html]
#06: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (MessagePump.cpp:368, in XUL)
#07: MessageLoop::Run() (message_loop.cc:517, in XUL)
[75414] WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file /Users/noef/Documents/mozilla-central/widget/cocoa/nsChildView.mm, line 5239
#08: nsThread::ThreadFunc(void*) (nsThread.cpp:350, in XUL)
#09: _pt_root (ptthread.c:215, in libnss3.dylib)
#10: _pthread_body (in libsystem_pthread.dylib) + 131
#11: _pthread_body (in libsystem_pthread.dylib) + 0
[NPAPI 75416] ###!!! ABORT: Aborting on channel error.: file /Users/noef/Documents/mozilla-central/ipc/glue/MessageChannel.cpp, line 1610
#01: mozilla::ipc::ProcessLink::OnChannelError() (Monitor.h:36, in XUL)
#02: IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int) (ipc_channel_posix.cc:861, in XUL)
#03: event_base_loop (event.c:1355, in XUL)
#04: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) (message_pump_libevent.cc:349, in XUL)
#05: MessageLoop::Run() (message_loop.cc:517, in XUL)
#06: base::Thread::ThreadMain() (thread.cc:173, in XUL)
#07: ThreadFunc(void*) (platform_thread_posix.cc:39, in XUL)
#08: _pthread_body (in libsystem_pthread.dylib) + 131
#09: _pthread_body (in libsystem_pthread.dylib) + 0
[NPAPI 75416] ###!!! ABORT: Aborting on channel error.: file /Users/noef/Documents/mozilla-central/ipc/glue/MessageChannel.cpp, line 1610
Hit MOZ_CRASH() at /Users/noef/Documents/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:33
TEST-INFO | Main app process: killed by out-of-range signal, number 117
TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_workerUpdate.html | application terminated with exit code -11
runtests.py | Application ran for: 0:00:34.333319
zombiecheck | Reading PID log: /var/folders/p6/q_3hc91j4ql73_b7c771mnh80000gp/T/tmp21uZ8vpidlog
Stopping web server
Stopping web socket server
Stopping ssltunnel
TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes
TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes
TEST-INFO | leakcheck | tab process: leak threshold set at 100000 bytes
TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes
 
TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks!
TEST-INFO | leakcheck | missing output line from log file /var/folders/p6/q_3hc91j4ql73_b7c771mnh80000gp/T/tmpLg3wM8.mozrunner/runtests_leaks.log
 
TEST-UNEXPECTED-FAIL | leakcheck | plugin process: missing output line for total leaks!
TEST-INFO | leakcheck | missing output line from log file /var/folders/p6/q_3hc91j4ql73_b7c771mnh80000gp/T/tmpLg3wM8.mozrunner/runtests_leaks_plugin_pid75416.log
runtests.py | Running tests: end.
SUITE-END | took 38s
(Reporter)

Updated

3 years ago
Version: 39 Branch → Trunk
Flags: needinfo?(ehsan)
This is DOM cache.
Flags: needinfo?(ehsan) → needinfo?(bkelly)
(Assignee)

Comment 2

3 years ago
I think the easy fix here is just to set mValid = false in Manager::Shutdown().  Its possible we could also lose the mShuttingDown flag as well, but I'm not sure.

I'll take this, but I won't have time to look at it until next week.  Ehsan, if you're so inclined, feel free to steal it from me.
Assignee: nobody → bkelly
Blocks: 1110144
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
No, it's ok, I have lots of other stuff to do!  Thanks :)
(Assignee)

Comment 4

3 years ago
Created attachment 8592379 [details] [diff] [review]
Refactor Cache Manager Context usage to be more sane and fix shutdown assert. r=ehsan

This should fix the crash.  While looking at how to do it cleanly I ended up doing a bit of refactory.

When QM OfflineStorage was implemented the Manager switched from creating Context objects on demand to only ever having at most one Context.  This lets us simplify things a bit by creating the Context immediately and making Factory::Remove() unconditionally occur when the context is destroyed.

I also improve Context cancellation handling here.  We should be aware whenever we drop an operation in Manager due to the Context being cancelled.  Mainly this means we leak an orphaned body or cache operation until maintenance can clean it up.
Attachment #8592379 - Flags: review?(ehsan)
(Assignee)

Comment 5

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c78145346e79
Comment on attachment 8592379 [details] [diff] [review]
Refactor Cache Manager Context usage to be more sane and fix shutdown assert. r=ehsan

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

::: dom/cache/Manager.cpp
@@ +1715,5 @@
> +  // Create the context immediately.  Since there can at most be one Context
> +  // per Manager now, this lets us cleanly call Factory::Remove() once the
> +  // Context goes away.
> +  nsRefPtr<Action> setupAction = new SetupAction();
> +  nsRefPtr<Context> ref = Context::Create(this, setupAction);

Why not assign to mContext directly?

@@ +1716,5 @@
> +  // per Manager now, this lets us cleanly call Factory::Remove() once the
> +  // Context goes away.
> +  nsRefPtr<Action> setupAction = new SetupAction();
> +  nsRefPtr<Context> ref = Context::Create(this, setupAction);
> +  mContext = ref;

This causes the manager object to be addref'ed before the constructor returns.  That in itself is harmless, but it does open the door to it being deleted as well, which is very bad.  In order to avoid that in the future, please move this part off to an Init() function to be called after constructing the object.
Attachment #8592379 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

3 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> Comment on attachment 8592379 [details] [diff] [review]
> Refactor Cache Manager Context usage to be more sane and fix shutdown
> assert. r=ehsan
> 
> Review of attachment 8592379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/Manager.cpp
> @@ +1715,5 @@
> > +  // Create the context immediately.  Since there can at most be one Context
> > +  // per Manager now, this lets us cleanly call Factory::Remove() once the
> > +  // Context goes away.
> > +  nsRefPtr<Action> setupAction = new SetupAction();
> > +  nsRefPtr<Context> ref = Context::Create(this, setupAction);
> 
> Why not assign to mContext directly?

Context::Create() returns already_AddRefed<> and mContext is a raw pointer.

I'll fix the other issue with an Init().
(Assignee)

Comment 8

3 years ago
If bug 1110485 is backed out, then this will need to be backed out again.  Pushing because people have been asking for this fix, though.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2629286ed4c
Depends on: 1110485
sorry had to back this out since bug 1110485 had to be backed out for bc1 memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=8868557&repo=mozilla-inbound
Flags: needinfo?(bkelly)
Blocks: 1153312
(Assignee)

Comment 10

3 years ago
I'll look, although dom/cache isn't executed in bc1 as far as I know.
Flags: needinfo?(bkelly)
(Assignee)

Comment 11

3 years ago
Created attachment 8593172 [details] [diff] [review]
Refactor Cache Manager Context usage to be more sane and fix shutdown assert. r=ehsan

Hopefully the last of the Great Rebase Nightmare of 2015.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6298c334e36
Attachment #8592379 - Attachment is obsolete: true
Attachment #8593172 - Flags: review+
Blocks: 1134329
(Assignee)

Comment 12

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dada69804f56
https://hg.mozilla.org/mozilla-central/rev/dada69804f56
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.