Closed Bug 1132436 Opened 5 years ago Closed 4 years ago

WorkerPrivate::RunBeforeNextEvent assertion fails sometimes (@NS_HasPendingEvents)


(Core :: DOM: Workers, defect)

Not set





(Reporter: baku, Assigned: khuey)




(2 files, 1 obsolete file)

Assertion failure: NS_HasPendingEvents(mThread), at /Volumes/mozilladev/m-c/dom/workers/WorkerPrivate.cpp:4556
#01: mozilla::dom::indexedDB::(anonymous namespace)::RunBeforeNextEvent(mozilla::dom::indexedDB::IDBTransaction*)[/Volumes/mozilladev/m-c/obj-firefox/dist/ +0x2389691]
#02: mozilla::dom::indexedDB::IDBTransaction::Create(mozilla::dom::indexedDB::IDBDatabase*, nsTArray<nsString> const&, mozilla::dom::indexedDB::IDBTransaction::Mode)[/Volumes/mozilladev/m-c/obj-firefox/dist/ +0x236a9a7]
#03: mozilla::dom::indexedDB::IDBDatabase::Transaction(mozilla::dom::Sequence<nsString> const&, mozilla::dom::IDBTransactionMode, mozilla::ErrorResult&)[/Volumes/mozilladev/m-c/obj-firefox/dist/ +0x236a48e]
#04: mozilla::dom::IDBDatabaseBinding::transaction(JSContext*, JS::Handle<JSObject*>, mozilla::dom::indexedDB::IDBDatabase*, JSJitMethodCallArgs const&)[/Volumes/mozilladev/m-c/obj-firefox/dist/ +0x181e05b]
This was on b2g, right?  Does it reproduce on desktop?
Hrm, this looks like a logic problem in RunBeforeNextEvent, not specific to IDB or SharedWorker.
Component: DOM: IndexedDB → DOM: Workers
Summary: IndexedDB in SharedWorkers crashes → WorkerPrivate::RunBeforeNextEvent assertion fails sometimes (@NS_HasPendingEvents)
Attached file test.tar.gz (obsolete) —
Here a test to reproduce the issue.
Probably we can simplify it a bit...
Actually the sequence can be reproduced with a Promise + IDB. A generic worker is fine as well.
Attached file test.tar.gz
A smaller test.
Attachment #8563461 - Attachment is obsolete: true
(In reply to Ben Kelly [:bkelly] from comment #1)
> This was on b2g, right?  Does it reproduce on desktop?

This is desktop.
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> Hrm, this looks like a logic problem in RunBeforeNextEvent, not specific to
> IDB or SharedWorker.

Debugging this issue I see that:

1. We call WorkerPrivate::RunBeforeNextEvent for IDB. At this point we don't have any preemptive runnable and the thread doesn't have pending events. recursionDepth is 1, and we store 0 in the PreemptingRunnableInfo.

2. We schedule OnProcessNextEvent. aRecursionDepth is 1 and we do not exec the IDB runnable.

3. A new call of RunBeforeNextEvent crashes here:
Assignee: nobody → amarchesini
Attached patch rdepth.patchSplinter Review
Attachment #8563694 - Flags: review?(bent.mozilla)
I don't think this is the correct fix.  I think that the fact that promises run with a recursion depth of 0 (both on the main thread and on workers) is wrong.
(In reply to Kyle Huey [:khuey] ( from comment #9)
> I think that the fact that promises run with a recursion depth of 0 is
> wrong.

I agree.

I'm working through some other big reviews and won't be able to look at this for a little bit. khuey, want to steal?
Well you can r- this as easily as I can ... :P
... but only if you give me another working approach!
Comment on attachment 8563694 [details] [diff] [review]

bent and I agree that this is the wrong approach.  The fundamental problem is that Promises run with a recursion depth of 0.  On the main thread this happens because we decrement nsThread::mRunningEvent before we call the main thread observer (which is XPConnect, and does promises (and mutation events) in AfterProcessNextEvent).  On the worker thread this happens because we manually run promises outside of NS_ProcessNextEvent.

Fixing this likely requires some refactoring of nsThread, unfortunately.
Attachment #8563694 - Flags: review?(bent.mozilla) → review-
Assignee: amarchesini → khuey
any update?
Flags: needinfo?(khuey)
Blocks: 1153503
No longer blocks: 1153503
Bug 1179909 changes the promise recursion depth stuff, worth seeing if it fixes this too.
Flags: needinfo?(khuey)
Kyle, apparently, patch from bug 1179909 fixes this crash. At least in my case. I haven't tested with attachment 8563532 [details].
Depends on: 1179909
Has anyone figured a work around until this gets fixed?
Blocks: graphene
Don't use IndexedDB in a Promise?
I think the approach :paul already took of landing the patch on a branch in bug 1185187 is the best solution.  (Or, if building gecko is not required, just pegging the used b2g binary to one with the patch.)
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1179909
You need to log in before you can comment on or make changes to this bug.