1132436 - WorkerPrivate::RunBeforeNextEvent assertion fails sometimes (@NS_HasPendingEvents)

Status: RESOLVED DUPLICATE of bug 1179909

(Core :: DOM: Workers, defect)

Description

[Andrea Marchesini [:baku]]

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]

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This was on b2g, right?  Does it reproduce on desktop?

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hrm, this looks like a logic problem in RunBeforeNextEvent, not specific to IDB or SharedWorker.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: test.tar.gz

Here a test to reproduce the issue.
Probably we can simplify it a bit...

Comment 4

[Andrea Marchesini [:baku]]

Actually the sequence can be reproduced with a Promise + IDB. A generic worker is fine as well.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: test.tar.gz

A smaller test.

Comment 6

[Andrea Marchesini [:baku]]

(In reply to Ben Kelly [:bkelly] from comment #1)
> This was on b2g, right?  Does it reproduce on desktop?

This is desktop.

Comment 7

[Andrea Marchesini [:baku]]

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

Comment 8

[Andrea Marchesini [:baku]]

Attachment: rdepth.patch

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well you can r- this as easily as I can ... :P

Comment 12

[Andrea Marchesini [:baku]]

... but only if you give me another working approach!

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 14

[Doug Turner (:dougt)]

any update?

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bug 1161690 maybe too.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Bug 1179909 changes the promise recursion depth stuff, worth seeing if it fixes this too.

Comment 18

[Paul Rouget [:paul]]

Kyle, apparently, patch from bug 1179909 fixes this crash. At least in my case. I haven't tested with attachment 8563532 [details].

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks for testing.

Comment 20

[Paul Rouget [:paul]]

Has anyone figured a work around until this gets fixed?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Don't use IndexedDB in a Promise?

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

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.)