Assertion failure in DispatchSuccessEvent when using the right scheduling for Promises

RESOLVED DUPLICATE of bug 1193394

Status

()

Core
DOM: IndexedDB
P2
normal
RESOLVED DUPLICATE of bug 1193394
a year ago
5 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

50 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

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

Comment 2

a year ago
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
(Assignee)

Comment 3

a year ago
Created attachment 8822098 [details] [diff] [review]
12_idb_crash.diff
Assignee: nobody → bugs
Attachment #8822098 - Flags: review?(jvarga)
(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
(Assignee)

Comment 5

a year ago
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
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Comment 7

a year ago
Except that our metastablestate handling is super messy because Promises' microtask handling is all broken... /me wonders how to fix this
(Assignee)

Comment 8

a year ago
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.
Blocks: 1179909
(Assignee)

Updated

a year ago
Attachment #8822098 - Flags: review?(jvarga)

Comment 9

a year ago
(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.
(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.
(Assignee)

Comment 11

a year ago
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.
Attachment #8822098 - Flags: review?(btseng)
(Assignee)

Comment 12

a year ago
Created attachment 8822377 [details] [diff] [review]
alternative

This is the alternative approach to run metastablestate at the end of task
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.
Attachment #8822098 - Flags: review?(btseng) → review+
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
(Assignee)

Comment 15

a year ago
Created attachment 8822417 [details] [diff] [review]
12_idb_crash.diff
(Assignee)

Comment 16

a year ago
ah, possibly could merge

Updated

8 months ago
Priority: -- → P2
The fix has been included in patch part 1 of bug 1193394.
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1193394
You need to log in before you can comment on or make changes to this bug.