1325975 - Assertion failure in DispatchSuccessEvent when using the right scheduling for Promises

Status: RESOLVED DUPLICATE of bug 1193394

(Core :: Storage: IndexedDB, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

testing/web-platform/tests/IndexedDB/idbindex-rename-abort.html


#0  0x00007fffe54a2318 in mozilla::dom::indexedDB::(anonymous namespace)::DispatchSuccessEvent(mozilla::dom::indexedDB::(anonymous namespace)::ResultHelper*, nsIDOMEvent*) (aResultHelper=0x7fffffffb168, aEvent=0x7fffac91b350) at /home/smaug/mozilla/hg/inbound2/dom/indexedDB/ActorsChild.cpp:880
#1  0x00007fffe54a51ad in mozilla::dom::indexedDB::BackgroundDatabaseChild::RecvPBackgroundIDBVersionChangeTransactionConstructor(mozilla::dom::indexedDB::PBackgroundIDBVersionChangeTransactionChild*, unsigned long const&, unsigned long const&, long const&, long const&) (this=0x7fffad7a7840, aActor=0x7fffb8103308, aCurrentVersion=@0x7fffffffb3c8: 0, aRequestedVersion=@0x7fffffffb3c0: 1, aNextObjectStoreId=@0x7fffffffb3b8: 1, aNextIndexId=@0x7fffffffb3b0: 1) at /home/smaug/mozilla/hg/inbound2/dom/indexedDB/ActorsChild.cpp:1902
#2  0x00007fffe2557caf in mozilla::dom::indexedDB::PBackgroundIDBDatabaseChild::OnMessageReceived(IPC::Message const&) (this=0x7fffad7a7840, msg__=...)
    at /home/smaug/mozilla/hg/inbound2/ff_build/ipc/ipdl/PBackgroundIDBDatabaseChild.cpp:599
#3  0x00007fffe22b57e4 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) (this=0x7fffba9ab000, msg__=...) at /home/smaug/mozilla/hg/inbound2/ff_build/ipc/ipdl/PBackgroundChild.cpp:1467
#4  0x00007fffe2201be6 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (this=0x7fffba9ab0f8, aMsg=...) at /home/smaug/mozilla/hg/inbound2/ipc/glue/MessageChannel.cpp:1750
#5  0x00007fffe2200663 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=0x7fffba9ab0f8, aMsg=<unknown type in /home/smaug/mozilla/hg/inbound2/ff_build/dist/bin/libxul.so, CU 0x1609c21, DIE 0x165f240>) at /home/smaug/mozilla/hg/inbound2/ipc/glue/MessageChannel.cpp:1688
#6  0x00007fffe2201263 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (this=0x7fffba9ab0f8, aTask=...)
    at /home/smaug/mozilla/hg/inbound2/ipc/glue/MessageChannel.cpp:1572
#7  0x00007fffe22016d8 in mozilla::ipc::MessageChannel::MessageTask::Run() (this=0x7fffad545580) at /home/smaug/mozilla/hg/inbound2/ipc/glue/MessageChannel.cpp:1597
#8  0x00007fffe18f780d in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fffd8a275c0, aMayWait=false, aResult=0x7fffffffbc7e) at /home/smaug/mozilla/hg/inbound2/xpcom/threads/nsThread.cpp:1213
#9  0x00007fffe1979961 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fffd8a275c0, aMayWait=false) at /home/smaug/mozilla/hg/inbound2/xpcom/glue/nsThreadUtils.cpp:381
#10 0x00007fffe2204689 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7fffd7abaac0, aDelegate=0x7ffff6ba6260) at /home/smaug/mozilla/hg/inbound2/ipc/glue/MessagePump.cpp:96
#11 0x00007fffe21625d5 in MessageLoop::RunInternal() (this=0x7ffff6ba6260) at /home/smaug/mozilla/hg/inbound2/ipc/chromium/src/base/message_loop.cc:232
#12 0x00007fffe2162555 in MessageLoop::RunHandler() (this=0x7ffff6ba6260) at /home/smaug/mozilla/hg/inbound2/ipc/chromium/src/base/message_loop.cc:225
#13 0x00007fffe216252d in MessageLoop::Run() (this=0x7ffff6ba6260) at /home/smaug/mozilla/hg/inbound2/ipc/chromium/src/base/message_loop.cc:205
#14 0x00007fffe597f853 in nsBaseAppShell::Run() (this=0x7fffcc23e390) at /home/smaug/mozilla/hg/inbound2/widget/nsBaseAppShell.cpp:156
#15 0x00007fffe6b6d2e2 in nsAppStartup::Run() (this=0x7fffcc2421a0) at /home/smaug/mozilla/hg/inbound2/toolkit/components/startup/nsAppStartup.cpp:283
#16 0x00007fffe6c6acb1 in XREMain::XRE_mainRun() (this=0x7fffffffc698) at /home/smaug/mozilla/hg/inbound2/toolkit/xre/nsAppRunner.cpp:4494
#17 0x00007fffe6c6b765 in XREMain::XRE_main(int, char**, mozilla::XREAppData const&) (this=0x7fffffffc698, argc=5, argv=0x7fffffffdbc8, aAppData=...)
    at /home/smaug/mozilla/hg/inbound2/toolkit/xre/nsAppRunner.cpp:4623
#18 0x00007fffe6c6bf3f in XRE_main(int, char**, mozilla::XREAppData const&, uint32_t) (argc=5, argv=0x7fffffffdbc8, aAppData=..., aFlags=0) at /home/smaug/mozilla/hg/inbound2/toolkit/xre/nsAppRunner.cpp:4714
#19 0x0000000000405f24 in do_main(int, char**, char**, nsIFile*) (argc=5, argv=0x7fffffffdbc8, envp=0x7fffffffdbf8, xreDirectory=0x7ffff6b59a80)
    at /home/smaug/mozilla/hg/inbound2/browser/app/nsBrowserApp.cpp:319
#20 0x0000000000405550 in main(int, char**, char**) (argc=5, argv=0x7fffffffdbc8, envp=0x7fffffffdbf8) at /home/smaug/mozilla/hg/inbound2/browser/app/nsBrowserApp.cpp:452
(gdb) up
#1  0x00007fffe54a51ad in mozilla::dom::indexedDB::BackgroundDatabaseChild::RecvPBackgroundIDBVersionChangeTransactionConstructor (this=0x7fffad7a7840, aActor=0x7fffb8103308, 
    aCurrentVersion=@0x7fffffffb3c8: 0, aRequestedVersion=@0x7fffffffb3c0: 1, aNextObjectStoreId=@0x7fffffffb3b8: 1, aNextIndexId=@0x7fffffffb3b0: 1)
    at /home/smaug/mozilla/hg/inbound2/dom/indexedDB/ActorsChild.cpp:1902
1902	  DispatchSuccessEvent(&helper, upgradeNeededEvent);
(gdb) down
#0  0x00007fffe54a2318 in mozilla::dom::indexedDB::(anonymous namespace)::DispatchSuccessEvent (aResultHelper=0x7fffffffb168, aEvent=0x7fffac91b350)
    at /home/smaug/mozilla/hg/inbound2/dom/indexedDB/ActorsChild.cpp:880

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

(gdb) p transaction
$1 = {mRawPtr = 0x7fffad544cc0}
(gdb) p transaction->IsOpen()
$2 = false
(gdb) p transaction->IsAborted()
$3 = false
(gdb) p transaction->IsDone()
$4 = true

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I may have an idea about this.
DispatchEvent() call before the assertion may do anything, so nothing really guarantees the state of ...well anything.
Perhaps the assertion could be just moved before the DispatchEvent call

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 12_idb_crash.diff

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Olli Pettay [:smaug] from comment #1)
> (gdb) p transaction
> $1 = {mRawPtr = 0x7fffad544cc0}
> (gdb) p transaction->IsOpen()
> $2 = false
> (gdb) p transaction->IsAborted()
> $3 = false
> (gdb) p transaction->IsDone()
> $4 = true

The assertion before modification looks reasonable to me:
We add an runnable in metastable state [1] when IDBVersionChangeTransaction is constructed and expects this runnable to be run to set the mReadyState to DONE [2] after the current runnable triggered from mozilla::ipc::MessageChannel::DispatchMessage() is done if there is no further IDBRequest added from the user.

Is there some change in bug 1193394 breaking this rule? and is that expected?

[1] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/dom/indexedDB/IDBTransaction.cpp#193-194
[2] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/dom/indexedDB/IDBTransaction.cpp#1017-1020

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

The code dispatches an DOM event. In the current world promise callbacks are run at the end of the current task. In the new world promise callbacks are called at the end of the microtask, so, at the end of the outermost script execution, like an event listener.

But even currently, if one triggers spinning event loop, like sync XHR, we had this same issue, as far as I see.

bug 1193394 doesn't change meta stable state handling as such, but since microtask handling gets fixed
(currently the thing which is called Microtask in CycleCollectedJSContext has nothing to do with microtask),
and meta stable state handling keeps following microtasks [1] the timing is changed as a side effect.


[1] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/xpcom/base/CycleCollectedJSContext.cpp#1407-1414

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Hmm, if spec's "When transaction is finished, immediately set request’s transaction to null. This must be done in the same task as the task firing the complete or abort event, but after the event has been fired." is what our metastablestate is about, then I need to decouple microtasks from metastablestate.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Except that our metastablestate handling is super messy because Promises' microtask handling is all broken... /me wonders how to fix this

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I guess I'll need to basically backout part of bug 1179909. It was just wrong, since it sort of relied on the bogus microtask handling we have for Promises.

Comment 9

[Jan Varga [:janv]]

(In reply to Olli Pettay [:smaug] from comment #8)
> I guess I'll need to basically backout part of bug 1179909. It was just
> wrong, since it sort of relied on the bogus microtask handling we have for
> Promises.

Ok.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Hmm, if spec's "When transaction is finished, immediately set request’s
> transaction to null. This must be done in the same task as the task firing
> the complete or abort event, but after the event has been fired." is what
> our metastablestate is about, then I need to decouple microtasks from
> metastablestate.
No, in short, this is not related to the metastable state.

The use case of metastable is more about implementing the auto-commiting behavior *especially* for the case when a new IDBTransaction is instantiated *without any IDBRequest added* at the end of the event callback.
http://w3c.github.io/IndexedDB/#transaction-commit

For other use cases after IDBRequest is done are all covered by the call sites of IDBTransaction::OnRequestFinished() to clean up the completed IDBRequest and to auto-commit the txn if the number of pending IDBRequest is decreased to zero.

The spec's "When transaction is finished, immediately set request’s transaction to null. This must be done in the same task as the task firing the complete or abort event, but after the event has been fired." is actually done at: BackgroundVersionChangeTransactionChild::RecvComplete() once IDB operations in parent process were committed which sets request’s transaction to null after the complete/abort event is fired: 
http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/dom/indexedDB/ActorsChild.cpp#2400-2402

Hence, we could focus on the use case of the IDBTransaction with *empty* IDBRequest that relies on metastable state:
1. The no IDBRequest added during 'onupgradeneeded' event callback.
2. In any IDB callback, user requests a new IDBTransaction from IDBDatabase::transaction() without further IDBRequests added.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822098 [details] [diff] [review]
12_idb_crash.diff

Based on code inspection chromium has similar-ish setup we have now, they rely on runEndOfScopeTasks(); which is called by microtasksCompletedCallback.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: alternative

This is the alternative approach to run metastablestate at the end of task

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8822098 [details] [diff] [review]
12_idb_crash.diff

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

r=me after the assertion condition is changed properly.

::: dom/indexedDB/ActorsChild.cpp
@@ +871,5 @@
>                   IDB_LOG_STRINGIFY(aEvent, kSuccessEventType));
>    }
>  
> +  MOZ_ASSERT_IF(transaction,
> +                transaction->IsOpen() || transaction->IsAborted());

MOZ_ASSERT_IF(transaction,
                transaction->IsOpen() && !transaction->IsAborted());

We are going to dispatch a success event.
If transaction is available, it shall be open and shall not be aborted yet.
It could be aborted in the event callback.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8822098 [details] [diff] [review]
12_idb_crash.diff

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

::: dom/indexedDB/ActorsChild.cpp
@@ +871,5 @@
>                   IDB_LOG_STRINGIFY(aEvent, kSuccessEventType));
>    }
>  
> +  MOZ_ASSERT_IF(transaction,
> +                transaction->IsOpen() || transaction->IsAborted());

I think we could just merge this assertion to this line:
http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/dom/indexedDB/ActorsChild.cpp#856

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 12_idb_crash.diff

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

ah, possibly could merge

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

The fix has been included in patch part 1 of bug 1193394.