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.
Created attachment 158873 [details] Patch
tony@ or ojan@ - r?
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.
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 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 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?
(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.)
Created attachment 158902 [details] Patch
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 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.
(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.
Created attachment 158949 [details] Patch
Added Worker tests. tony@ or abarth@ - r?
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 on attachment 158949 [details] Patch IDB stuff LGTM, for what it's worth
Created attachment 159745 [details] Patch
Needed a rebase. dgrogan@ can you take another look, mostly at the IDBRequest::dispatchEvent method?
Comment on attachment 159745 [details] Patch LGTM
Comment on attachment 159745 [details] Patch Attachment 159745 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13545695
Created attachment 159764 [details] Patch for landing
Comment on attachment 159764 [details] Patch for landing Clearing flags on attachment: 159764 Committed r126254: <http://trac.webkit.org/changeset/126254>
All reviewed patches have been landed. Closing bug.