Bug 94235 – IndexedDB: IDBRequest can be GCd during event dispatch

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

This may be V8/Chromium and possibly Worker specific.

Repro is at: http://crbug.com/142419

Scenario:

* Build up a lot of garbage in script
* Iterate a cursor using syntax like this:

store.openCursor(lo, hi).onsuccess = function(e) {
  var cursor = e.target.result;
  if (cursor) {
    cursor.continue();
  }
};

* Since there is no reference to the IDBRequest returned by openCursor() it can be reclaimed. 

There is logic in IDBRequest::hasPendingActivity() to prevent this, which tests:

  m_readyState·==·PENDING·||·ActiveDOMObject::hasPendingActivity()

But m_readyState is set to DONE at the start of IDBRequest::dispatchEvent(), so when the dispatch actually occurs and we dive into script, GC can kick in before the listener function is called and request can be GC'd on the script side of things.

A trivial fix is to add m_isDispatching to IDBRequest and set/unset it around the actual IDBEventDispatcher::dispatch() call.

This might be a (new?) Worker/V8 bug, though - it seems like allowing GC to kick in at this point is a little sketchy.

[Joshua Bell]

Created attachment 158873 [details]
Patch

[Joshua Bell]

tony@ or ojan@ - r?

[Ojan Vafai]

This seems reasonable to me. I'm not too familiar with how we generally handle GC issues though. CC'ing some people more knowledgeable about the right ways to handle keeping objects alive during event dispatch.

[Joshua Bell]

For IDBRequest, this would have been broken by:

http://trac.webkit.org/changeset/123275

The m_requestFinished flag (which was removed) was tested in hasPendingActivity() and was set after the dispatch occurred.

[Joshua Bell]

Comment on attachment 158873 [details]
Patch

Removing r? - on further thinking, some of the previous simplifications went too far. Namely, since an IDBCursor can be asked to continue at an arbitrary time the IDBRequest it came from must be kept alive. This means http://trac.webkit.org/changeset/124842 should really be reverted.

[Adam Barth]

Comment on attachment 158873 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:468
> +    m_dispatching = true;
>      bool dontPreventDefault = IDBEventDispatcher::dispatch(event.get(), targets);
> +    m_dispatching = false;

Can this be re-entered?

[Joshua Bell]

(In reply to comment #6)
> (From update of attachment 158873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158873&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:468
> > +    m_dispatching = true;
> >      bool dontPreventDefault = IDBEventDispatcher::dispatch(event.get(), targets);
> > +    m_dispatching = false;
> 
> Can this be re-entered?

No - script actions can cause new events to be enqueued, but dispatch should occur asynchronously.

(Upcoming patch removes that anyway, though.)

[Joshua Bell]

Created attachment 158902 [details]
Patch

[Joshua Bell]

As mentioned in the comments and changelog, after further thought this was reintroduced by previous refactors that went too far and missed this case.

The patch is now a partial reversion of:

http://trac.webkit.org/changeset/121492 - IDBTransaction state simplification; effectively reverts the m_transactionFinished changes, but changes the variable to m_hasPendingActivity to clarify the use


http://trac.webkit.org/changeset/123275 - IDBRequest state simplification; effectively reverts the m_requestFinished and m_cursorFinished changes, but (again) changes the variable to m_hasPendingActivity to clarify the use

http://trac.webkit.org/changeset/124842 - simplification following the above; turns out this is necessary, so wholesale reverted.

[David Grogan]

Comment on attachment 158902 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:480
> +    if (m_readyState == DONE && (!cursorToNotify || m_cursorFinished))

I would have expected garbage collection to run on this same thread, in between dispatching events. But if I understand this problem, gc runs in some other thread, querying IDBRequest::hasPendingActivity() when the script execution thread is between line 432/440 and 464/472.  Accurate?

> LayoutTests/storage/indexeddb/resources/pending-activity.js:1
> +if (this.importScripts) {

Somewhere I saw that this might only be a problem when run from a worker. Do you want to run this script from a worker in pending-activity-workers.html?

> LayoutTests/storage/indexeddb/resources/pending-activity.js:6
> +description("Checks that garbage collection doesn't relaim objects with pending activity");

reclaim

> LayoutTests/storage/indexeddb/resources/pending-activity.js:55
> +    debug("");

You can use preamble() in place of these two debug lines.  For standalone functions at least.

[Joshua Bell]

(In reply to comment #10)
> (From update of attachment 158902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158902&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:480
> > +    if (m_readyState == DONE && (!cursorToNotify || m_cursorFinished))
> 
> I would have expected garbage collection to run on this same thread, in between dispatching events. But if I understand this problem, gc runs in some other thread, querying IDBRequest::hasPendingActivity() when the script execution thread is between line 432/440 and 464/472.  Accurate?

Nope, GC does run in the same thread.

The GC is kicking in when the event is asked to fire - basically, as soon as we drop into script execution, GC happens and claims this IDBRequest, then the logic to fire the event proceeds but the object is gone so it's a no-op.
 
> > LayoutTests/storage/indexeddb/resources/pending-activity.js:1
> > +if (this.importScripts) {
> 
> Somewhere I saw that this might only be a problem when run from a worker. Do you want to run this script from a worker in pending-activity-workers.html?

Good idea, With the Chromium repro I couldn't get it to happen outside of Workers.

[Joshua Bell]

Created attachment 158949 [details]
Patch

[Joshua Bell]

Added Worker tests.

tony@ or abarth@ - r?

[Adam Barth]

This is a good way of integrating with the GC, but I don't know much about the IDB specific parts of this patch.  Perhaps Tony is the better reviewer here.

[David Grogan]

Comment on attachment 158949 [details]
Patch

IDB stuff LGTM, for what it's worth

[Joshua Bell]

Created attachment 159745 [details]
Patch

[Joshua Bell]

Needed a rebase. dgrogan@ can you take another look, mostly at the IDBRequest::dispatchEvent method?

[David Grogan]

Comment on attachment 159745 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 159745 [details]
Patch

Attachment 159745 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13545695

[Joshua Bell]

Created attachment 159764 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 159764 [details]
Patch for landing

Clearing flags on attachment: 159764

Committed r126254: <http://trac.webkit.org/changeset/126254>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.