Closed
Bug 1079355
Opened 7 years ago
Closed 6 years ago
Make 'dom.indexeddb.enabled' pref only affect content
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
VERIFIED
FIXED
mozilla37
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(2 files)
4.29 KB,
patch
|
bent.mozilla
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.15 KB,
patch
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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: 7 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 3•7 years ago
|
||
(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)
Comment 4•7 years ago
|
||
(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 → ---
Duplicate of this bug: 1082338
Making it content-only seems ok to me.
Flags: needinfo?(bent.mozilla)
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 | ||
Comment 10•6 years ago
|
||
remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3acbc7fc8852 Something like this?
Attachment #8544250 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 11•6 years ago
|
||
(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.
Comment 14•6 years ago
|
||
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?
Assignee | ||
Comment 15•6 years ago
|
||
(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•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/15c6f78cd4ff
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 20•6 years ago
|
||
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?
Assignee | ||
Comment 21•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #20) > See bug 1122204 (duped > here) and bug . I meant to quote bug 1122030.
Updated•6 years ago
|
Updated•6 years ago
|
Attachment #8544250 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•6 years ago
|
||
Needs to be rebased around bug 701634 for the beta uplift.
status-firefox38:
fixed → ---
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 23•6 years ago
|
||
Ah, sorry about that. I'd be landing this now except the tree just closed...
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 25•6 years ago
|
||
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?
Duplicate of this bug: 1122861
You shouldn't be pressing buttons just because they're there. Disabling IndexedDB is not a supported configuration.
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Duplicate of this bug: 1123423
Assignee | ||
Comment 31•6 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/97b34f0b9946
Updated•6 years ago
|
Flags: qe-verify-
Updated•6 years ago
|
Blocks: sdk-fennec
Comment 33•6 years ago
|
||
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.
Assignee | ||
Comment 34•6 years ago
|
||
(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)
Comment 35•6 years ago
|
||
(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)
Updated•6 years ago
|
Flags: qe-verify- → qe-verify+
Assignee | ||
Comment 36•6 years ago
|
||
(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)
Comment 37•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•