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

RESOLVED FIXED in mozilla33

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: mak)

Tracking

unspecified
mozilla33
Points:
8
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
FTR, about:looppanel and about:loopconversation are unprivileged.
(Reporter)

Comment 2

4 years ago
Created attachment 8443461 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch
(Reporter)

Comment 3

4 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)
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

4 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

4 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)
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]
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+
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

4 years ago
Created attachment 8447611 [details] [diff] [review]
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.
Assignee: nobody → ttaubert
Attachment #8443461 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8447611 - Flags: review?(bzbarsky)
(Reporter)

Updated

4 years ago
Component: Client → DOM
Product: Loop → Core
(Reporter)

Updated

4 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

4 years ago
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.

Updated

4 years ago
Blocks: 1000112
I've filed bug 1033587 with notes on a possible longer-term solution as requested by bz in comment 8.
(Reporter)

Comment 14

4 years ago
Created attachment 8449768 [details] [diff] [review]
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();
Attachment #8447611 - Attachment is obsolete: true
Attachment #8449768 - Flags: review+
(Reporter)

Comment 15

4 years ago
Created attachment 8449769 [details] [diff] [review]
0002-Bug-1028187-Fix-up-additional-nsIAboutModule-instanc.patch

Had to add some GetIndexedDBOriginPostfix() stubs after touching nsIAboutModule.
Attachment #8449769 - Flags: review?(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.
Attachment #8449769 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 17

4 years ago
Fixed nsAboutBloat and squashed everything:

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

Comment 18

4 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

4 years ago
Created attachment 8450988 [details] [diff] [review]
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
Attachment #8449768 - Attachment is obsolete: true
Attachment #8449769 - Attachment is obsolete: true
Attachment #8450988 - Flags: review?(bzbarsky)
(Reporter)

Comment 21

4 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

4 years ago
You can also run dom/indexedDB/test mochitests locally and if that passes, push to try.
(Reporter)

Comment 23

4 years ago
Created attachment 8451196 [details] [diff] [review]
0001-Bug-1028187-Enable-IndexedDB-for-about-looppanel-and.patch, v5

Better.

https://tbpl.mozilla.org/?tree=Try&rev=dcd13ed0ffd2
Attachment #8450988 - Attachment is obsolete: true
Attachment #8451196 - Flags: review?(bzbarsky)
(Reporter)

Comment 24

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

Updated

4 years ago
QA Whiteboard: [qa?]
Flags: firefox-backlog+

Updated

4 years ago
Assignee: ttaubert → mak77
Iteration: --- → 33.3
Points: --- → 8
QA Whiteboard: [qa?] → [qa-]

Updated

4 years ago
See Also: → bug 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+
(Assignee)

Comment 26

4 years ago
Created attachment 8456765 [details] [diff] [review]
patch v6

This one should address all of the comments.
Attachment #8451196 - Attachment is obsolete: true
(Assignee)

Comment 27

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/acf444215f8d
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/acf444215f8d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
See Also: → bug 1040771

Comment 29

4 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

4 years ago
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?

Comment 32

4 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

4 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.