Closed Bug 1132436 Opened 5 years ago Closed 4 years ago
Private::Run Before Next Event assertion fails sometimes (@NS _Has Pending Events)
http://pastebin.mozilla.org/8694926 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/NightlyDebug.app/Contents/MacOS/XUL +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/NightlyDebug.app/Contents/MacOS/XUL +0x236a9a7] #03: mozilla::dom::indexedDB::IDBDatabase::Transaction(mozilla::dom::Sequence<nsString> const&, mozilla::dom::IDBTransactionMode, mozilla::ErrorResult&)[/Volumes/mozilladev/m-c/obj-firefox/dist/NightlyDebug.app/Contents/MacOS/XUL +0x236a48e] #04: mozilla::dom::IDBDatabaseBinding::transaction(JSContext*, JS::Handle<JSObject*>, mozilla::dom::indexedDB::IDBDatabase*, JSJitMethodCallArgs const&)[/Volumes/mozilladev/m-c/obj-firefox/dist/NightlyDebug.app/Contents/MacOS/XUL +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)
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.
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: MOZ_ASSERT_IF(!mPreemptingRunnableInfos.IsEmpty(), NS_HasPendingEvents(mThread));
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] (firstname.lastname@example.org) 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] rdepth.patch 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-
5 years ago
Assignee: amarchesini → khuey
Bug 1161690 maybe too.
Duplicate of this bug: 1141539
Bug 1179909 changes the promise recursion depth stuff, worth seeing if it fixes this too.
Kyle, apparently, patch from bug 1179909 fixes this crash. At least in my case. I haven't tested with attachment 8563532 [details].
Thanks for testing.
Has anyone figured a work around until this gets fixed?
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.)
4 years ago
Status: ASSIGNED → RESOLVED
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.