Provide access to storage API from content scripts

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +
blocking-webextensions +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [storage]triaged)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

2 years ago
Whiteboard: [storage]

Comment 1

2 years ago
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

Updated

2 years ago
Blocks: 1214433

Updated

2 years ago
Flags: blocking-webextensions+
Blocks: 1213475

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
I'll take a look at this. It shouldn't be too hard.
Assignee: nobody → wmccloskey

Updated

2 years ago
Iteration: --- → 45.2 - Nov 30
(Assignee)

Updated

2 years ago
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14

Comment 5

a year ago
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

a year ago
@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

a year ago
(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.

Updated

a year ago
Whiteboard: [storage] → [storage]triaged

Comment 9

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 1246787

Comment 11

a year ago
Would be really nice if someone took care of it.

Updated

a year ago
Flags: blocking-webextensions+
Bill did you sketch something out for this already?

Comment 13

a year ago
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.
(Assignee)

Comment 15

a year ago
Created attachment 8738332 [details] [diff] [review]
rename ExtensionPage to ExtensionContext

This just makes things a bit more uniform.
Attachment #8738332 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 16

a year ago
Created attachment 8738333 [details] [diff] [review]
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).
Attachment #8738333 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 17

a year ago
Created attachment 8738336 [details] [diff] [review]
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.
Attachment #8738336 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 18

a year ago
Created attachment 8738339 [details] [diff] [review]
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.
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.
(Assignee)

Comment 22

a year ago
(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.
(Assignee)

Updated

a year ago
Attachment #8738333 - Attachment is obsolete: true
Attachment #8738333 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 23

a year ago
Created attachment 8738736 [details] [diff] [review]
allow storage.* and test.* to be used from content scripts, v2

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+
(Assignee)

Comment 26

a year ago
(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.)
(Assignee)

Comment 27

a year ago
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)

Comment 29

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83e84e280fe7
https://hg.mozilla.org/mozilla-central/rev/7c6d8e1ab937
https://hg.mozilla.org/mozilla-central/rev/2ce33bb34122
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 31

10 months ago
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

10 months ago
We have tests that check this and they are passing, does your extension include the storage permission in the manifest?
You need to log in before you can comment on or make changes to this bug.