Closed Bug 1331618 Opened 7 years ago Closed 7 years ago

An extension with the unlimitedStorage permission should be able to bypass the IndexedDB Site Permission popup

Categories

(WebExtensions :: General, defect, P2)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.
59 bytes, text/x-review-board-request
aswan
: review+
Details 
As discussed in the comments related to Bug 1323414, to be able to use the IndexedDB File and MutableFile instances to store files bigger than 2Gb, the database needs to be opened using the "persistent" storage mode:

    const dbReq = indexedDB.open("tempFilesDB",  {
      version: 1, storage: "persistent"
    });

(more info are available in the following MDN doc page:
- https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Browser_storage_limits_and_eviction_criteria#Firefox_specifics)

Currently, when an extension opens a IndexedDB with the storage type "persistent", a Site Permission popup is triggered (which works from an extension tab, but it doesn't from a background page), which asks the user if it wants to allow the extension to store data.

Once we have the permission visible during the addon installation (Bug 1308309, Bug 1308295), it can be reasonable to "pre" allow the "IndexedDB" permission when the addon include the "unlimitedStorage" permission:

the user is supposed to have already evaluated the addon permission on its installation, and in that case he doesn't want/need to allow it again explicitly when the IndexedDB is opened for the first time.
Blocks: 1323414
Depends on: 1308309, 1308295
Updating the permissions dependency, but note that Chrome doesn't include this permission in its installation prompts...
Depends on: webext-permissions
No longer depends on: 1308295, 1308309
(In reply to Andrew Swan [:aswan] from comment #1)
> Updating the permissions dependency, 

Thanks Andrew, I was going exactly to ask you for that!

> but note that Chrome doesn't include this permission in its installation prompts...

I just tried what is going to happen on Firefox once the "permission in the WebExtensions installation prompt" is enabled and it looks like the installation prompt is going to include it (currently with the "localized message for unlimitedStorage permission" placeholder).

do you see any reason to prefer to behave like Chrome and not including it on Firefox? 
or, in other word, what do you think of the issue and the approach described in Comment 0? can it be a reasonable behavior?
Flags: needinfo?(aswan)
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

If we decide that the approach is reasonable, this is a patch with a proposed appraoch to pre-allow the indexedDB permission for the addon principal if the unlimitedStorage permission is enabled and a test case (the test case fails, with a short timeout, without the patch and it passes with the change applied) attached for a preliminary feedback on the possible implementation strategies.
Attachment #8827534 - Flags: feedback?(aswan)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
This bug has a patch on it a clear description, whereas bug 1282972 does not. I'd recommend we keep moving forward with this bug and if this resolves bug 1282972 as a dupe, so be it.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to Luca Greco [:rpl] from comment #3)
> > but note that Chrome doesn't include this permission in its installation prompts...
> 
> I just tried what is going to happen on Firefox once the "permission in the
> WebExtensions installation prompt" is enabled and it looks like the
> installation prompt is going to include it (currently with the "localized
> message for unlimitedStorage permission" placeholder).
> 
> do you see any reason to prefer to behave like Chrome and not including it
> on Firefox? 
> or, in other word, what do you think of the issue and the approach described
> in Comment 0? can it be a reasonable behavior?

Note that the current behavior is a placeholder, in bug 1316996 we will land the actual text for those permissions, including no text for some permissions that we choose not to include in permissions prompts.  Since we didn't previously implement this permission (unlimitedStorage), I don't think it was included in the drafting of that text.  I don't have a strong opinion one way or the other but Scott has been writing that text.  Scott, this is a new permission being added, the name pretty much describes what it does.  What, if any, text do you want to use for it?
Flags: needinfo?(aswan) → needinfo?(sdevaney)
To keep the messaging in line with the other permission APIs (bullet point style, no period), how about this?:

Provide unlimited storage of client-side data
Flags: needinfo?(sdevaney)
(In reply to sdevaney from comment #8)
> To keep the messaging in line with the other permission APIs (bullet point
> style, no period), how about this?:
> 
> Provide unlimited storage of client-side data

Sounds good to me, it is more or less the same message that the SitePermission popup contains (it is probably even better):

https://dxr.mozilla.org/mozilla-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/browser/locales/en-US/chrome/browser/browser.properties#326-327
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

Hi Jan,

as described in Comment 0, the goal of the proposed patch attached here is to bypass the Site Permissions popup that is opened when an extension opens an IndexedDB db with the "persistent" storage mode for the first time.

The rationale is that the user is going to explicitly allow the addon to store data locally when he reviews the addon permissions during the addon installation and so he doesn't need to allow it again on the Site Permissions popup (only if the WebExtension has the "unlimitedStorage" permission).

I followed the code in "dom/indexedDB/PermissionRequestBase.cpp", "browser/base/content/browser.js" and "	mobile/android/chrome/content/browser.js" 
and, based on that code, I prepared this proposed patch which adds the "indexedDB" permission for the principal related to the WebExtensions Addon, if the addon has the "unlimitedStorage" permission in its manifest.json file when it is started (it also remove the permission if the same addon doesn't have the "unlimitedStorage" permission and the permission wasn't assigned through the user interfaction).

If you have the opportunity to briefly look at it, I'd like to get your feedback, and confirm that I'm reading  correctly the code linked above.

Thanks in advance for you help.
Attachment #8827534 - Flags: feedback?(jvarga)
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review106664

Looks good to me.

::: toolkit/components/extensions/Extension.jsm:871
(Diff revision 1)
> +      // that the permission hasn't been selected manually by the user.
> +      Services.perms.addFromPrincipal(principal, "WebExtensions-unlimitedStorage",
> +                                      Services.perms.ALLOW_ACTION);
> +      Services.perms.addFromPrincipal(principal, "indexedDB", Services.perms.ALLOW_ACTION);
> +    } else if (Services.perms.testPermissionFromPrincipal(principal,
> +                                                          "WebExtensions-unlimitedStorage")) {

Just curious, when does this happen ? I mean that there's WebExtensions-unlimitedStorage but no unlimitedStorage.
Attachment #8827534 - Flags: feedback?(jvarga) → feedback+
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review106682

::: toolkit/components/extensions/Extension.jsm:871
(Diff revision 1)
> +      // that the permission hasn't been selected manually by the user.
> +      Services.perms.addFromPrincipal(principal, "WebExtensions-unlimitedStorage",
> +                                      Services.perms.ALLOW_ACTION);
> +      Services.perms.addFromPrincipal(principal, "indexedDB", Services.perms.ALLOW_ACTION);
> +    } else if (Services.perms.testPermissionFromPrincipal(principal,
> +                                                          "WebExtensions-unlimitedStorage")) {

it is probably a more common scenario during the development of a WebExtension, e.g.:

- the addon is installed as a temporary installed addon 
- the addon has been executed once with the "unlimitedStorage" permission
- then we remove the "unlimitedStorage" permission and reload the addon

once the addon has been reloaded without the permission, the Site Permission popup is supposed to be opened again (so that the user can decide to allow or disallow the "persistent" indexedDB storage on it).

We check the "fake WebExtension-unlimitedStorage site permission" to be sure that we are not going to reset the "indexedDB" permission if it has been allowed manually by the user.

(even if I doubt that a real world addon installed from a user is going to remove the "unlimitedStorage" permission in an update, once it has been added in a released version, the above behavior should be reasonable even in that case)
Oh I see, thanks for the explanation.
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review107216

I don't really understand the test.  Is the idea that without this change a permission prompt comes up and the open doesn't succeed so the timer kicks in and fails the test?  In that case, it is pretty simple to add a "popupshown" listener with PopupNotifications.jsm and use that instead of a timer for that failure case.

::: browser/components/extensions/test/browser/browser_ext_indexedDB_unlimitedStorage.js:12
(Diff revision 1)
> +      const timeoutId = setTimeout(() => {
> +        browser.test.fail("Timeout opening persistent db from background page");
> +        browser.test.notifyFail("indexeddb-unlimited-storage-done");
> +      }, 2000);

Another equivalent way to write this that might be clearer to read is create a Promise for the actual test and then use `Promise.race()` to race the test against the 2000msec timer.

::: toolkit/components/extensions/Extension.jsm:861
(Diff revision 1)
>        return super_(locale);
>      }.bind(this));
>    }
>  
> +  initUnlimitedStoragePermission() {
> +    let principal = Services.scriptSecurityManager

We have the same logic in onUninstall to clear out IndexedDB, maybe create a lazy getter for this so we're not repeating it unnecessarily?

::: toolkit/components/extensions/Extension.jsm:869
(Diff revision 1)
> +    if (this.hasPermission("unlimitedStorage")) {
> +      // Set the indexedDB permission and a custom "WebExtensions-unlimitedStorage" to remember
> +      // that the permission hasn't been selected manually by the user.
> +      Services.perms.addFromPrincipal(principal, "WebExtensions-unlimitedStorage",
> +                                      Services.perms.ALLOW_ACTION);
> +      Services.perms.addFromPrincipal(principal, "indexedDB", Services.perms.ALLOW_ACTION);

When does this get cleaned up?  Seems like it should certainly happen when the extension is uninstalled, and probably even if it is disabled?

::: toolkit/components/extensions/schemas/manifest.json:218
(Diff revision 1)
>                "alarms",
>                "clipboardWrite",
>                "idle",
>                "notifications",
> -              "storage"
> +              "storage",
> +              "unlimitedStorage"

Don't forget to land a string for this if we want to prompt for it
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

A few observations but overall looks good to me.
Attachment #8827534 - Flags: feedback?(aswan) → feedback+
Assignee: nobody → lgreco
Priority: -- → P2
Whiteboard: triaged
(In reply to Luca Greco [:rpl] from comment #0)
> As discussed in the comments related to Bug 1323414, to be able to use the
> IndexedDB File and MutableFile instances to store files bigger than 2Gb

Are these standardized? If not, are we exposing their existence with whatever we're doing here for Web Extensions?
Flags: needinfo?(lgreco)
Flags: needinfo?(jvarga)
Hi Andrew,
follows some additional details related to your questions:

(In reply to Andrew Overholt [:overholt] from comment #16)
> (In reply to Luca Greco [:rpl] from comment #0)
> > As discussed in the comments related to Bug 1323414, to be able to use the
> > IndexedDB File and MutableFile instances to store files bigger than 2Gb
> 
> Are these standardized? 

The MutableFile is not a standardized feature (and also `indexedDB.open(name, { version: n, storage: "default/temporary/persistent" })` is not a standard syntax).

> If not, are we exposing their existence with whatever we're doing here for Web Extensions?

Both the MutableFile and the indexedDB.open `storage` parameter are already exposed to the regular webpages (and, by extent, also to any WebExtensions page, e.g. background page, popups, tab etc.), with lower quota limits by default. 

The maximum quota limits are already available to regular webpages (and WebExtensions pages) by using "persistent" as the requested IndexedDB storage mode, and by allowing the request through an explicit site permission prompt.

This issue is about allow a WebExtension with an "unlimitedStorage" permission to use the "persistent" mode without requiring an additional user action (the rationale is that the user has already allowed the extension to store data on the client through the "WebExtension permissions prompt")

Let me know if the above additional details do not answer your questions.
Flags: needinfo?(lgreco)
Hi Luca,

(In reply to Luca Greco [:rpl] from comment #17)
> 
> (In reply to Andrew Overholt [:overholt] from comment #16)
> > (In reply to Luca Greco [:rpl] from comment #0)
> > > As discussed in the comments related to Bug 1323414, to be able to use the
> > > IndexedDB File and MutableFile instances to store files bigger than 2Gb
> > 
> > Are these standardized? 
> 
> The MutableFile is not a standardized feature (and also
> `indexedDB.open(name, { version: n, storage: "default/temporary/persistent"
> })` is not a standard syntax).
> 
> > If not, are we exposing their existence with whatever we're doing here for Web Extensions?
> 
> Both the MutableFile and the indexedDB.open `storage` parameter are already
> exposed to the regular webpages (and, by extent, also to any WebExtensions
> page, e.g. background page, popups, tab etc.), with lower quota limits by
> default. 

Hmm. I (I guess quite naively :) didn't realize WebExtensions in Firefox could just use whatever was exposed to regular content (I somehow thought they had some special clearly-defined limited set of APIs). We have bug 1254928 to stop shipping these non-standard IDB features.

> This issue is about allow a WebExtension with an "unlimitedStorage"
> permission to use the "persistent" mode without requiring an additional user
> action (the rationale is that the user has already allowed the extension to
> store data on the client through the "WebExtension permissions prompt")

This makes perfect sense. I'm just worried about locking ourselves into maintaining these non-standard things once people start using them. If I were to be a Web Extension developer wanting to insert data into the middle of a file on disk and I wanted to use MutableFile in Firefox, what would I use in Chrome? Is the idea that browsers have their own unique APIs and capabilities in Web Extensions? For some reason I was thinking it was intended to be pretty simple to take Web Extensions from browser to browser.

Thanks for the information!
Flags: needinfo?(lgreco)
Hi Andrew,

> Hmm. I (I guess quite naively :) didn't realize WebExtensions in Firefox
> could just use whatever was exposed to regular content (I somehow thought
> they had some special clearly-defined limited set of APIs). We have 
> bug 1254928 to stop shipping these non-standard IDB features.

Oh I see, and I'm very glad that we started this conversation, I completely
see the reasons behing Bug 1254928 and it is good that we are discussing
about its impact of the WebExtensions as well now.

I'm wondering: besides removing the non-standard MutableFile, is Bug 1254928 about removing the "storage" parameter as well?

By using the "persistent" storage mode, the File object saved in the IndexedDB are allowed to be bigger than 2Gb, which would be probably useful for some extensions even besides the MutableFile availability.

> This makes perfect sense. I'm just worried about locking ourselves into
> maintaining these non-standard things once people start using them. 

Yeah, it is completely understandable, and I definitely agree with this concern.

> If I were to be a Web Extension developer wanting to insert data into the
> middle of a file on disk and I wanted to use MutableFile in Firefox, what
> would I use in Chrome? Is the idea that browsers have their own unique
> APIs and capabilities in Web Extensions? For some reason I was thinking it
> was intended to be pretty simple to take Web Extensions from browser to
> browser.

Currently this is a tricky scenario to be implemented efficently (e.g. "without storing the entire blobs in memory") with a "cross-browsers strategy" (and we have probably much more addons that currently use direct filesystem access than chrome has, and so we are figuring out how to provide them as much as possible without breaking the "sandboxing principles" of the WebExtensions APIs)

To be able to store files in the "browser profile" (not directly shared with external applications), it seems to be currently possible:

- on both Chrome and Firefox, you can store File objects into IndexedDB (by retrieving the File lists by direct user interaction, e.g. drag and drop or an open files/directory dialog)
- on Chrome, the fileSystem API can be used to request a temporary or permanent filesystem storage and create/change files in it
- on Firefox, the IndexedDB and the MutableFile can be used to achieve the same (with a different API, but most of the same features)

I think that chrome doesn't provide to a chrome extension any arbitrary filesystem access (Chrome Apps used to have a chrome.fileSystem API which used to provide this kind of feature)
Flags: needinfo?(lgreco) → needinfo?(overholt)
(In reply to Luca Greco [:rpl] from comment #19)
> I'm wondering: besides removing the non-standard MutableFile, is Bug 1254928
> about removing the "storage" parameter as well?

Yeah, but only once persist() is fully working, bug 1287701.
However, we will keep the special persistent repository for internal use, but it won't be usable by IndexedDB anymore.
Flags: needinfo?(jvarga)
(In reply to Luca Greco [:rpl] from comment #19)
> > If I were to be a Web Extension developer wanting to insert data into the
> > middle of a file on disk and I wanted to use MutableFile in Firefox, what
> > would I use in Chrome? Is the idea that browsers have their own unique
> > APIs and capabilities in Web Extensions? For some reason I was thinking it
> > was intended to be pretty simple to take Web Extensions from browser to
> > browser.
> 
> Currently this is a tricky scenario to be implemented efficently (e.g.
> "without storing the entire blobs in memory") with a "cross-browsers
> strategy" (and we have probably much more addons that currently use direct
> filesystem access than chrome has, and so we are figuring out how to provide
> them as much as possible without breaking the "sandboxing principles" of the
> WebExtensions APIs)
> 
> To be able to store files in the "browser profile" (not directly shared with
> external applications), it seems to be currently possible:
> 
> - on both Chrome and Firefox, you can store File objects into IndexedDB (by
> retrieving the File lists by direct user interaction, e.g. drag and drop or
> an open files/directory dialog)
> - on Chrome, the fileSystem API can be used to request a temporary or
> permanent filesystem storage and create/change files in it

We had previously looked at this API and were considering the provider API for Firefox OS. Do you know if Chrome is going to keep shipping this API? Do we want to have something that lets Web Extensions be ported to Firefox easily?

> - on Firefox, the IndexedDB and the MutableFile can be used to achieve the
> same (with a different API, but most of the same features)

I wonder if we could somehow mask the use of MutableFile here while providing similar functionality.

> I think that chrome doesn't provide to a chrome extension any arbitrary
> filesystem access (Chrome Apps used to have a chrome.fileSystem API which
> used to provide this kind of feature)

Oh, given you say "used to" here maybe they won't continue with their FileSystem API for use by Web Extensions?
Flags: needinfo?(overholt) → needinfo?(lgreco)
webextensions: --- → +
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review107216

The main reason why I opted for the timeout is that (at least currently) the background page will never show any popup (currently there is anything that listen for the site permissions prompt requests originated from a background page, and it is also not completely clear what it should be the behavior when the request is originated from a background page, because the site permissions are currently always attached to a tab).

> Another equivalent way to write this that might be clearer to read is create a Promise for the actual test and then use `Promise.race()` to race the test against the 2000msec timer.

Yeah, using `Promise.race()` sounds good to me. 

I've reworked the test to use `Promise.race()` (and also extended the amount of behavior tested by the new test file)

> We have the same logic in onUninstall to clear out IndexedDB, maybe create a lazy getter for this so we're not repeating it unnecessarily?

I've opted to reuse this.principal (that has been introduced in the meantime on the extension object).

> When does this get cleaned up?  Seems like it should certainly happen when the extension is uninstalled, and probably even if it is disabled?

The updated patch clean up the site permissions in the following two scenarios:

- when an extension has been updated with a new version that does not require the "unlimitedStorage" permission anymore (unlikely but still possible ;-)) 
- when the extension is uninstalled (and the data has been removed as well)

I also added an additional test case and some assertions to check this behavior explicitly.

> Don't forget to land a string for this if we want to prompt for it

New permission description string added in the updated patch.
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review106664

> Just curious, when does this happen ? I mean that there's WebExtensions-unlimitedStorage but no unlimitedStorage.

This is going to happen when an extension is updated with a new version that doesn't require the "unlimitedStorage" anymore.

I'm setting (and then test and reset) the "custom" site permission to be able to detect when the site permissions has been set by the extension permissions vs. manually by the user.
Attachment #8827534 - Flags: review?(aswan)
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review154216

mostly nits, the only thing i'm really concerned about is nailing down the permission string.

::: browser/components/extensions/test/browser/browser-common.ini:141
(Diff revision 2)
>  [browser_ext_tabs_update.js]
>  [browser_ext_tabs_zoom.js]
>  [browser_ext_tabs_update_url.js]
>  [browser_ext_themes_icons.js]
>  [browser_ext_themes_validation.js]
> +[browser_ext_unlimitedStorage.js]

why does this need to be a browser test?  seems like it could be a chrome mochitest?

::: browser/components/extensions/test/browser/browser_ext_unlimitedStorage.js:69
(Diff revision 2)
> +        new Promise((resolve, reject) => {
> +          setTimeout(() => {
> +            reject(new Error("Timeout opening persistent db from background page"));
> +          }, PROMISE_RACE_TIMEOUT);
> +        }),
> +      ]).then(

With async this is typically written as:
```
try {
  await Promise.race(...);
  browser.test.notifyPass(...);
} catch (error) {
  browser.test.notifyFail(...);
}
```

::: browser/locales/en-US/chrome/browser/browser.properties:112
(Diff revision 2)
>  webextPerms.description.notifications=Display notifications to you
>  webextPerms.description.privacy=Read and modify privacy settings
>  webextPerms.description.sessions=Access recently closed tabs
>  webextPerms.description.tabs=Access browser tabs
>  webextPerms.description.topSites=Access browsing history
> +webextPerms.description.unlimitedStorage=Provide unlimited storage of client-side data

I love a good permission string bikeshed, so: the word provide here is wrong.  An extension that is granted this permission is allowed to use/consume storage, not provide it.

::: toolkit/components/extensions/Extension.jsm:1007
(Diff revision 2)
>    }
>  
> +  initUnlimitedStoragePermission() {
> +    const principal = this.principal;
> +
> +    // Check if the site permissions are already been set for the extension by the WebExtensions

nit: "permissions are" -> "permission has"

::: toolkit/components/extensions/Extension.jsm:1009
(Diff revision 2)
> +  initUnlimitedStoragePermission() {
> +    const principal = this.principal;
> +
> +    // Check if the site permissions are already been set for the extension by the WebExtensions
> +    // internals (instead of being manually allowed by the user).
> +    const hasUnlimitedStorageSitePermission = Services.perms.testPermissionFromPrincipal(

nit: since this whole function is about unlimitedStorage, this could be shortened to `hasSitePermission` if you like.
Attachment #8827534 - Flags: review?(aswan) → review+
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review154216

> why does this need to be a browser test?  seems like it could be a chrome mochitest?

yeah, actually, let me move this test at toolkit level and try to make it run also on android.

> With async this is typically written as:
> ```
> try {
>   await Promise.race(...);
>   browser.test.notifyPass(...);
> } catch (error) {
>   browser.test.notifyFail(...);
> }
> ```

true, I totally forgot to rewrite it into that form when I turned it into an async function.

> I love a good permission string bikeshed, so: the word provide here is wrong.  An extension that is granted this permission is allowed to use/consume storage, not provide it.

Sounds good to me, I preferred to start from the one that was in the original document related to the permissions strings, but we can agree on a better one.
Hi Scott,
we are discussing about how to improve the wording in the unlimitedStorage permission string (see Comment 27).

How about something around the following?

- Store unlimited amount of client-side data
- Store unlimited amount of data on disk
- Store unlimited amount of data locally
Flags: needinfo?(sdevaney)
I talked about this with Scott face-to-face and he chooses "Store unlimited amount of client-side data"
Flags: needinfo?(sdevaney)
Comment on attachment 8827534 [details]
Bug 1331618 - allow persistent indexedDB on unlimitedStorage permission.

https://reviewboard.mozilla.org/r/105190/#review154216

> yeah, actually, let me move this test at toolkit level and try to make it run also on android.

In the updated patch, I rewrote these tests as plain mochitest, so that they also run on Android.

Thanks to this change I discovered that on Firefox for Android the "legacy indexedDB persistent storage mode" is only allowed
from chrome privileged code (and deprecated "open web apps"), while it raises a InvalidStateError when a
regular webpage (and a WebExtension page by extent), as it can be verified also by looking at Bug 1119462 Comment 0 and the following test:
http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/dom/indexedDB/test/test_persistenceType.html#36-46

For the above reason, I splitted the test file and set a `skip-if = os == 'android'` on the test file related to the "unlimitedStorage permission and the legacy indexedDB persistent storage mode".
The main change from the version that Andrew has already given his r+ is "the tests splitted into two test files" (as described in Comment 32, also all the suggested changes have been applied) and the last pushes to try looks good (besides some oranges which doesn't look related):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ecf2a73e3878bb8d567a097c8a2335d22ca87bc
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4cd57518ef0d51b96e4a55fcff98a7b60bd6325

I'm going to move this patch in autoland, but I will also chat with aswan about the splitted tests and the issues on Android with the skipped test once he is back from PTO.
Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/6ce22ac573a7
allow persistent indexedDB on unlimitedStorage permission. r=aswan
https://hg.mozilla.org/mozilla-central/rev/6ce22ac573a7
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
The "unlimitedStorage" permission should be documented, not only with the feature introduced by this bug, but also:

- The "storage.local" documentation [1] should reference the "unlimitedStorage" permission, and state that the limit is currently not enforced (https://bugzilla.mozilla.org/show_bug.cgi?id=1322101#c2) but that it will be enforced in the future (bug 1282972). This is necessary because add-on devs who intend on using lots of storage would otherwise break if the permission were to be enforced without warning in the future. An example of a popular add-on is AdBlock(lastest version, v3.4.0), which allegedly stored 58MB of data with the browser.storage.local API.
Chrome enforces the following limit: https://developer.chrome.com/extensions/storage#property-local-QUOTA_BYTES

- The following pages assert that "unlimitedStorage" is not supported. That should be updated of course:
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/local
Keywords: dev-doc-needed
Looks good to me.
Flags: needinfo?(rob)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: