Closed
Bug 1028187
Opened 10 years ago
Closed 10 years ago
Allow opting in to IndexedDB for about: pages optionally specifying a desired origin
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
People
(Reporter: ttaubert, Assigned: mak)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
FTR, about:looppanel and about:loopconversation are unprivileged.
Reporter | ||
Comment 2•10 years ago
|
||
Reporter | ||
Comment 3•10 years ago
|
||
Is sharing an origin what Loop wants/needs? So that both pages could access the same DBs?
Flags: needinfo?(standard8)
Flags: needinfo?(rgauthier)
Comment 4•10 years ago
|
||
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...
Reporter | ||
Comment 5•10 years ago
|
||
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.
Reporter | ||
Comment 6•10 years ago
|
||
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.
Attachment #8443461 -
Flags: review?(bzbarsky)
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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.
Attachment #8443461 -
Flags: review?(bzbarsky) → review+
Comment 9•10 years ago
|
||
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...
Reporter | ||
Comment 10•10 years ago
|
||
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
Status: NEW → ASSIGNED
Attachment #8447611 -
Flags: review?(bzbarsky)
Reporter | ||
Updated•10 years ago
|
Component: Client → DOM
Product: Loop → Core
Reporter | ||
Updated•10 years ago
|
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
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(standard8)
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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•10 years ago
|
||
I've filed bug 1033587 with notes on a possible longer-term solution as requested by bz in comment 8.
Reporter | ||
Comment 14•10 years ago
|
||
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+
Reporter | ||
Comment 15•10 years ago
|
||
Had to add some GetIndexedDBOriginPostfix() stubs after touching nsIAboutModule.
Attachment #8449769 -
Flags: review?(bzbarsky)
Comment 16•10 years ago
|
||
Comment on attachment 8449769 [details] [diff] [review]
0002-Bug-1028187-Fix-up-additional-nsIAboutModule-instanc.patch
r=me, but please fix nsAboutBloat as well.
Attachment #8449769 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 17•10 years ago
|
||
Fixed nsAboutBloat and squashed everything:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caf43baf3306
Reporter | ||
Comment 18•10 years ago
|
||
Pushed small follow-up to fix non-unified build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01cba8ff52dd
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
Reporter | ||
Comment 20•10 years ago
|
||
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
Attachment #8449768 -
Attachment is obsolete: true
Attachment #8449769 -
Attachment is obsolete: true
Attachment #8450988 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
You can also run dom/indexedDB/test mochitests locally and if that passes, push to try.
Reporter | ||
Comment 23•10 years ago
|
||
Attachment #8450988 -
Attachment is obsolete: true
Attachment #8451196 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 24•10 years ago
|
||
Please feel free to pick this up and drive it forward/land it while I'm on PTO. Thanks!
Updated•10 years ago
|
QA Whiteboard: [qa?]
Flags: firefox-backlog+
Updated•10 years ago
|
Assignee: ttaubert → mak77
Iteration: --- → 33.3
Points: --- → 8
QA Whiteboard: [qa?] → [qa-]
Updated•10 years ago
|
Comment 25•10 years ago
|
||
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+
Assignee | ||
Comment 26•10 years ago
|
||
This one should address all of the comments.
Attachment #8451196 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Target Milestone: --- → mozilla33
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Comment 29•10 years ago
|
||
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
Reporter | ||
Comment 30•10 years ago
|
||
Uh, I wasn't aware that NS_ENSURE_TRUE warns :/ We should convert those to just return or use NS_ENSURE_SUCCESS.
Comment 31•10 years ago
|
||
Isn't it the case that NS_ENSURE_* warns?
Comment 32•10 years ago
|
||
(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•10 years ago
|
||
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.
Description
•