Bug 108477 - Remove call to SecurityOrigin::canAccessDatabase from IDB constructor.
Summary: Remove call to SecurityOrigin::canAccessDatabase from IDB constructor.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
Depends on: 94171
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-31 06:41 PST by Mike West
Modified: 2013-02-01 12:00 PST (History)
11 users (show)

See Also:


Attachments
Patch (4.91 KB, patch)
2013-01-31 08:11 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (7.30 KB, patch)
2013-01-31 23:59 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (11.83 KB, patch)
2013-02-01 09:05 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-01-31 06:41:52 PST
We currently call SecurityOrigin::canAccessDatabase from DOMWindowIndexedDatabase::indexedDB and WorkerContextIndexedDatabase::indexedDB. I'm not sure it's necessary after http://wkbug.com/94171's patch.

Now that we're checking canAccessDatabase in the entry points to IDB, I think we can safely remove the call here. This means that the IDB object will be created, and the property will exist on the window object; it simply won't be accessible. This, I think, is more in line with the other storage mechanisms' behavior.
Comment 1 Mike West 2013-01-31 08:11:59 PST
Created attachment 185786 [details]
Patch
Comment 2 Mike West 2013-01-31 08:13:03 PST
Joshua, would you mind taking a look at this patch? I'm pretty sure these calls are now completely redundant, but you know the code much better than I.
Comment 3 Joshua Bell 2013-01-31 08:33:36 PST
Looks good to me. 

(Watch out for tests on the Chromium side that might exercise this and expect that window.indexedDB is undefined. I didn't see one in a quick scan but you may want to do a quick linux try bot run.)
Comment 4 WebKit Review Bot 2013-01-31 10:14:48 PST
Comment on attachment 185786 [details]
Patch

Attachment 185786 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16266315

New failing tests:
http/tests/security/no-indexeddb-from-sandbox.html
Comment 5 Adam Barth 2013-01-31 10:47:19 PST
Comment on attachment 185786 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        allows us to stop holding a pointer to the ScriptExecutionContext we're
> +        extending, which can only be a good thing.

Indeed!
Comment 6 Adam Barth 2013-01-31 10:47:46 PST
> http/tests/security/no-indexeddb-from-sandbox.html

^^^ Looks like you have a test failure to work through.
Comment 7 Mike West 2013-01-31 11:49:33 PST
(In reply to comment #6)
> > http/tests/security/no-indexeddb-from-sandbox.html
> 
> ^^^ Looks like you have a test failure to work through.

Ugh. It's expecting the property not to exist in a sandboxed IFrame. I'll adjust it to expect an exception when opening the DB instead. That's consistent with the behavior when we block in a third-party context, and I think it makes more sense than hiding the API entirely.
Comment 8 Mike West 2013-01-31 23:59:32 PST
Created attachment 185959 [details]
Patch
Comment 9 Build Bot 2013-02-01 00:48:55 PST
Comment on attachment 185959 [details]
Patch

Attachment 185959 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16297527

New failing tests:
http/tests/security/no-indexeddb-from-sandbox.html
Comment 10 Build Bot 2013-02-01 02:53:07 PST
Comment on attachment 185959 [details]
Patch

Attachment 185959 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16300546

New failing tests:
http/tests/security/no-indexeddb-from-sandbox.html
Comment 11 Mike West 2013-02-01 09:05:25 PST
Created attachment 186058 [details]
Patch
Comment 12 Mike West 2013-02-01 09:06:14 PST
Amusingly, that test was "passing" on a variety of ports that don't implement IndexedDB. :) The latest patch skips it where it's not applicable.
Comment 13 Adam Barth 2013-02-01 11:26:39 PST
Comment on attachment 186058 [details]
Patch

This patch looks good.  I hopefully we won't run into compat problem from folks using the non-null-ness of window.indexedDB to check whether database access is allowed.
Comment 14 WebKit Review Bot 2013-02-01 12:00:20 PST
Comment on attachment 186058 [details]
Patch

Clearing flags on attachment: 186058

Committed r141621: <http://trac.webkit.org/changeset/141621>
Comment 15 WebKit Review Bot 2013-02-01 12:00:26 PST
All reviewed patches have been landed.  Closing bug.