IDB: Pending setVersion call in worker prevents document from opening the db, even after worker termination

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

Pending setVersion transaction prevents document from opening that db, even after worker termination

[David Grogan]

Created attachment 137168 [details]
Patch

[David Grogan]

Josh, Alec, could you take a look at these layout tests before I ask ojan or tony to review?

The chrome bug these exercise is http://code.google.com/p/chromium/issues/detail?id=123418.

[Joshua Bell]

Comment on attachment 137168 [details]
Patch

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

The logic of the test lgtm

> LayoutTests/storage/indexeddb/pending-version-change-stuck-works-with-terminate.html:15
> +  request.onsuccess = startTheWorker;

I'd put this assignment in an "else" clause to the if block below, to improve readability.

> LayoutTests/storage/indexeddb/pending-version-change-stuck-works-with-terminate.html:28
> +  realFinishJSTest = finishJSTest;

Rather than this IMHO fragile hooking into the finishJSTest mechanism, how about adding a new worker script that does a custom postMessage?

(At some point we should add a better hook mechanism, e.g. startWorker(script, function myfilter(message) { if (handled) { return true; } })

[David Grogan]

Created attachment 137633 [details]
Patch

[David Grogan]

Comment on attachment 137168 [details]
Patch

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

>> LayoutTests/storage/indexeddb/pending-version-change-stuck-works-with-terminate.html:15
>> +  request.onsuccess = startTheWorker;
> 
> I'd put this assignment in an "else" clause to the if block below, to improve readability.

Done.

>> LayoutTests/storage/indexeddb/pending-version-change-stuck-works-with-terminate.html:28
>> +  realFinishJSTest = finishJSTest;
> 
> Rather than this IMHO fragile hooking into the finishJSTest mechanism, how about adding a new worker script that does a custom postMessage?
> 
> (At some point we should add a better hook mechanism, e.g. startWorker(script, function myfilter(message) { if (handled) { return true; } })

Done.  Agreed on the better hook mechanism.  My plan was to add a method to the worker object that would let scripts assign handlers to codes.

[David Grogan]

Comment on attachment 137633 [details]
Patch

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

> LayoutTests/storage/indexeddb/pending-version-change-stuck-works-with-terminate.html:35
> +    originalOnMessage = worker.onmessage;

This is still hacky but less hacky than the old patch. Just this bit of marginal unhackiness was enough to reveal that the tests did not pass in DRT either, so thanks for the suggestion.

The unexpectedSuccessCallback from the worker was calling finishJSTest which reloaded the page with "?second", something I thought was being done by the finishJSTest call in the blocked handler.

> LayoutTests/storage/indexeddb/pending-version-change-stuck-works-with-terminate.html:38
> +            worker.terminate();

This line is the only difference between pending-version-change-stuck.html and pending-version-change-stuck-works-with-terminate.html.

[Joshua Bell]

Comment on attachment 137633 [details]
Patch

lgtm

[David Grogan]

Ojan, could you review this?

[WebKit Review Bot]

Comment on attachment 137633 [details]
Patch

Clearing flags on attachment: 137633

Committed r114540: <http://trac.webkit.org/changeset/114540>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.