1176434 - Enable indexedDB for content JS sandboxes

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Martin Thomson [:mt:]]

Attachment: sandboxIDB.patch

It seems like the restriction to Chrome JS is largely as a result of being conservative.  I have a need for this (not a need, a want really), so...

I've attached a hacked version, which has been lightly tested.  I'll add some more rigorous testing if this looks like a generally acceptable direction.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8624961 [details] [diff] [review]
sandboxIDB.patch

Review of attachment 8624961 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is reasonable. Some solid testing would be a requirement to get this landed of course.

::: dom/indexedDB/IDBFactory.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "IDBFactory.h"
>  
> +#include "AccessCheck.h"

Hopefully this isn't needed, see below.

@@ +189,3 @@
>  
>    nsAutoPtr<PrincipalInfo> principalInfo(
> +      new PrincipalInfo);

Please add parentheses, and this should now fit on one line.

@@ +189,5 @@
>  
>    nsAutoPtr<PrincipalInfo> principalInfo(
> +      new PrincipalInfo);
> +  nsIPrincipal* principal =
> +      xpc::AccessCheck::getPrincipal(js::GetObjectCompartment(aOwningObject));

I'd prefer you use |nsContentUtils::ObjectPrincipal(aOwningObject)| here.

@@ +192,5 @@
> +  nsIPrincipal* principal =
> +      xpc::AccessCheck::getPrincipal(js::GetObjectCompartment(aOwningObject));
> +  bool isSystem;
> +  if (!AllowedForPrincipal(principal, isSystem)) {
> +    return NS_ERROR_DOM_NOT_SUPPORTED_ERR;

This should probably be NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR.

The code you have here is a special one that we're using elsewhere to make the 'indexedDB' property be null in the case of third party windows (sort of a temporary hack that should go away someday).

Comment 2

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1176434 - Enabling indexedDB for content JS sandboxes, r=bent

Bug 1176434 - Enabling indexedDB for content JS sandboxes

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8625800 [details]
MozReview Request: Bug 1176434 - Enabling indexedDB for content JS sandboxes, r=bent

https://reviewboard.mozilla.org/r/11921/#review10459

Looks good\! r=me with these fixed:

::: dom/indexedDB/IDBFactory.cpp:190
(Diff revision 1)
> -    new PrincipalInfo(SystemPrincipalInfo()));
> +  nsIPrincipal* principal = nsContentUtils::ObjectPrincipal(aOwningObject);

Can you assert that |principal| is non-null here?

::: js/xpconnect/tests/mochitest/mochitest.ini:111
(Diff revision 1)
> +[test_sandbox_idb.html]

These tests are great but they're really tests of indexedDB, can you move them into dom/indexedDB/test instead of adding them to js/xpconnect?

::: js/xpconnect/tests/unit/test_sandbox_idb.js:39
(Diff revision 1)
> +    put: function(k, v) {

If you changed this to use "add" instead of "put" you would also be testing that the database opened in the sandbox is a different database from that opened outside the sandbox. As it is now you could be writing the same value twice into the same database and the test would pass.

::: js/xpconnect/tests/unit/xpcshell.ini:130
(Diff revision 1)
> +[test_sandbox_idb.js]

These tests are great but they're really tests of indexedDB, can you move them into dom/indexedDB/test/unit instead of adding them to js/xpconnect?

Comment 4

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d1b6e69bb00
n-i self for checkin.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Doesn't look like the attached patch ever got updated for the review comments.

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 8625800 [details]
MozReview Request: Bug 1176434 - Enabling indexedDB for content JS sandboxes, r=bent

Bug 1176434 - Enabling indexedDB for content JS sandboxes, r=bent

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8625800 [details]
MozReview Request: Bug 1176434 - Enabling indexedDB for content JS sandboxes, r=bent

r=bent

Comment 8

[Martin Thomson [:mt:]]

Thanks for checking on me.

Comment 9

[Martin Thomson [:mt:]]

damn, dinnae work

Comment 10

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83ff704f1ecc

Comment 11

[Martin Thomson [:mt:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/335fe5e5e8e5

Comment 12

[Martin Thomson [:mt:]]

Oh, and here's the proof that this was OK, I just screwed up the try push first time around: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fc6920b6aea

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/335fe5e5e8e5