1028187 - Allow opting in to IndexedDB for about: pages optionally specifying a desired origin

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Talking to :tOkeshu he told me that about:loop* pages need IndexedDB storage and folks are currently experimenting using message passing for that. Seems like we should just follow bug 861302 with the exception that we make both about:loop* pages share an artificial origin.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

FTR, about:looppanel and about:loopconversation are unprivileged.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Is sharing an origin what Loop wants/needs? So that both pages could access the same DBs?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Yes.  We've taken about a number of different runs at that before (see bug 976109 for all the fun), and ended up punting on it to make a deadline.  That said, I suspect sharing an origin is going to help us avoid a bunch of issues going forward, and think it's worth another try.  My guess is that an "app:" origin is the next thing to try.  If you're interested in taking a run at any of this, I'm happy to chat on video to fill you in on past experience...

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

We can totally chat on video if there's more to know. The attached patch implements a shared origin for both pages (wrt to Indexed DB) and should make Indexed DB work for those.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8443461 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch

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

Boris, are you okay with adding another exception for about:loop* pages the same way we handle about:home? They both share an origin. The artificial origin contains a "::" to ensure that no other about: page could get access to that data.

Comment 7

[Romain Gauthier [:tOkeshu]]

I applied the patch and can confirm it works in the loop panel.
As :dmose said, we probably want a shared origin for the panel and the conversation.

Thanks :ttaubert for the patch so we can move forward with the contacts in loop :)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8443461 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch

>+    nsCString group = EmptyCString();

Just drop the "= EmptyCString()" bit.  The string starts out empty.

I'm really not a huge fan of this sort of hardcoding.  It seems like other about: URIs (e.g. extension-provided ones) might want similar things, no?

Speaking of which, extensions could be overriding the "looppanel" and "loopconversation" about modules, in which case this patch might be creating a security hole for such extensions...

>+              group.AssignLiteral("moz-safe-about::loop");

Why not just make the group be "about:looppanel" or the same thing with "moz-safe-about"?  That seems like it would provide "matches this about: URI" guarantees better than the "::" thing.  That last seems to be relying on some sort of undocumented behavior.

In general, I'm ok with this as a stopgap if we're desperate for time, but only if there's a followup bug filed, with a credible plan with timeframes to fix it, for doing this in a sane way that doesn't assume that about:looppanel is the about:looppanel we're shipping by default.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and patches touching nsGlobalWindow should probably live in bugs in the Core product, which will have the appropriate tracking flags and might actually be found by someone...

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v2

Thanks for the feedback. What do you think about this approach? It would disable IndexedDB for overriden about: pages by default and would allow add-ons to enable IndexedDB and choose their own origins.

To not make it too easy to get access to a real-world web site's indexed DB data we only allow specifying the postfix of the desired origin, that means e.g. "moz-about-safe:" is prepended automatically.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8447611 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v2

Why the new interface?  Why not just put this on nsIAboutModule?

Other than that, this makes me much happier.  r=me with that change or an explanation of why not.  In any case, the _constant_ should be on nsIAboutModule, since it shares a value space with the constants there.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8447611 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v2

Actaully, one more thing:

>+      result.AssignLiteral(postfix);

Should be AssignASCII.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

I've filed bug 1033587 with notes on a possible longer-term solution as requested by bz in comment 8.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v3

Carrying over r=bz.

Moved everything to nsIAboutModule and removed nsIAboutModule2. s/AssignLiteral/AssignASCII/. Includes the following skipThirdPartyCheck fix to not enable IndexedDB for all about: URIs:

> origin = GetIndexedDBOriginPostfixForAboutURI(uri);
> skipThirdPartyCheck = !origin.IsEmpty();

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1028187-Fix-up-additional-nsIAboutModule-instanc.patch

Had to add some GetIndexedDBOriginPostfix() stubs after touching nsIAboutModule.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8449769 [details] [diff] [review]
0002-Bug-1028187-Fix-up-additional-nsIAboutModule-instanc.patch

r=me, but please fix nsAboutBloat as well.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Fixed nsAboutBloat and squashed everything:

https://hg.mozilla.org/integration/mozilla-inbound/rev/caf43baf3306

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Pushed small follow-up to fix non-unified build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/01cba8ff52dd

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for apparently adding an assertion to mochitest-oth runs: https://tbpl.mozilla.org/php/getParsedLog.php?id=42982986&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/878fe076a95f



16:37:18     INFO -  [Parent 968] ###!!! ASSERTION: Non-chrome may not supply their own origin!: 'aASCIIOrigin.IsEmpty() || nsContentUtils::IsCallerChrome()', file /builds/slave/m-in-osx64-d-00000000000000000/build/dom/indexedDB/IDBFactory.cpp, line 111

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v4

The (non-fatal, uh) assertion was caused due to passing an explicit origin to IDBFactory::Create() - which it doesn't like for non-chrome principals.

I moved determining the origin from nsGlobalWindow to QuotaManager::GetInfoFromPrincipal() instead.

https://tbpl.mozilla.org/?tree=Try&rev=ffeaa2994270

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8450988 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v4

Ok, wow. That's even worse.

Comment 22

[Jan Varga [:janv]]

You can also run dom/indexedDB/test mochitests locally and if that passes, push to try.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v5

Better.

https://tbpl.mozilla.org/?tree=Try&rev=dcd13ed0ffd2

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Please feel free to pick this up and drive it forward/land it while I'm on PTO. Thanks!

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8451196 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v5

>+++ b/browser/components/about/AboutRedirector.cpp
>+    "home" },

How come this is needed but "loopconversation" doesn't need a postfix?  I expect this line is not needed.

>+++ b/dom/quota/QuotaManager.cpp
>+TryGetInfoForAboutURI(nsIPrincipal* aPrincipal,
>+  rv = uri->SchemeIs("about", &isAbout);

Should probably return with error before this line if !uri, right?

Alternately, you could move the call to this function to after the IsSystemPrincipal check in GetInfoFromPrincipal, which would allow you to assume uri is not null here.

>+  aGroup->Assign(origin);
>+  aASCIIOrigin->Assign(origin);

These should probably be references, not pointers.  That's how string outparams normally work in Gecko.

r=me with that, and sorry for the lag.  :(

Comment 26

[Marco Bonardo [:mak]]

Attachment: patch v6

This one should address all of the comments.

Comment 27

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/acf444215f8d

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/acf444215f8d

Comment 29

[Jan Varga [:janv]]

Comment on attachment 8456765 [details] [diff] [review]
patch v6

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

::: dom/quota/QuotaManager.cpp
@@ +2031,5 @@
> +  }
> +
> +  bool isAbout;
> +  rv = uri->SchemeIs("about", &isAbout);
> +  NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && isAbout, NS_ERROR_FAILURE);

This is obviously wrong, why do you want to warn if the scheme is not "about" ?

We're now getting tons of warnings like:
1168 INFO [58385] WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && isAbout) failed: file /Users/varga/Sources/Jamun/dom/quota/QuotaManager.cpp, line 2080

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Uh, I wasn't aware that NS_ENSURE_TRUE warns :/ We should convert those to just return or use NS_ENSURE_SUCCESS.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Isn't it the case that NS_ENSURE_* warns?

Comment 32

[Jan Varga [:janv]]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #31)
> Isn't it the case that NS_ENSURE_* warns?

Yes, that one warns too.
I'll try to clean it up by myself.

Comment 33

[Jan Varga [:janv]]

Hm, does this need to use separate origin directories for apps ?
The original patch ignores the jar prefix, is it on purpose ?