Closed Bug 1258139 Opened 4 years ago Closed 4 years ago

storage.local doesn't save data from popup script, only after browser restart extension gets access to saved data

Categories

(WebExtensions :: Untriaged, defect)

45 Branch
x86_64
Linux
defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: igaolong, Assigned: kmag)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160304163510

Steps to reproduce:

I've tried 45.0 on different machines and 47.0a1 trunk. Same results.
1. save data
browser.storage.local.set({
                    s_auth: {
                        id: data.id
                    }
                }, function() {
                    console.log(chrome.runtime.lastError); // null
                    console.log("Auth saved.");
                });
2. get data
browser.storage.local.get("s_auth", function(items) {
        console.log(items); // Object { s_auth: DeadObject }
    });
3. restart browser and saved data is accessible.


Actual results:

browser.storage.local.get gives Object { s_auth: DeadObject }

Log:

22:28:41.637 can't access dead object
runSafeSync() ExtensionUtils.jsm:56
bound runSafeSync() self-hosted:794
wrapPromise/<() ExtensionUtils.jsm:247
Handler.prototype.process() Promise-backend.js:937
this.PromiseWalker.walkerLoop() Promise-backend.js:816
bound () self-hosted:750
bound bound () self-hosted:750
1 ExtensionUtils.jsm:56:0

Security wrapper denied access to property "s_auth" on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ is being gradually removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported.

Also, when I'm trying to save data again, I get:

22:41:07.278 TypeError: browser is null1 nsLoginManagerPrompter.js:808:9
22:41:07.278 NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "browser is null" {file: "resource://gre/components/nsLoginManagerPrompter.js" line: 808}]'[JavaScript Error: "browser is null" {file: "resource://gre/components/nsLoginManagerPrompter.js" line: 808}]' when calling method: [nsILoginManagerPrompter::promptToSavePassword]1 LoginManagerParent.jsm:410:0



Expected results:

Saved data is accessible right after saving.
Summary: storage.local doesn't save data from popup script, only after restart extension gets access to saved data → storage.local doesn't save data from popup script, only after browser restart extension gets access to saved data
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
I suspect that popup scripts are not trusted and all storage operations should be done in background script.
Assignee: nobody → kmaglione+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8734169 [details]
MozReview Request: Bug 1258139: Part 2 - [webext] Fix dead wrapper issues in storage API. r?gabor

https://reviewboard.mozilla.org/r/42123/#review38723

::: toolkit/components/extensions/ExtensionStorage.jsm:81
(Diff revision 1)
>        AsyncShutdown.profileBeforeChange.removeBlocker(promise);
>      });
>    },
>  
>    set(extensionId, items) {
> +    items = Cu.cloneInto(items, global);

Conceptually this does the right thing. However what chromium does (according to the docs which does not mean a lot...): "Primitive values such as numbers will serialize as expected. Values with a typeof "object" and "function" will typically serialize to {}, with the exception of Array (serializes as expected), Date, and Regex (serialize using their String representation)."

cloneInto will throw for function properties, and we will store object values as JSON, no? Another interesting case are properties with getters (cloneInto will throw for these too, I have no idea what chrome does for them)

Finally the object or any of the property can be a proxy object... (do we care?)

If we want to copy chromium behavior we should just check the type of the properties, and if they are functions/objects and not arrays, could just use {} for them. And then we don't even need cloneInto. But that would suck imo and I'm not even sure the docs are correct. So instead we could just check the type of each properties and use {} for functions and cloneInto the object properties (that should work for Arrays too). I wish we had a cloneInto that ignores function arguments.

Anyway, let me know what you think, I can r+ this version as well if the decisions is that we want this behavior. But want to make sure that these side effects are clear. Btw It might make sense to flag properties to be cloned at scheme level and display it in the docs too (I don't know our current status at this area so this might be a dumb idea).
Attachment #8734169 - Flags: review?(gkrizsanits)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Comment on attachment 8734169 [details]
> MozReview Request: Bug 1258139: Part 2 - [webext] Fix dead wrapper issues in
> storage API. r?gabor
> 
> https://reviewboard.mozilla.org/r/42123/#review38723
> 
> ::: toolkit/components/extensions/ExtensionStorage.jsm:81
> (Diff revision 1)
> >        AsyncShutdown.profileBeforeChange.removeBlocker(promise);
> >      });
> >    },
> >  
> >    set(extensionId, items) {
> > +    items = Cu.cloneInto(items, global);
> 
> Conceptually this does the right thing. However what chromium does
> (according to the docs which does not mean a lot...): "Primitive values such
> as numbers will serialize as expected. Values with a typeof "object" and
> "function" will typically serialize to {}, with the exception of Array
> (serializes as expected), Date, and Regex (serialize using their String
> representation)."

Hmm... That's a good point. This also has implications for OOP add-ons, since
the arguments to these methods will have to be structured cloned across
processes.

> cloneInto will throw for function properties, and we will store object
> values as JSON, no? Another interesting case are properties with getters
> (cloneInto will throw for these too, I have no idea what chrome does for
> them)
> 
> Finally the object or any of the property can be a proxy object... (do we
> care?)

I suspect that Chrome just treats getters as ordinary properties, since that's
what it does elsewhere. I don't know what it does for Proxies.

> If we want to copy chromium behavior we should just check the type of the
> properties, and if they are functions/objects and not arrays, could just use
> {} for them. And then we don't even need cloneInto. But that would suck imo
> and I'm not even sure the docs are correct. So instead we could just check
> the type of each properties and use {} for functions and cloneInto the
> object properties (that should work for Arrays too). I wish we had a
> cloneInto that ignores function arguments.

The more I think about it, the more I think we should probably just use
JSON.stringify and then JSON.parse. We can use a replacer to handle Date and
Regex objects, and reject any other type of exotic object (including proxies).

And, now that I think about it some more, we should really probably use the
JSON object from the extension scope, so extensions can't use the storage API
to get access to properties of objects they don't have privileges to read.

> Btw It might make sense to flag properties to be cloned at scheme level and
> display it in the docs too (I don't know our current status at this area so
> this might be a dumb idea).

I think that once we have OOP add-ons, all properties will have to be cloned.
I think we're going to have to come up with some way to handle the ones that
can't, though. I'll give it some thought.
Comment on attachment 8734168 [details]
MozReview Request: Bug 1258139: Part 1 - [webext] Refactor storage API code. r?rpl

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42121/diff/1-2/
Comment on attachment 8734169 [details]
MozReview Request: Bug 1258139: Part 2 - [webext] Fix dead wrapper issues in storage API. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42123/diff/1-2/
Attachment #8734169 - Flags: review?(gkrizsanits)
Comment on attachment 8734168 [details]
MozReview Request: Bug 1258139: Part 1 - [webext] Refactor storage API code. r?rpl

https://reviewboard.mozilla.org/r/42121/#review38977
Attachment #8734168 - Flags: review?(lgreco) → review+
https://reviewboard.mozilla.org/r/42119/#review38981

::: toolkit/components/extensions/test/mochitest/test_ext_storage.html:125
(Diff revision 2)
>    }).then(() => {
>      return check("test-prop2", "value2");
>  
>    // Make sure we can store complex JSON data.
>    }).then(() => {
> -    return set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2]}});
> +    return storage.set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2],

How about test explicitly that properties which are created using custom constructors can be saved and retrieved correctly? (spoiler: it obviously works correctly, but it can be reasonable to test it anyhow) e.g.

    function CustomObject() {
      this.v = "custom prototype";
    }

    return storage.set({"test-prop1": {
      custom: new CustomObject(),
      ...
    }});
    

Nit, This line would be probably much more readable if formatted to span into multiple lines (which would make immediately clear which kind of properties we're testing), e.g.

    return storage.set({"test-prop1": {
      str: "hello",
      bool: true,
      undef: undefined,
      obj: {},
      arr: [1, 2],
      date: new Date(0),
      regexp: /regexp/,
      func: function func() {},
      custom: new CustomObject(),
      window,
    }});

::: toolkit/components/extensions/test/mochitest/test_ext_storage.html:128
(Diff revision 2)
>    // Make sure we can store complex JSON data.
>    }).then(() => {
> -    return set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2]}});
> +    return storage.set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2],
> +                                       date: new Date(0),
> +                                       regexp: /regexp/,
> +                                       func: function func() {}, window}});

This line is logging security errors in the Error console, they are silent errors and the tests work as expected (on a debug build they are sent to the terminal too), I'm not sure that it is something to be considered as an actual issue, but I prefer to leave at least a trace of it, and so I'm mentioning it here:

- on the func property:
  'Xraywrapper denied access to property "func" (reason: value is callable)'

::: toolkit/components/extensions/test/mochitest/test_ext_storage.html:130
(Diff revision 2)
> -    return set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2]}});
> +    return storage.set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2],
> +                                       date: new Date(0),
> +                                       regexp: /regexp/,
> +                                       func: function func() {}, window}});
>    }).then(() => {
> -    browser.test.assertEq(globalChanges["test-prop1"].oldValue, "value1", "oldValue correct");
> +    return storage.set({"test-prop2": function func() {}});

This line logs a security error similar to the above (but its error message lacks some of the informations because it hasn't a name):
  
"Security wrapper denied access to property (void 0) on privileged Javascript object..."
(In reply to Kris Maglione [:kmag] from comment #5)
> I suspect that Chrome just treats getters as ordinary properties, since
> that's
> what it does elsewhere. I don't know what it does for Proxies.
>
> ...
> 
> The more I think about it, the more I think we should probably just use
> JSON.stringify and then JSON.parse. We can use a replacer to handle Date and
> Regex objects, and reject any other type of exotic object (including
> proxies).

I did some test on Chrome, in particular I tested how Chrome behaves with
the window object (as in our test cases) and an object with a getter:

- window object:
  - on Chrome, all the window properties which are serializable are saved
  - on Firefox, its saved value will be undefined

- object with a getter
  - on Chrome, the object is saved and it will contains the getter property name with the getter result as a value
  - on Firefox, the getter properties are not saved

In my own opinion, our behavior is more reasonable (and safe), but I tried them explicitly so that we can add them to the our list of Chrome incompatibilities, in case some addon developers are leveraging the Chrome behavior.

(I have not tried with a Proxy object, because it seems that they are not currently supported on Chrome and so, at least, they cannot introduce an incompatibility)
(In reply to Luca Greco [:rpl] from comment #9)
> How about test explicitly that properties which are created using custom
> constructors can be saved and retrieved correctly? (spoiler: it obviously
> works correctly, but it can be reasonable to test it anyhow) e.g.

Honestly, I'm not sure what the correct behavior should be in this case, so
I'd rather not test for it, one way or the other.

> This line is logging security errors in the Error console, they are silent
> errors and the tests work as expected (on a debug build they are sent to the
> terminal too), I'm not sure that it is something to be considered as an
> actual issue, but I prefer to leave at least a trace of it, and so I'm
> mentioning it here:

Hm. I suppose I have to unwrap the value before I pass it to JSON.stringify,
then. I'm surprised it wasn't automatically unwrapped when I passed it to a
compartment with the same principal.

(In reply to Luca Greco [:rpl] from comment #10)
> - window object:
>   - on Chrome, all the window properties which are serializable are saved
>   - on Firefox, its saved value will be undefined

Yeah, that's how we'd normally behave with JSON.stringify, but I think it's
clearly the wrong behavior here, and the Chrome documentation suggests that
objects other than arrays, Dates, and RegExps should be ignored, in any case.

> - object with a getter
>   - on Chrome, the object is saved and it will contains the getter property
> name with the getter result as a value
>   - on Firefox, the getter properties are not saved

Hm. I guess that might be worth emulating. It would be easy enough.
Comment on attachment 8734169 [details]
MozReview Request: Bug 1258139: Part 2 - [webext] Fix dead wrapper issues in storage API. r?gabor

https://reviewboard.mozilla.org/r/42123/#review39461

Yeah, I think this looks good. One thing seems to be a bit unfortunate, that xrays over regular js object have this odd behavior that they don't just forbid to call methods on the object but also ignore them during property enumeration (with a warning on the console for the first time). Unfortunately the same goes for properties with getters/setters :(

I think that is the explanation for this:

(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Luca Greco [:rpl] from comment #9)
> > This line is logging security errors in the Error console, they are silent
> > errors and the tests work as expected (on a debug build they are sent to the
> > terminal too), I'm not sure that it is something to be considered as an
> > actual issue, but I prefer to leave at least a trace of it, and so I'm
> > mentioning it here:
> 
> Hm. I suppose I have to unwrap the value before I pass it to JSON.stringify,
> then. I'm surprised it wasn't automatically unwrapped when I passed it to a
> compartment with the same principal.


If you want to support getters and do something more fancy with methods maybe, what you can do is doing all the property enumeration and replacer work in a sandbox with the contentWindows principal and with wantsXray: false flag. I think this can be a followup so I r+ this one, it's up to you.

::: toolkit/components/extensions/ExtensionStorage.jsm:136
(Diff revision 2)
>      return promise.then(() => {
>        AsyncShutdown.profileBeforeChange.removeBlocker(promise);
>      });
>    },
>  
> -  set(extensionId, items) {
> +  set(extensionId, items, global) {

contentWindow/content/window would make more sense to me than global, but if it's a convention then global is fine...

::: toolkit/components/extensions/test/mochitest/test_ext_storage.html:125
(Diff revision 2)
> -    return storage.set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2]}});
> +    return storage.set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2],
> +                                       date: new Date(0),
> +                                       regexp: /regexp/,
> +                                       func: function func() {}, window}});
> +  }).then(() => {
> +    return storage.set({"test-prop2": function func() {}});

Format of this object seems a bit off...
Attachment #8734169 - Flags: review?(gkrizsanits) → review+
https://reviewboard.mozilla.org/r/42123/#review39461

I think it may be enough to just add `wantsXrays: false` to the current sandbox, since the enumeration should already be happening there anyway. I'll leave it for a follow-up, though. I'm not sure if that's really desirable...

> contentWindow/content/window would make more sense to me than global, but if it's a convention then global is fine...

I think `contentWindow` is more common, but when we add storage support to content scripts, the global will be a sandbox, so it might be a bit deceptive.
https://hg.mozilla.org/mozilla-central/rev/cf7a2d48ca5d
https://hg.mozilla.org/mozilla-central/rev/494289c72ba3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 1274911
Duplicate of this bug: 1280747
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.