Bug 94235 - IndexedDB: IDBRequest can be GCd during event dispatch
Summary: IndexedDB: IDBRequest can be GCd during event dispatch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-16 11:42 PDT by Joshua Bell
Modified: 2012-08-21 19:43 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.37 KB, patch)
2012-08-16 12:20 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2012-08-16 14:18 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (18.04 KB, patch)
2012-08-16 17:01 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (17.87 KB, patch)
2012-08-21 13:05 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (17.66 KB, patch)
2012-08-21 14:03 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-08-16 11:42:39 PDT
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.
Comment 1 Joshua Bell 2012-08-16 12:20:08 PDT
Created attachment 158873 [details]
Patch
Comment 2 Joshua Bell 2012-08-16 13:10:04 PDT
tony@ or ojan@ - r?
Comment 3 Ojan Vafai 2012-08-16 13:13:33 PDT
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.
Comment 4 Joshua Bell 2012-08-16 13:17:35 PDT
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.
Comment 5 Joshua Bell 2012-08-16 13:28:12 PDT
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.
Comment 6 Adam Barth 2012-08-16 13:31:11 PDT
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?
Comment 7 Joshua Bell 2012-08-16 13:33:02 PDT
(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.)
Comment 8 Joshua Bell 2012-08-16 14:18:49 PDT
Created attachment 158902 [details]
Patch
Comment 9 Joshua Bell 2012-08-16 14:27:01 PDT
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.
Comment 10 David Grogan 2012-08-16 16:20:58 PDT
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.
Comment 11 Joshua Bell 2012-08-16 16:28:50 PDT
(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.
Comment 12 Joshua Bell 2012-08-16 17:01:15 PDT
Created attachment 158949 [details]
Patch
Comment 13 Joshua Bell 2012-08-21 10:58:35 PDT
Added Worker tests.

tony@ or abarth@ - r?
Comment 14 Adam Barth 2012-08-21 11:07:51 PDT
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.
Comment 15 David Grogan 2012-08-21 12:29:09 PDT
Comment on attachment 158949 [details]
Patch

IDB stuff LGTM, for what it's worth
Comment 16 Joshua Bell 2012-08-21 13:05:01 PDT
Created attachment 159745 [details]
Patch
Comment 17 Joshua Bell 2012-08-21 13:05:55 PDT
Needed a rebase. dgrogan@ can you take another look, mostly at the IDBRequest::dispatchEvent method?
Comment 18 David Grogan 2012-08-21 13:10:43 PDT
Comment on attachment 159745 [details]
Patch

LGTM
Comment 19 WebKit Review Bot 2012-08-21 13:37:41 PDT
Comment on attachment 159745 [details]
Patch

Attachment 159745 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13545695
Comment 20 Joshua Bell 2012-08-21 14:03:11 PDT
Created attachment 159764 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-08-21 19:43:25 PDT
Comment on attachment 159764 [details]
Patch for landing

Clearing flags on attachment: 159764

Committed r126254: <http://trac.webkit.org/changeset/126254>
Comment 22 WebKit Review Bot 2012-08-21 19:43:29 PDT
All reviewed patches have been landed.  Closing bug.