Closed Bug 1176434 Opened 6 years ago Closed 6 years ago

Enable indexedDB for content JS sandboxes

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: mt, Assigned: mt)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch sandboxIDB.patch (obsolete) — Splinter Review
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.
Attachment #8624961 - Flags: feedback?(bent.mozilla)
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).
Attachment #8624961 - Flags: feedback?(bent.mozilla) → feedback+
Bug 1176434 - Enabling indexedDB for content JS sandboxes
Attachment #8625800 - Flags: review?(bent.mozilla)
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?
Attachment #8625800 - Flags: review?(bent.mozilla) → review+
Attachment #8624961 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d1b6e69bb00
n-i self for checkin.
Flags: needinfo?(martin.thomson)
Assignee: nobody → martin.thomson
Flags: needinfo?(martin.thomson)
Keywords: checkin-needed
Doesn't look like the attached patch ever got updated for the review comments.
Keywords: checkin-needed
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
Attachment #8625800 - Attachment description: MozReview Request: Bug 1176434 - Enabling indexedDB for content JS sandboxes → MozReview Request: Bug 1176434 - Enabling indexedDB for content JS sandboxes, r=bent
Attachment #8625800 - Flags: review+
Comment on attachment 8625800 [details]
MozReview Request: Bug 1176434 - Enabling indexedDB for content JS sandboxes, r=bent

r=bent
Attachment #8625800 - Flags: review+
Thanks for checking on me.
Keywords: checkin-needed
damn, dinnae work
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/335fe5e5e8e5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.