Bug 79954 - IndexedDB: Check WebCommonWorkerClient::allowIndexedDB before proceeding from a shared worker
Summary: IndexedDB: Check WebCommonWorkerClient::allowIndexedDB before proceeding from...
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-02-29 15:57 PST by David Grogan
Modified: 2012-03-06 12:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (12.32 KB, patch)
2012-02-29 16:00 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (15.89 KB, patch)
2012-02-29 16:30 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (15.89 KB, patch)
2012-02-29 16:39 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (15.93 KB, patch)
2012-02-29 16:52 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (18.62 KB, patch)
2012-02-29 17:58 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (20.40 KB, patch)
2012-02-29 18:14 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (10.69 KB, patch)
2012-02-29 18:16 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (10.75 KB, patch)
2012-03-01 16:56 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (10.99 KB, patch)
2012-03-02 15:17 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (12.54 KB, patch)
2012-03-02 17:00 PST, 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-02-29 15:57:58 PST
IndexedDB: Check WebCommonWorkerClient::allowIndexedDB before proceeding from a shared worker
Comment 1 David Grogan 2012-02-29 16:00:24 PST
Created attachment 129540 [details]
Patch
Comment 2 David Grogan 2012-02-29 16:30:30 PST
Created attachment 129551 [details]
Patch
Comment 3 David Grogan 2012-02-29 16:39:13 PST
Created attachment 129552 [details]
Patch
Comment 4 David Grogan 2012-02-29 16:52:48 PST
Created attachment 129555 [details]
Patch
Comment 5 David Grogan 2012-02-29 16:59:25 PST
This is the webkit side, there still needs to be an override of WebSharedWorkerClient::allowDatabase in WebSharedWorkerClientProxy::allowDatabase.
Comment 6 WebKit Review Bot 2012-02-29 17:01:21 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 7 Joshua Bell 2012-02-29 17:31:16 PST
Comment on attachment 129555 [details]
Patch

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

lgtm (once I recovered from those pre-existing names.... WebCommonWorkerClient? ugh...)

> Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
> +    virtual bool allowIndexedDB(const WebString& name)

This can be made pure virtual once the Chromium override is in place, correct?
Comment 8 David Grogan 2012-02-29 17:43:28 PST
(In reply to comment #7)
> > Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
> > +    virtual bool allowIndexedDB(const WebString& name)
> 
> This can be made pure virtual once the Chromium override is in place, correct?

Correct.
Comment 9 David Grogan 2012-02-29 17:58:22 PST
Created attachment 129567 [details]
Patch
Comment 10 David Grogan 2012-02-29 18:04:06 PST
Michael, could you take a look at this?
Comment 11 David Grogan 2012-02-29 18:14:05 PST
Created attachment 129572 [details]
Patch

Move layout test stuff to separate patch
Comment 12 David Grogan 2012-02-29 18:16:29 PST
Created attachment 129573 [details]
Patch
Comment 13 Michael Nordman 2012-03-01 13:15:14 PST
Comment on attachment 129573 [details]
Patch

looks reasonable to me

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:58
> +    virtual bool allowIndexedDB(const WebString& name) = 0;

Please add the comment that clarifies which thread the new method is called on (similar to the others above).

> Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
> +    virtual bool allowIndexedDB(const WebString& name)

I think if the chrome-side is committed first, you don't need to provide any overriden stubs here. Looks like pure virtual allow methods declared in the base class should be enough. I think they're only replicated on this interface (sometimes with a stub) to appease the two-sided-patch-landing gods.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:-124
> -        // webFrame is not deleted as long as the process is alive, relying on

yikes... good to get rid of this webFrame code, this comment is flat out incorrect given in-process dedicated workers
Comment 14 David Grogan 2012-03-01 16:54:20 PST
Comment on attachment 129573 [details]
Patch

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

>> Source/WebKit/chromium/public/WebCommonWorkerClient.h:58
>> +    virtual bool allowIndexedDB(const WebString& name) = 0;
> 
> Please add the comment that clarifies which thread the new method is called on (similar to the others above).

Done.

>> Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
>> +    virtual bool allowIndexedDB(const WebString& name)
> 
> I think if the chrome-side is committed first, you don't need to provide any overriden stubs here. Looks like pure virtual allow methods declared in the base class should be enough. I think they're only replicated on this interface (sometimes with a stub) to appease the two-sided-patch-landing gods.

Good point.  I hadn't considered landing chrome-side first.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:-124
>> -        // webFrame is not deleted as long as the process is alive, relying on
> 
> yikes... good to get rid of this webFrame code, this comment is flat out incorrect given in-process dedicated workers

Agreed.  I wrote that comment but now I'm confused as to how I ever thought that.
Comment 15 David Grogan 2012-03-01 16:56:38 PST
Created attachment 129776 [details]
Patch
Comment 16 David Grogan 2012-03-01 17:29:11 PST
Tony, could you review this?
Comment 17 Tony Chang 2012-03-01 18:15:30 PST
Comment on attachment 129776 [details]
Patch

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

Seems fine, but please let fishd review the API change.

> Source/WebKit/chromium/ChangeLog:8
> +
> +        * public/WebCommonWorkerClient.h:

Where are the tests?  If this is covered on the chromium side, please mention which test covers it.
Comment 18 David Grogan 2012-03-02 15:17:29 PST
Created attachment 129964 [details]
Patch
Comment 19 David Grogan 2012-03-02 15:21:20 PST
(In reply to comment #17)
> View in context: https://bugs.webkit.org/attachment.cgi?id=129776&action=review
> 
> Seems fine, but please let fishd review the API change.
> 
> > Source/WebKit/chromium/ChangeLog:8
> > +
> > +        * public/WebCommonWorkerClient.h:
> 
> Where are the tests?  If this is covered on the chromium side, please mention which test covers it.

Sorry, I should have anticipated this.  Comment added in the ChangeLog file:

Tests: The 'allow' case will be tested once the patch at http://webkit.org/b/80189 and https://chromiumcodereview.appspot.com/9585031/ both land. Bug for testing the 'deny' case is http://crbug.com/113738.
Comment 20 Michael Nordman 2012-03-02 16:26:41 PST
Comment on attachment 129964 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:59
> +    virtual bool allowIndexedDB(const WebString& name) = 0;

About the webkit api.

I think we're moving away from pure virtuals in these interfaces in favor of empty stubs (so the two-sided patch landing dance has fewer steps), take a look at WebFrameClient.h for an example. With that in mind, i think it'd be good to provide inline stubs of the existing 3 methods and the new method being added.

Also, there's no reason to re-declare these same four methods in the derived WebSharedWorkerClient class. It'd be better to remove those re-declarations to eliminate the possibility of them getting out of sync.
Comment 21 David Grogan 2012-03-02 17:00:44 PST
Created attachment 129981 [details]
Patch
Comment 22 David Grogan 2012-03-02 17:02:38 PST
(In reply to comment #20)
> (From update of attachment 129964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129964&action=review
> 
> > Source/WebKit/chromium/public/WebCommonWorkerClient.h:59
> > +    virtual bool allowIndexedDB(const WebString& name) = 0;
> 
> About the webkit api.
> 
> I think we're moving away from pure virtuals in these interfaces in favor of empty stubs (so the two-sided patch landing dance has fewer steps), take a look at WebFrameClient.h for an example. With that in mind, i think it'd be good to provide inline stubs of the existing 3 methods and the new method being added.
> 
> Also, there's no reason to re-declare these same four methods in the derived WebSharedWorkerClient class. It'd be better to remove those re-declarations to eliminate the possibility of them getting out of sync.

Done and done.
Comment 23 Darin Fisher (:fishd, Google) 2012-03-06 01:22:20 PST
Comment on attachment 129981 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:-50
> -    virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) = 0;

webkit API changes LGTM
Comment 24 Tony Chang 2012-03-06 10:08:26 PST
Comment on attachment 129981 [details]
Patch

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

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:107
> +    virtual bool allowIndexedDB(const WebString& name);

Should we add OVERRIDE to these methods (separate patch)?
Comment 25 David Grogan 2012-03-06 10:57:25 PST
Comment on attachment 129981 [details]
Patch

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

>> Source/WebKit/chromium/src/WebWorkerClientImpl.h:107
>> +    virtual bool allowIndexedDB(const WebString& name);
> 
> Should we add OVERRIDE to these methods (separate patch)?

Looks like it.  Forthcoming.
Comment 26 WebKit Review Bot 2012-03-06 12:17:00 PST
Comment on attachment 129981 [details]
Patch

Clearing flags on attachment: 129981

Committed r109948: <http://trac.webkit.org/changeset/109948>
Comment 27 WebKit Review Bot 2012-03-06 12:17:06 PST
All reviewed patches have been landed.  Closing bug.