Bug 86911 – IndexedDB: "ASSERTION FAILED: !m_requestFinished" hit in IDBRequest::dispatchEvent

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

Created attachment 142799 [details]
Repro case

Seen in DumpRenderTree with attached LayoutTest (which could be dropped into storage/indexeddb for dependencies):

crash log for DumpRenderTree (pid 8867):
STDERR: ASSERTION FAILED: !m_requestFinished
STDERR: ../../third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBRequest.cpp(342) : virtual bool WebCore::IDBRequest::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)

What's probably happening is the transaction is completing, and IDBTransaction::onComplete() calls IDBTransaction::closeOpenCursors() calls IDBCursor::close() calls IDBRequest::finishCursor() which sets m_cursorFinished, which allows m_requestFinished to be set. This may be happening too early.

[Joshua Bell]

Created attachment 142804 [details]
Real repro case

[Joshua Bell]

Comment on attachment 142799 [details]
Repro case

Whoops. First repro had been reduced too far and I was testing the wrong file. Look at the second one that has noSuchFunction

[Joshua Bell]

(In reply to comment #2)
> (From update of attachment 142799 [details])
> Whoops. First repro had been reduced too far and I was testing the wrong file. Look at the second one that has noSuchFunction

... and I admit I now have no idea what's going on. Changing the ordering of lines in the minimal repro - e.g. when the non-existent handler is assigned - makes the issue go away, which means my previous theory is incorrect.

[Joshua Bell]

Ah, here's the key bit:

    request = store.openCursor();
    request.onsuccess = function () {
        event.target.result.continue();
        throw "foo";
    };

If an exception is thrown in the event handler after the continue() is called the ASSERT is hit.

[Joshua Bell]

The uncaught exception is causing the transaction to be aborted, which closes the cursor which sets m_requestFinished on the IDBRequest between an event being enqueued and it being dispatched.

[Joshua Bell]

(In reply to comment #5)
> The uncaught exception is causing the transaction to be aborted, which closes the cursor which sets m_requestFinished on the IDBRequest between an event being enqueued and it being dispatched.

Scratch that, not quite correct. Here's what's going on:

* success event fired at the cursor request
* success event dispatched by the cursor request
* script calls continue(), which queues an task in the transaction
* script raises an exception, which aborts the transaction
* transaction aborting closes the cursor
* transaction aborting aborts the queued task, which fires an error event at the cursor request
* cursor request attempt to dispatch the error event, and ASSERTs

[Joshua Bell]

Seems like we need to untangle things slightly - either the cursor shouldn't "finish" the request while there are outstanding events, or we should allow the error event to be dispatched even if the cursor is closed. Given that dispatch can be arbitrarily delayed, it seems like the latter is more tractable.

(The timing difference between event queuing and dispatching is tricksy.)

[Joshua Bell]

Note that this still repros after the untangling in 90412 (which makes sense because that patch should only impact multiprocess ports, whereas this repros DRT), but any fix here should wait until that lands since it touches related code.

[Joshua Bell]

Created attachment 153361 [details]
Patch

[Joshua Bell]

Counting only "interesting" LOC, IDBRequest.cpp is now 20% ASSERTs!

dgrogan@, alecflett@ - please take a look.

[David Grogan]

Comment on attachment 153361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153361&action=review

LGTM

> Source/WebCore/Modules/indexeddb/IDBRequest.h:76
>      bool resetReadyState(IDBTransaction*);

Can this line be deleted?

[Joshua Bell]

Created attachment 153605 [details]
Patch

[Joshua Bell]

(In reply to comment #11)
> > Source/WebCore/Modules/indexeddb/IDBRequest.h:76
> >      bool resetReadyState(IDBTransaction*);
> 
> Can this line be deleted?

Good catch - deleted!

tony@ - r?

[WebKit Review Bot]

Comment on attachment 153605 [details]
Patch

Clearing flags on attachment: 153605

Committed r123275: <http://trac.webkit.org/changeset/123275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.