Closed Bug 1028187 Opened 7 years ago Closed 7 years ago

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


(Core :: Storage: IndexedDB, defect)

Not set





(Reporter: ttaubert, Assigned: mak)




(1 file, 6 obsolete files)

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.
FTR, about:looppanel and about:loopconversation are unprivileged.
Is sharing an origin what Loop wants/needs? So that both pages could access the same DBs?
Flags: needinfo?(standard8)
Flags: needinfo?(rgauthier)
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...
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 on attachment 8443461 [details] [diff] [review]

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.
Attachment #8443461 - Flags: review?(bzbarsky)
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 :)
Flags: needinfo?(rgauthier)
Comment on attachment 8443461 [details] [diff] [review]

>+    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.
Attachment #8443461 - Flags: review?(bzbarsky) → review+
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...
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.
Assignee: nobody → ttaubert
Attachment #8443461 - Attachment is obsolete: true
Attachment #8447611 - Flags: review?(bzbarsky)
Component: Client → DOM
Product: Loop → Core
Component: DOM → DOM: IndexedDB
Summary: Enable IndexedDB for about:looppanel and about:loopconversation, sharing an origin → Allow opting in to IndexedDB for about: pages optionally specifying a desired origin
Flags: needinfo?(standard8)
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.
Attachment #8447611 - Flags: review?(bzbarsky) → review+
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.
I've filed bug 1033587 with notes on a possible longer-term solution as requested by bz in comment 8.
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();
Attachment #8447611 - Attachment is obsolete: true
Attachment #8449768 - Flags: review+
Had to add some GetIndexedDBOriginPostfix() stubs after touching nsIAboutModule.
Attachment #8449769 - Flags: review?(bzbarsky)
Comment on attachment 8449769 [details] [diff] [review]

r=me, but please fix nsAboutBloat as well.
Attachment #8449769 - Flags: review?(bzbarsky) → review+
Pushed small follow-up to fix non-unified build bustage:
I had to back this out for apparently adding an assertion to mochitest-oth runs:

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
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.
Attachment #8449768 - Attachment is obsolete: true
Attachment #8449769 - Attachment is obsolete: true
Attachment #8450988 - Flags: review?(bzbarsky)
Comment on attachment 8450988 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v4

Ok, wow. That's even worse.
Attachment #8450988 - Flags: review?(bzbarsky)
You can also run dom/indexedDB/test mochitests locally and if that passes, push to try.
Please feel free to pick this up and drive it forward/land it while I'm on PTO. Thanks!
QA Whiteboard: [qa?]
Flags: firefox-backlog+
Assignee: ttaubert → mak77
Iteration: --- → 33.3
Points: --- → 8
QA Whiteboard: [qa?] → [qa-]
See Also: → 1033587
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.  :(
Attachment #8451196 - Flags: review?(bzbarsky) → review+
Attached patch patch v6Splinter Review
This one should address all of the comments.
Attachment #8451196 - Attachment is obsolete: true
Target Milestone: --- → mozilla33
Closed: 7 years ago
Resolution: --- → FIXED
See Also: → 1040771
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);

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
Uh, I wasn't aware that NS_ENSURE_TRUE warns :/ We should convert those to just return or use NS_ENSURE_SUCCESS.
Isn't it the case that NS_ENSURE_* warns?
(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.
Hm, does this need to use separate origin directories for apps ?
The original patch ignores the jar prefix, is it on purpose ?
You need to log in before you can comment on or make changes to this bug.