Closed Bug 1079355 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla37
Tracking Status
firefox36 --- verified
firefox37 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(2 files)

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.
Was thinking about filing that too :) Thanks!
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
(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?
Flags: needinfo?(bent.mozilla)
(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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Making it content-only seems ok to me.
Flags: needinfo?(bent.mozilla)
Duplicate of this bug: 1107076
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?
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
(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 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!)
Attachment #8544250 - Flags: review?(bent.mozilla) → review+
Summary: Get rid of dom.indexeddb.enabled pref (or make it content-only) → Make 'dom.indexeddb.enabled' pref only affect content
(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.
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?
(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.
https://hg.mozilla.org/mozilla-central/rev/15c6f78cd4ff
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Duplicate of this bug: 1121181
Duplicate of this bug: 1122204
Duplicate of this bug: 1122030
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
Attachment #8544250 - Flags: approval-mozilla-beta?
(In reply to :Gijs Kruitbosch from comment #20)
> See bug 1122204 (duped
> here) and bug . 

I meant to quote bug 1122030.
Attachment #8544250 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs to be rebased around bug 701634 for the beta uplift.
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch Patch for betaSplinter Review
Ah, sorry about that. I'd be landing this now except the tree just closed...
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: 1122524
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?
You shouldn't be pressing buttons just because they're there.  Disabling IndexedDB is not a supported configuration.
Flags: qe-verify-
Duplicate of this bug: 1124742
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.
(In reply to o2627091@rtrtr.com 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.
Flags: needinfo?(florin.mezei)
(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.
Flags: needinfo?(florin.mezei) → needinfo?(gijskruitbosch+bugs)
Flags: qe-verify- → qe-verify+
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Status: RESOLVED → VERIFIED
See Also: → 1432994
You need to log in before you can comment on or make changes to this bug.