[V8] IndexedDB: IDBTransaction objects leak in Workers
Created attachment 128341 [details] Patch
http://crbug.com/115341 has repro steps I have no idea if the patch is complete or sensible, but it appears to address the issue.
Comment on attachment 128341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128341&action=review The patch looks good, but will be nice to have tests > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Can we have a layout test for this?
Created attachment 128548 [details] Patch
(In reply to comment #3) > Can we have a layout test for this? Of course! I identified a few more points where Workers call into script (Events, Errors) so those are handled and there are tests. (Having to trap the Worker's error event in the page is unfortunate.)
dslomov, are there other reviewers I should have take a look at this?
(In reply to comment #6) > dslomov, are there other reviewers I should have take a look at this? (I am not a reviewer) levin@ and dimich@ would be other people, but I know they are both on vacation. I'll add abarth@ too. maybe he can review, suggest someone else or trust my judgement.
This seems related to https://bugs.webkit.org/show_bug.cgi?id=72063 +rafaelw: thoughts on this patch?
This generally seems reasonable to me, but we should check with rafaelw.
Comment on attachment 128548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128548&action=review LGTM with nit and other ports' expectations > LayoutTests/fast/js/resources/js-test-pre.js:469 > + }; nit: Is this required for this fix? We try to avoid unnecessary style changes bundled with patches. Ditto other places. > LayoutTests/storage/indexeddb/transaction-abort-workers-expected.txt:1 > +[Worker] Test IndexedDB workers, recursion, and transaction termination. Do JSC ports work fine on this test?
(In reply to comment #10) > (From update of attachment 128548 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128548&action=review > > LGTM with nit and other ports' expectations > > > LayoutTests/fast/js/resources/js-test-pre.js:469 > > + }; > > nit: Is this required for this fix? We try to avoid unnecessary style changes bundled with patches. Ditto other places. Not required; I'll pull it out. > > > LayoutTests/storage/indexeddb/transaction-abort-workers-expected.txt:1 > > +[Worker] Test IndexedDB workers, recursion, and transaction termination. > > Do JSC ports work fine on this test? Only the Chromium/V8 currently enables IndexedDB; see https://bugs.webkit.org/show_bug.cgi?id=45110
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 128548 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=128548&action=review > > > > LGTM with nit and other ports' expectations > > > > > LayoutTests/fast/js/resources/js-test-pre.js:469 > > > + }; > > > > nit: Is this required for this fix? We try to avoid unnecessary style changes bundled with patches. Ditto other places. > > Not required; I'll pull it out. > > > > > > LayoutTests/storage/indexeddb/transaction-abort-workers-expected.txt:1 > > > +[Worker] Test IndexedDB workers, recursion, and transaction termination. > > > > Do JSC ports work fine on this test? > > Only the Chromium/V8 currently enables IndexedDB; see https://bugs.webkit.org/show_bug.cgi?id=45110 Right so this test will fail on those ports wouldn't it? You should add it to the appropriate platform/*/Skipped
(In reply to comment #12)> > Right so this test will fail on those ports wouldn't it? You should add it to the appropriate platform/*/Skipped All of storage/indexeddb is skipped on all of the non-Chromium platforms.
(In reply to comment #13) > (In reply to comment #12)> > > Right so this test will fail on those ports wouldn't it? You should add it to the appropriate platform/*/Skipped > > All of storage/indexeddb is skipped on all of the non-Chromium platforms. Got it - thanks!
Created attachment 128558 [details] Patch
Comment on attachment 128558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128558&action=review LGTM, FWIW > LayoutTests/fast/js/resources/js-test-pre.js:463 > + return worker; Thanks for adding this, I had to add it locally for something else. > LayoutTests/storage/indexeddb/resources/transaction-abort-workers.js:37 > + evalAndLog("store = transaction.objectStore('store')"); Before this patch, none of these event handlers would be called? > LayoutTests/storage/indexeddb/resources/transaction-abort-workers.js:50 > +function recursionTest() This still worked before this patch, right? > LayoutTests/storage/indexeddb/resources/transaction-abort-workers.js:86 > +function timeoutTest() This tests the change to V8WorkerContextEventListener.cpp ?
(In reply to comment #16) > (From update of attachment 128558 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128558&action=review > > LGTM, FWIW > > > LayoutTests/fast/js/resources/js-test-pre.js:463 > > + return worker; > > Thanks for adding this, I had to add it locally for something else. > > > LayoutTests/storage/indexeddb/resources/transaction-abort-workers.js:37 > > + evalAndLog("store = transaction.objectStore('store')"); > > Before this patch, none of these event handlers would be called? Yes. Because the transaction didn't start, it would remain in the queue since an abort was never fired. > > LayoutTests/storage/indexeddb/resources/transaction-abort-workers.js:50 > > +function recursionTest() > > This still worked before this patch, right? Yes; this test is to verify that the transaction abort logic isn't kicking in too early with the change > > > LayoutTests/storage/indexeddb/resources/transaction-abort-workers.js:86 > > +function timeoutTest() > > This tests the change to V8WorkerContextEventListener.cpp ? The timeout test tests the change to ScheduledAction.cpp (which deals with setTimeout, setInterval and friends). The V8WorkerContextEventListener.cpp change is actually tested by the createTransaction test, since that runs in response to the setVersion's transaction's "complete" event. The V8WorkerContextErrorHandler.cpp change is tested by the errorTest. The WorkerContextExecutionProxy.cpp change is not directly tested, since it would require a transaction to be created in the worker's "root" execution context and not in any event/error/scheduled callbacks, which I don't think is possible in IDB (since you get the initial DB handle via a callback). Ideas welcome, though.
Looks good to me from the V8RecursionScope angle.
Comment on attachment 128558 [details] Patch Clearing flags on attachment: 128558 Committed r108867: <http://trac.webkit.org/changeset/108867>
All reviewed patches have been landed. Closing bug.