1192643 - window.indexedDB throws when dom.indexedDB.enabled=false

Status: RESOLVED WORKSFORME

(Core :: Storage: IndexedDB, defect, P3)

Description

[al_9x]

InvalidStateError: A mutation operation was attempted on a database that did not allow mutations.

When a particular API is disabled, it should behave as if it was never added (returning undefined), because that's how feature detection is done (e.g. http://modernizr.com/downloads/modernizr-2.8.2.js)

throwing, breaks feature detection and consequently breaks sites.

Comment 1

[Mark Straver]

I've confirmed this.

Practical site where this breaks: www.canadapost.ca track&trace. Diabling indexedDB (disabling offline storage for all sites) there makes modernizr throw the mentioned InvalidStateError, breaking feature detection and init, making the modernizr object unavailable later on, which completely breaks tracking.

InvalidStateError: A mutation operation was attempted on a database that did not allow mutations.modernizr.js:8
ReferenceError: Modernizr is not defined[Learn More]search_autocomplete.js:137:1
ReferenceError: Modernizr is not defined[Learn More]findByTrackNumber:303:8

Comment 2

[duzopoh]

For the www.canadapost.ca site problem, there's a discussion in Palemoon forum: https://forum.palemoon.org/viewtopic.php?t=13565 

The developer states this, referencing this bug report:

"EDIT2: This is bug #1192643 (blocking for all sites disables IndexedDB (sets dom.indexedDB.enabled to false)) -> indexedDB throws because it's disabled (it shouldn't throw, that's a bug) -> modernizr fails to initialize -> tracking fails.

EDIT3: The fix for this is relatively easy, actually - just don't throw, but return undefined/null instead. That way, even when indexedDB is switched off as part of globally denying offline storage (it sort of makes sense to do that), the detection of the API being available or not doesn't error out but acts as if it was never there to begin with, which makes feature detection as it's supposed to be done happy.

Fix here: https://github.com/MoonchildProductions/Pale-Moon/commit/84d8514ae8c00b860ae823f915d40973791175b0

Comment 3

[duzopoh]

I have seen this problem with many sites since I two days ago turned the dom.indexedDB.enabled pref to false.

This problem breaks sites that use the Modernizr library, and even sites that don't even want to store IndexedDB.

Here's few:

- https://www.wunderground.com/
- http://www.xe.com/currencyconverter/

--------

I don't know if it's related but 2 days ago I filed another IndexedDB bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1323974 Even if the dom.indexedDB.enabled pref is set to false, some sites still save data to profile\storage\* folders.

Comment 4

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1192643-Set-indexedDB-to-null-when-dom.indexeddb.patch

Here's a patch inspired by the one in comment 2. It includes a simple mochitest to confirm the change works properly. I also confirmed this patch fixes breakage observed on Twitter when dom.indexedDB.enabled = false.

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85562b77eea7c8a5d991dabd4e4b5657319229b6&selectedJob=76252012

Comment 5

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

Comment on attachment 8836214 [details] [diff] [review]
0001-Bug-1192643-Set-indexedDB-to-null-when-dom.indexeddb.patch

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

-> janv

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

If we're going to make a change like this, we should probably coordinate across all APIs that use this idiom and send an "intent to ship" to our various lists beforehand.  It may also entail coordination with feature detecting libraries out there.  It's probably appropriate to find/create a meta bug about the throwing idiom.

Specifically, for unclear (to me) reasons it seems like we've standardized on throwing exceptions when disabling functionality by preference.  For example, for the 3rd party cookies preference[1] which doubles as the "3rd party iframe storage" pref, the DOM Cache API interfaces start throwing exceptions everywhere where they previously would return promises, which is weird.  (It wouldn't totally surprise me if the issue is that we assumed JS code would sniff Firefox and its version and then make assumptions about available APIs.  Or that checking for the existence of an API, but not using the API, was part of sniffing for other functionality.).

1: https://support.mozilla.org/t5/Cookies-and-cache/Disable-third-party-cookies-in-Firefox-to-stop-some-types-of/ta-p/1895

Comment 7

[Jan Varga [:janv]]

I agree with Andrew, we can't just land this and expect that everyone will be happy.

Comment 8

[Jan Varga [:janv]]

Comment on attachment 8836214 [details] [diff] [review]
0001-Bug-1192643-Set-indexedDB-to-null-when-dom.indexeddb.patch

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

Even when you address the comments, this can't land without the coordination Andrew described in comment 6.

::: dom/indexedDB/test/test_disable_indexeddb.html
@@ +1,1 @@
> +<!DOCTYPE html>

test_indexeddb_pref.html might be a better name for this test

Should we test the pref in xpcshell too ?

We also mention a license here, you can use test_add_put.html as a template.

@@ +10,5 @@
> +</head>
> +
> +<body>
> +  <script>
> +   add_task(function* () {

Why this can't use the generators like other tests do ?
If we decide to replace generators then we will do it at once.

@@ +11,5 @@
> +
> +<body>
> +  <script>
> +   add_task(function* () {
> +       for (let enable of [false, true]) {

Why is this using 4 char indentation ?

Comment 9

[Tom Ritter [:tjr]]

P1 if Arthur wants to work on it for 60; otherwise P2 probably (or already fixed) - he's going to dig in.

Comment 10

[Arthur Edelstein [:arthur]]

Tor Browser no longer sets dom.indexeddb.enabled to false. Instead, we rely on the fact that indexedDB is automatically disabled in Private Browsing windows and first-party isolated in normal windows. (See https://trac.torproject.org/23745.)

I'm not sure if there is still interest in fixing this bug, but I don't need it to have a high priority.

Comment 11

[Simon Giesecke [:sg] [he/him]]

Since the pref no longer exists, this appears obsolete. Can this be closed?

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Yes, marking WORKSFORME since bug 1488583 mooted this but this was a legit bug.