Closed Bug 1197346 Opened 9 years ago Closed 8 years ago

Provide access to storage API from content scripts

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
45.3 - Dec 14
Tracking Status
firefox48 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [storage]triaged)

Attachments

(3 files, 2 obsolete files)

Whiteboard: [storage]
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
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
Blocks: webext
Flags: blocking-webextensions+
Blocks: 1213475
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.
I'll take a look at this. It shouldn't be too hard.
Assignee: nobody → wmccloskey
Iteration: --- → 45.2 - Nov 30
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
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.
@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.
(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?
(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.
Whiteboard: [storage] → [storage]triaged
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?
(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.
Would be really nice if someone took care of it.
Flags: blocking-webextensions+
Bill did you sketch something out for this already?
There is also a need to provide access to the storage API from the options page.
(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.
This just makes things a bit more uniform.
Attachment #8738332 - Flags: review?(kmaglione+bmo)
Attached patch synchronously load schema JSON (obsolete) — Splinter Review
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).
Attachment #8738333 - Flags: review?(kmaglione+bmo)
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.
Attachment #8738336 - Flags: review?(kmaglione+bmo)
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.
Attachment #8738339 - Flags: review?(kmaglione+bmo)
(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 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?
Attachment #8738332 - Flags: review?(kmaglione+bmo) → review+
I'm on a train without working power outlets, so I'm going to have to finish reviewing the rest tomorrow.
(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.
Attachment #8738333 - Attachment is obsolete: true
Attachment #8738333 - Flags: review?(kmaglione+bmo)
This eliminates the need for the sync JSON load.
Attachment #8738339 - Attachment is obsolete: true
Attachment #8738339 - Flags: review?(kmaglione+bmo)
Attachment #8738736 - Flags: review?(kmaglione+bmo)
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.
Attachment #8738336 - Flags: review?(kmaglione+bmo) → review+
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"
Attachment #8738736 - Flags: review?(kmaglione+bmo) → review+
(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.)
See comment 26.
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
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"
We have tests that check this and they are passing, does your extension include the storage permission in the manifest?
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.