1079355 - Make 'dom.indexeddb.enabled' pref only affect content

Status: VERIFIED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[:Gijs (he/him)]

Per bug 1078085, lots of stuff depends on this pref, essentially turning it into a footgun.

As with bug 1013457, I think we should remove prefs which are footguns.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Was thinking about filing that too :) Thanks!

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The pref is there in case we have a critical security issue that we can't fix in a timely manner, and I don't want to remove it. Flipping it will indeed break some stuff, but that's true of lots of our semi-hidden prefs. Users should not flip them unless they know what they're doing.

Comment 3

[:Gijs (he/him)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> Flipping it will
> indeed break some stuff, but that's true of lots of our semi-hidden prefs.
> Users should not flip them unless they know what they're doing.

Sure, but the reality is that they do, and then end up wasting engineering/QA/support time to try to diagnose why stuff is broken and whether/how to fix it.

Can we not make this content-only?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> The pref is there in case we have a critical security issue that we can't
> fix in a timely manner, and I don't want to remove it.

This seems nice in theory, but I don't see us ever making this tradeoff (indexedDB is unbreakable at this point). We don't have prefs to disable querySelectorAll or XHR.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Making it content-only seems ok to me.

Comment 9

[msth67]

So the solution is to remove a preference that controls a database virtually unlimited in size and for which there is no control interface whatsoever,in order to prevent users from "turning it into a footgun" ?

What is Nightly storing there to begin with,given that the release version still works OK with dom.indexedDB.enabled toggled to false (I haven't tested with Beta and Aurora or whatever it is named now) ?

Is the most sensible choice to force everyone to have this database enabled,security-wise and privacy-wise ? 

Shouldn't such a powerful storage method be controllable by the user,not unlike cookies,LSOs and cache,if he wishes so?

Comment 10

[:Gijs (he/him)]

Attachment: indexedDB pref should only apply for content pages, not chrome ones,

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3acbc7fc8852
Something like this?

Comment 11

[:Gijs (he/him)]

(In reply to msth67 from comment #9)
> Shouldn't such a powerful storage method be controllable by the user,not
> unlike cookies,LSOs and cache,if he wishes so?

A global on/off switch is a poor way to control such a feature - we do better for cookies; if we're convinced we need to do something here for privacy's sake, it should be better, too.

In any case, the need here is mostly for this to stop breaking chrome code, which is already at liberty to use other forms of storage (at worst, just writing its own (st)reams of data directly to disk in whatever format it likes). The patch I just put up should not affect the current pref's functionality for content (and didn't in my testing).

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8544250 [details] [diff] [review]
indexedDB pref should only apply for content pages, not chrome ones,

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

I think this looks fine with one change below. Thanks!

::: dom/indexedDB/IDBFactory.cpp
@@ +263,5 @@
>                                         IDBFactory** aFactory)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  if (aPrincipalInfo && aPrincipalInfo->type() != PrincipalInfo::TSystemPrincipalInfo &&

Nit: aPrincipalInfo should never be null so no need to check that.

(I wouldn't mind adding an assertion to that effect right after the main thread assertion above!)

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Gijs Kruitbosch from comment #11)
> A global on/off switch is a poor way to control such a feature - we do
> better for cookies; if we're convinced we need to do something here for
> privacy's sake, it should be better, too.

Eventually all indexedDB+localStorage+cookies+whateverElse will be treated the same with respect to clearing and third-party behavior.

Comment 14

[msth67]

Does this mean that the indexedDB/localStorage is currently out of the user's direct control and that the assimilation of this kind of storage to other data such as cookies and the likes is not coming anytime soon?

Shouldn't the priority of this bug be risen, given that until then huge amounts of data can be silently stored inside Firefox with the users not even suspecting that?

Comment 15

[:Gijs (he/him)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #12)
> Comment on attachment 8544250 [details] [diff] [review]
> indexedDB pref should only apply for content pages, not chrome ones,
> 
> Review of attachment 8544250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks fine with one change below. Thanks!
> 
> ::: dom/indexedDB/IDBFactory.cpp
> @@ +263,5 @@
> >                                         IDBFactory** aFactory)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +  if (aPrincipalInfo && aPrincipalInfo->type() != PrincipalInfo::TSystemPrincipalInfo &&
> 
> Nit: aPrincipalInfo should never be null so no need to check that.
> 
> (I wouldn't mind adding an assertion to that effect right after the main
> thread assertion above!)

Done. Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/15c6f78cd4ff

msth67: this bug is solely about ensuring Firefox's own code keeps working if you turn the pref off. It doesn't affect anything else. If you think there are other issues with indexedDB and Firefox and the amount of control users have over the web using it, please look for other bugs covering those (based on comment #13, it sounds like they might exist already) and/or file a new bug.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15c6f78cd4ff

Comment 20

[:Gijs (he/him)]

Comment on attachment 8544250 [details] [diff] [review]
indexedDB pref should only apply for content pages, not chrome ones,

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: somehow a lot of people have toggled this pref and now many of their add-ons are breaking in 35. See bug 1122204 (duped here) and bug . The error message is non-descriptive, and this patch fixes the issue completely.
[Describe test coverage new/current, TBPL]: nope :-(
[Risks and why]: very low. Unless I and Ben missed something in the patch, all this does is ensure that add-ons/chrome can always use indexeddb, even if it's turned off for content.
[String/UUID change made/needed]: nope

Comment 21

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #20)
> See bug 1122204 (duped
> here) and bug . 

I meant to quote bug 1122030.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Needs to be rebased around bug 701634 for the beta uplift.

Comment 23

[:Gijs (he/him)]

Attachment: Patch for beta

Ah, sorry about that. I'd be landing this now except the tree just closed...

Comment 25

[al_9x]

disabled idexeddb breaks Fx 35 (Bug 1122861 and possibly other issues)

why isn't this backported to the versions that is affected? why knowingly release broken updates?

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You shouldn't be pressing buttons just because they're there.  Disabling IndexedDB is not a supported configuration.

Comment 28

[al_9x]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #27)
> You shouldn't be pressing buttons just because they're there.  Disabling
> IndexedDB is not a supported configuration.

No one is looking for your "support" for this "configuration".  It was a bug for the setting to break chrome and it was fixed, but it should have been backported to 35.

Follow your own advice and avoid clicking the submit button just because it's there, you have nothing of value to contribute here.

Comment 29

[Jan Varga [:janv]]

(In reply to al_9x from comment #28)
> Follow your own advice and avoid clicking the submit button just because
> it's there, you have nothing of value to contribute here.

Hey, calm down please.

Comment 31

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/97b34f0b9946

Comment 33

[[e-mail]]

This part of comment 28 does not deserve to be hidden as off-topic.

> No one is looking for your "support" for this "configuration".  It was a bug
> for the setting to break chrome and it was fixed, but it should have been
> backported to 35.

Comment 34

[:Gijs (he/him)]

(In reply to [e-mail] from comment #33)
> This part of comment 28 does not deserve to be hidden as off-topic.
> 
> > No one is looking for your "support" for this "configuration".  It was a bug
> > for the setting to break chrome.

It had broken chrome code in 34 as well. Just different, harder to notice bits (I see errors in the console on about:home, for instance, when running with this pref disabled on earlier versions).

The pref was originally added to disable a new web feature if security or API concerns warranted doing so. Now that it's proved itself and we're using it in chrome, we updated what the pref did. The pref wasn't put there for users to toggle because they felt like it - much like, say, the NSS protocol prefs are there so that we can quickly and efficiently disable/enable ciphers by default, and if people mess with those prefs themselves there are few if any guarantees about the result being web-compatible. That this wasn't a theoretical issue was, coincidentally proven in 35 as well, as some people's connections to google broke because they changed those prefs.

The warning in front of about:config is there for good reason: don't mess with the prefs if you're not able or prepared to debug a "broken" Firefox and don't know what the prefs do, and don't blame us when stuff breaks if you change any of the prefs not surfaced in the UI.

> > and it was fixed, but it should have been
> > backported to 35.

35rc had already been built by the time this was fixed, and landing it straight to release with 0 testing on nightly would have been irresponsible. So no, it could not have been, and even now for 35.0.1 I would say it should not have been. Turning on the pref is a straightforward fix, and pulling forward this fix even to 36 was not without risk because of the unrelated changes in the indexeddb code that bitrotted the fix.

Speaking of which... Florin, can you reconsider the qe- at least for 36? Thank you.

Comment 35

[Florin Mezei, QA (:FlorinMezei)]

(In reply to :Gijs Kruitbosch from comment #34)
> Speaking of which... Florin, can you reconsider the qe- at least for 36?

Sure thing Gijs! I've set this as qe- initially as it did not seem like a very widespread problem, being a non-standard configuration. Considering your assessment of this being a potentially risky change I've now set qe+ so it will get the needed attention from the QA team.

Gijs, could you please provide some guidelines on what to look for (what might be affected by this), so we can make sure that this gets the proper amount of testing? We can verify that disabling the pref no longer breaks things, but I'm guessing there are other areas that may be at risk.

Comment 36

[:Gijs (he/him)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #35)
> (In reply to :Gijs Kruitbosch from comment #34)
> > Speaking of which... Florin, can you reconsider the qe- at least for 36?
> 
> Sure thing Gijs! I've set this as qe- initially as it did not seem like a
> very widespread problem, being a non-standard configuration. Considering
> your assessment of this being a potentially risky change I've now set qe+ so
> it will get the needed attention from the QA team.
> 
> Gijs, could you please provide some guidelines on what to look for (what
> might be affected by this), so we can make sure that this gets the proper
> amount of testing? We can verify that disabling the pref no longer breaks
> things, but I'm guessing there are other areas that may be at risk.

I guess checking both the case of the pref being disabled and the pref being enabled resulting in no relevant errors in chrome, and the pref being disabled still resulting in the code throwing sane error messages for content. Mostly, that beta (in this respect) behaves no worse than Nightly.

Comment 37

[Cornel Ionce [:cornel_ionce], Desktop Release QA]

Verified on Windows 7 64-bit, Windows 8.1 32-bit, Ubuntu 12.04 and Mac OS X 10.9.5 using latest Aurora and Firefox 36 beta 6 (build ID: 20150202183609) with the "dom.indexeddb.enabled" pref set to true/false.
Also ran through the entire list of duplicates to make sure they are no longer reproducing and that the browser is no longer affected.