Bug 79308 – [V8] IndexedDB: IDBTransaction objects leak in Workers

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

[V8] IndexedDB: IDBTransaction objects leak in Workers

[Joshua Bell]

Created attachment 128341 [details]
Patch

[Joshua Bell]

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.

[Dmitry Lomov]

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?

[Joshua Bell]

Created attachment 128548 [details]
Patch

[Joshua Bell]

(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.)

[Joshua Bell]

dslomov, are there other reviewers I should have take a look at this?

[Dmitry Lomov]

(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.

[Adam Barth]

This seems related to https://bugs.webkit.org/show_bug.cgi?id=72063

+rafaelw: thoughts on this patch?

[Adam Barth]

This generally seems reasonable to me, but we should check with rafaelw.

[Dmitry Lomov]

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?

[Joshua Bell]

(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

[Dmitry Lomov]

(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

[Joshua Bell]

(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.

[Dmitry Lomov]

(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!

[Joshua Bell]

Created attachment 128558 [details]
Patch

[David Grogan]

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 ?

[Joshua Bell]

(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.

[Adam Klein]

Looks good to me from the V8RecursionScope angle.

[WebKit Review Bot]

Comment on attachment 128558 [details]
Patch

Clearing flags on attachment: 128558

Committed r108867: <http://trac.webkit.org/changeset/108867>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.