1197346 - Provide access to storage API from content scripts

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

According to the documentation it should be supported.
https://developer.chrome.com/extensions/storage#property-local

Comment 1

[u529383]

It should be working but it appears that it is not. Tested with permissions ["storage"] set in the manifest and tried to make use of chrome.storage.local in the content script, throws the following error in (tested in 44.0a1 (2015-10-09)):

chrome.storage is undefined

Comment 2

[Dan Callahan [:callahad]]

Sounds similar to Bug 1208761; we have successfully implemented APIs, but not exposed them to content scripts, when they should be according to https://developer.chrome.com/extensions/content_scripts

Comment 3

[jshackles]

Has there been any update on this?  My extension Enhanced Steam (http://www.EnhancedSteam.com) relies heavily on the chrome.storage API access in content scripts and I am unable to proceed with porting to WebExtensions until this bug is resolved.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll take a look at this. It shouldn't be too hard.

Comment 5

[ncla]

Same as jshackles, my extension relies on chrome.storage being available from content script. This is a major roadblock for even running the extension as my it needs user settings from local storage in content scripts. 46.0a1 still throws 'chrome.storage is undefined' in the browser console.

Comment 6

[amaslo]

@ncla (In reply to ncla from comment #5)
> Same as jshackles, my extension relies on chrome.storage being available
> from content script. This is a major roadblock for even running the
> extension as my it needs user settings from local storage in content
> scripts. 46.0a1 still throws 'chrome.storage is undefined' in the browser
> console.

I agree that this is a much needed feature, yet it doesn't have to stop you. In my extension I send a message from content to background script requesting a specific storage key, returning the retrieved value in response. This complicates the code, sure, but works till access from content script is in place.

I hope this helps.

Comment 7

[ncla]

(In reply to amaslo from comment #6)

Yes, I have thought about doing that, but I am mainly wondering if we developers can expect for this to be fixed/implemented, or do we have to implement this workaround in our extensions?

Comment 8

[Andy McKay]

(In reply to ncla from comment #7)
> (In reply to amaslo from comment #6)
> 
> Yes, I have thought about doing that, but I am mainly wondering if we
> developers can expect for this to be fixed/implemented, or do we have to
> implement this workaround in our extensions?

It might be easier to help implement the API rather than the workarounds.

Comment 9

[Steve Sobel]

this bug is flagged "blocking-webextensions" but is still marked as "new" - I wanted to ask, with webextensions coming very soon, if this is in the works at all?

Comment 10

[Andy McKay]

(In reply to Steve Sobel from comment #9)
> this bug is flagged "blocking-webextensions" but is still marked as "new" -
> I wanted to ask, with webextensions coming very soon, if this is in the
> works at all?

Hoping a contributor will work on it.

Comment 11

[kein-of]

Would be really nice if someone took care of it.

Comment 12

[Tom S [:evilpie]]

Bill did you sketch something out for this already?

Comment 13

[João Augusto]

There is also a need to provide access to the storage API from the options page.

Comment 14

[Andy McKay]

(In reply to João Augusto from comment #13)
> There is also a need to provide access to the storage API from the options
> page.

Please file a seperate bug for that.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename ExtensionPage to ExtensionContext

This just makes things a bit more uniform.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: synchronously load schema JSON

This is unfortunate, but I don't see a way around it. We now need to load storage.json for content scripts, and we have to do it before we've actually loaded any web content. Otherwise the extension is liable to miss activity.

This seems bad, but its already the way that we load message manager frame scripts as well as scripts loaded via the subscript loader (including WebExtension content scripts).

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: add callFunctionNoReturn hook

This adds a separate schema wrapperFunc that's used when calling a function that's not async and not declared to return anything. In the main patch, this allows me to call these functions remotely even though they don't have callbacks, while still throwing an error if you try to call a function that's supposed to return something.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: allow storage.* and test.* to be used from content scripts

This is the main patch. I tried different ways of using MessageChannel to communicate between the processes, but in the end I just found it easier to use the message manager directly. MessageChannel would have sort of worked for function calls (where you need to run the callback when it's done), but not really for event listeners. And I actually found it a little easier to do function calls using the message manager since they're pretty simple.

Anyway, suggestions welcome about better ways to do this. I probably need more tests, but I'll get to that soon.

Comment 19

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #16)
> This is unfortunate, but I don't see a way around it. We now need to load
> storage.json for content scripts, and we have to do it before we've actually
> loaded any web content. Otherwise the extension is liable to miss activity.

Can we add the parsed JSON to the initial process data after we load it in the parent?

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8738332 [details] [diff] [review]
rename ExtensionPage to ExtensionContext

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

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +503,5 @@
>  // }).api()
>  //
>  // The result is an object with addListener, removeListener, and
>  // hasListener methods. |context| is an add-on scope (either an
> +// ExtensionContext in the chrome process or ExtensionContext in a

This wording is a bit confusing now. Maybe reword to make it clear that they're different ExtensionContexts?

Comment 21

[Kris Maglione [:kmag]]

I'm on a train without working power outlets, so I'm going to have to finish reviewing the rest tomorrow.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #19)
> (In reply to Bill McCloskey (:billm) from comment #16)
> > This is unfortunate, but I don't see a way around it. We now need to load
> > storage.json for content scripts, and we have to do it before we've actually
> > loaded any web content. Otherwise the extension is liable to miss activity.
> 
> Can we add the parsed JSON to the initial process data after we load it in
> the parent?

Yeah, that's a good idea. I'll try that.

In general I'm also worried about this happening in the parent process. We need to make sure that security extensions, for example, won't miss loads that happen very early in startup. But that's a separate issue.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: allow storage.* and test.* to be used from content scripts, v2

This eliminates the need for the sync JSON load.

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8738336 [details] [diff] [review]
add callFunctionNoReturn hook

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

::: toolkit/components/extensions/Extension.jsm
@@ +382,5 @@
>              return findPath(path)[name](...args);
>            },
>  
> +          callFunctionNoReturn(path, name, args) {
> +            return findPath(path)[name](...args);

This probably shouldn't explicitly return.

::: toolkit/components/extensions/Schemas.jsm
@@ +1001,5 @@
> +    } else if (!this.returns) {
> +      stub = (...args) => {
> +        this.checkDeprecated(context);
> +        let actuals = this.checkParameters(args, context);
> +        return context.callFunctionNoReturn(path, name, actuals);

This probably shouldn't explicitly return either.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8738736 [details] [diff] [review]
allow storage.* and test.* to be used from content scripts, v2

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

I like this. Thanks

Sorry it took so long to review. I've spent so much time traveling and reviewing patches over the past couple of weeks that I've barely had time to get any other work done.

::: toolkit/components/extensions/Extension.jsm
@@ +320,5 @@
> +    this.listenerProxies = new Map();
> +  }
> +
> +  get cloneScope() {
> +    return global;

Would it be better to use a sandbox with the same principal as we have on the other side? I'm not sure what the exact implications would be, but at the very least it would give us Error objects without internal frames in their stack traces.

@@ +324,5 @@
> +    return global;
> +  }
> +
> +  get principal() {
> +    return this.extension.createPrincipal(this.uri);

Would it be worth it to just structured clone the principal from the other side?

@@ +420,5 @@
> +    let args = data.args;
> +    if (data.callId) {
> +      args = data.args.concat(callback);
> +    }
> +    findPathInObject(context.apiObj, data.path)[data.name](...args);

We should reply with an error if this throws, for whatever reason.

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +1095,5 @@
> +
> +  removeListener(path, name, listener) {
> +    let ref = path.concat(name).join(".");
> +    let set = this.listeners.get(ref) || new Set();
> +    if (!set.has(listener)) {

Is this necessary? The function should work the same without it, I think.

::: toolkit/components/extensions/test/mochitest/test_ext_storage_content.html
@@ +9,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body>
> +
> +<script type="application/javascript;version=1.8">

Please remove ";version=1.8"

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #25)
> ::: toolkit/components/extensions/Extension.jsm
> @@ +320,5 @@
> > +    this.listenerProxies = new Map();
> > +  }
> > +
> > +  get cloneScope() {
> > +    return global;
> 
> Would it be better to use a sandbox with the same principal as we have on
> the other side? I'm not sure what the exact implications would be, but at
> the very least it would give us Error objects without internal frames in
> their stack traces.

This seems fine but it introduced a problem. If the global for the ProxyContext is a sandbox, then that sandbox is used here:
http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionStorage.jsm#81

Apparently Cu.Sandbox does not accept a Sandbox as its first argument. Do we need this sandbox here for sanitizing? Why wouldn't global.JSON work just as well? All of our globals should be content-privileged, so I think we should always have an Xray wrapper around the global. (Also, Cu.Sandbox is fairly expensive, so we shouldn't be doing it so often.)

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

See comment 26.

Comment 28

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #26)
> Apparently Cu.Sandbox does not accept a Sandbox as its first argument. Do we
> need this sandbox here for sanitizing? Why wouldn't global.JSON work just as
> well? All of our globals should be content-privileged, so I think we should
> always have an Xray wrapper around the global.

My concern was that an extension could pass an object belonging to a
compartment it didn't have privileges for, and we'd give it access to
serialized properties it shouldn't be able to read:

    browser.storage.set("data", {window: iframe.contentWindow});

I think I may have been slightly too paranoid, though, since exportFunction
should prevent arguments from a different compartment, and it looks X-ray
wrappers now prevent access to properties that belong to a different
compartment too.

> (Also, Cu.Sandbox is fairly expensive, so we shouldn't be doing it so
> often.)

Yeah, I was working under the assumption that `storage.set` wouldn't be
happening that often, but I suppose in some cases it might.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83e84e280fe7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c6d8e1ab937
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce33bb34122

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83e84e280fe7
https://hg.mozilla.org/mozilla-central/rev/7c6d8e1ab937
https://hg.mozilla.org/mozilla-central/rev/2ce33bb34122

Comment 31

[stephen]

has this bug regressed? i'm testing using chrome.storage.local in a content script in the latest firefox nightly (51.0a1 (2016-08-02)), and im getting the error message "chrome.storage is undefined"

Comment 32

[Andrew Swan [:aswan]]

We have tests that check this and they are passing, does your extension include the storage permission in the manifest?