Bug 83952 - IDB: Pending setVersion call in worker prevents document from opening the db, even after worker termination
Summary: IDB: Pending setVersion call in worker prevents document from opening the db,...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-13 15:51 PDT by David Grogan
Modified: 2012-04-18 11:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.71 KB, patch)
2012-04-13 15:51 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (9.27 KB, patch)
2012-04-17 16:38 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-04-13 15:51:08 PDT
Pending setVersion transaction prevents document from opening that db, even after worker termination
Comment 1 David Grogan 2012-04-13 15:51:30 PDT
Created attachment 137168 [details]
Patch
Comment 2 David Grogan 2012-04-16 15:02:16 PDT
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.
Comment 3 Joshua Bell 2012-04-17 10:36:23 PDT
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; } })
Comment 4 David Grogan 2012-04-17 16:38:36 PDT
Created attachment 137633 [details]
Patch
Comment 5 David Grogan 2012-04-17 16:39:48 PDT
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.
Comment 6 David Grogan 2012-04-17 16:45:02 PDT
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.
Comment 7 Joshua Bell 2012-04-17 16:48:01 PDT
Comment on attachment 137633 [details]
Patch

lgtm
Comment 8 David Grogan 2012-04-17 19:00:09 PDT
Ojan, could you review this?
Comment 9 WebKit Review Bot 2012-04-18 11:40:47 PDT
Comment on attachment 137633 [details]
Patch

Clearing flags on attachment: 137633

Committed r114540: <http://trac.webkit.org/changeset/114540>
Comment 10 WebKit Review Bot 2012-04-18 11:40:52 PDT
All reviewed patches have been landed.  Closing bug.