callbacks passed to a cpow (like a DOM listener) throw "Permission denied to pass object to chrome" (e10s)

RESOLVED INCOMPLETE

Status

()

Firefox
Extension Compatibility
RESOLVED INCOMPLETE
4 years ago
2 months ago

People

(Reporter: zombie, Unassigned)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: CPOW, triaged)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8483174 [details]
e10s-cpow.zip

none of the shims listed in bug 1031609 comment 0 work in Jetpack addons when tabs.remote.autostart pref is set.

SDK creates Sandbox-es to load both the addon code and SDK modules, so it's possible somehow those compartments are not marked as "addon code", and thus interposition is not enabled.

attached is a STR addon that shows:
1) browser.contentWindow doesn't work, while contentWindowAsCPOW does
2) attaching a DOM event listener to a window CPOW throws when that listener should be invoked:
  "Permission denied to pass object to chrome" 


to run, execute this line from the addon dir:

  \addon-sdk\bin\cfx run -v -b \nightly\firefox.exe --e10s


(or install the xpi manually, and check the browser console)
(Reporter)

Comment 1

4 years ago
> none of the shims listed in bug 1031609 comment 0 work 

that should read "bug 1017320 comment 0"..


i just tested this with Nightly from 8/20 (before ..AsCPOW bug 1051017 landed), and both these things worked, but observer notification shim never did, so..

so my theory on reasons for breakage above might not be entirely accurate..
Created attachment 8484519 [details] [diff] [review]
jetpack fix

This seems to be caused by an insane nesting of Sandbox and loadSubScript stuff. I haven't actually been able to track down at which point we forget the addon id or the addon interposition, but there is an easy fix for this inside the jetpack code.

This does nothing about the "Permission denied to pass object to chrome" error.
(Reporter)

Comment 3

4 years ago
nice, thank you very much for this Tom -- i should have probably found that one by myself, but i'm not that familiar with our Loader.

i filed bug 1064252 for this issue..


> This does nothing about the "Permission denied to pass object to chrome" error.

:( 

which probably means this is not related to interposition?

and unfortunately, this one is the bigger issue, as the other one is avoided simply by adding AsCPOW..
(Reporter)

Updated

4 years ago
Summary: e10s compatibility shims don't work in SDK addons → callbacks passed to a cpow (like a DOM listener) throw "Permission denied to pass object to chrome" (e10s)
I am coming across this issue in a mochitest-browser test:

head.js:
function asyncTest(generator) {
  return () => Task.spawn(generator).then(null, ok.bind(null, false)).then(finish);
}

function wait(ms) {
  let def = promise.defer();
  content.setTimeout(def.resolve, ms); // Throws "Permission denied to pass object to chrome."
  return def.promise;
}

my-test.js:
let test = asyncTest(function*() {
  yield wait(1000);
});
Interestingly, the code in the previous comment works if you use a fat arrow:
function wait(ms) {
  let def = promise.defer();
  content.setTimeout(() => {
    def.resolve();
  }, ms);
  return def.promise;
}
Blocks: 972507
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> I am coming across this issue in a mochitest-browser test:
> 
> head.js:
> function asyncTest(generator) {
>   return () => Task.spawn(generator).then(null, ok.bind(null,
> false)).then(finish);
> }
> 
> function wait(ms) {
>   let def = promise.defer();
>   content.setTimeout(def.resolve, ms); // Throws "Permission denied to pass
> object to chrome."

This line is causing us to send a message to the content process asking it to send us another message when the timeout expires. Can you just use window.setTimeout instead (i.e., use the chrome window)?
(Reporter)

Comment 7

4 years ago
just tested, the last Nightly where cpow DOM events work is from 8/21, which is the same one when contentWindow stopped working, which is curious as they don't look related..
(Reporter)

Comment 8

4 years ago
this doesn't look like it has anything to do with addons, this code in scratchpad also throws:

  gBrowser.selectedTab.linkedBrowser.contentWindowAsCPOW.
    addEventListener('pagehide', () => console.log('hide'));

(then navigate away)
Created attachment 8486814 [details] [diff] [review]
allow-passing-to-cpows

Right now, all references from the child to the parent go through COWs (via the unprivileged junk scope). I didn't realize that COWs prevent certain kinds of objects being passed through them. In particular, it looks like we can't pass CPOWs, so things like event handlers (where the event lives in the child and the handler lives in the parent) won't work at all.

So I just changed the check to allow CPOWs to be passed through. I don't understand what the checks are for, so it's hard to know if this is bad or not. What do you think, Bobby?
Attachment #8486814 - Flags: review?(bobbyholley)
Comment on attachment 8486814 [details] [diff] [review]
allow-passing-to-cpows

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

Per IRC discussion, I think we should narrow this check. Bill's making a new patch.
Attachment #8486814 - Flags: review?(bobbyholley) → review-
Created attachment 8486858 [details] [diff] [review]
allow-passing-to-cpows v2

Bobby asked me to tighten this up a bit.
Attachment #8486814 - Attachment is obsolete: true
Attachment #8486858 - Flags: review?(bobbyholley)
Comment on attachment 8486858 [details] [diff] [review]
allow-passing-to-cpows v2

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

r=me with that.

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +118,5 @@
>          return true;
>  
> +    // CPOWs use COWs (in the unprivileged junk scope) for all child->parent
> +    // references. Without this test, the child process wouldn't be able to
> +    // pass any objects at all to CPOWs.

I'd add "to CPOWed functions".

@@ +120,5 @@
> +    // CPOWs use COWs (in the unprivileged junk scope) for all child->parent
> +    // references. Without this test, the child process wouldn't be able to
> +    // pass any objects at all to CPOWs.
> +    if (mozilla::jsipc::IsWrappedCPOW(obj) &&
> +        js::GetObjectCompartment(wrapper) == js::GetObjectCompartment(xpc::UnprivilegedJunkScope()) &&

I'd prefer JS_GetGlobalForObject(cx, wrapper) == xpc::UnprivilegedJunkScope.
Attachment #8486858 - Flags: review?(bobbyholley) → review+
Comment on attachment 8484519 [details] [diff] [review]
jetpack fix

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

We want to pass the addon-id to the sandbox, so that we can probably account for jetpack code running inside the sandbox and identify it as addon code.
Attachment #8484519 - Flags: review?(gkrizsanits)
(In reply to Tom Schuster [:evilpie] from comment #13)
> Comment on attachment 8484519 [details] [diff] [review]

I'm sorry to hold you up with this for a moment but there are a few things that I want to make clear first.

First, what is this addonID on sandbox? We used to pass the addonID via the sandboxName since long time ago for the memory reporter. Are these two things now different or we just pass/store the same thing twice for no reason? Flagging Bobby to correct me if I'm wrong or if not we should file a follow up bug to clean this up.

Two, based on some patches it seems like Bill was going to change this code in Bug 990729 so that the addonID is no longer stored on the metadata. What happened with that patch? (Flagging Bill for this)

Actually this second part is why I want to hold you up a bit because I have the impression that since Bill landed the patch on inbound, it has been overwritten by a merge (SDK dev usually happens on the fx-team branch). If that is the case I want to avoid the same issue to happen here as well.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bobbyholley)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> (In reply to Tom Schuster [:evilpie] from comment #13)
> > Comment on attachment 8484519 [details] [diff] [review]
> 
> I'm sorry to hold you up with this for a moment but there are a few things
> that I want to make clear first.
> 
> First, what is this addonID on sandbox? We used to pass the addonID via the
> sandboxName since long time ago for the memory reporter. Are these two
> things now different or we just pass/store the same thing twice for no
> reason? Flagging Bobby to correct me if I'm wrong or if not we should file a
> follow up bug to clean this up.

sandboxName is supposed to be a useful human-readable identifier of the sandbox. Sometimes we use a URI or UUID or something machine-parseable, but sometimes we don't [1].

An addonId is a string whose identity is used to make decisions in C++ about which interpositions to apply. It needs to be unique, consistent, and doesn't need to be human-readable. I don't think we should merge these concepts.

[1] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSSubScriptLoader.cpp#489
Flags: needinfo?(bobbyholley)

Updated

4 years ago
Blocks: 905436
tracking-e10s: ? → +
Comment on attachment 8484519 [details] [diff] [review]
jetpack fix

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

Alright, if you want to make sure this lands properly, land it on fx-team branch or give a heads-up to someone from the jetpack team so it won't be overwritten. (I'm not sure who is doing the merges these days.. maybe Tomislav is more up to date)
Attachment #8484519 - Flags: review?(gkrizsanits) → review+
I looked through the patches in bug 990729 and I think I remember what happened. Originally I wanted to replace the addonId passed in as metadata with one passed via options. However, I ran into a bunch of problems with that approach, so I fell back to passing in both. However, I must have forgotten that we still need to pass the add-on ID as part of options in the SDK.

Anyway, Tom's patch is the right way to go.
Flags: needinfo?(wmccloskey)
(In reply to Bobby Holley (:bholley) from comment #17)
> sandboxName is supposed to be a useful human-readable identifier of the
> sandbox. 

It just does not feel like we're doing a great job there then... A few examples:

jar:file:///home/gabor/.mozilla/firefox/83p5fgt7.default/extensions/flashstopper@byo.co.il.xpi!/bootstrap.js (from: resource://gre/modules/addons/XPIProvider.jsm:4174)

[anonymous sandbox] (from: chrome://flashstopper/content/flashstopper.js:50)

jar:file:///home/gabor/.mozilla/firefox/83p5fgt7.default/extensions/jid0-AWShpy08txla2QGDYvv5bed4sjs@jetpack.xpi!/bootstrap.js (from: resource://gre/modules/addons/XPIProvider.jsm:4174)

resource://gre/modules/commonjs/sdk/core/namespace.js (from: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/gabor/.mozilla/firefox/83p5fgt7.default/extensions/jid0-AWShpy08txla2QGDY

The last two examples are I think SDK add-ons, but I think following the concept you just described it would be nicer to see which add-on is that jid0-AWShpy08txla2QGDYvv5bed4s... What do you think? Should we improve something here or is it just me?
(In reply to Bill McCloskey (:billm) from comment #19)
> I looked through the patches in bug 990729 and I think I remember what
> happened.

Thanks for checking this for me, I was just very confused what happened there and wanted to make this clear.
(Reporter)

Updated

4 years ago
Blocks: 1066685
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> (In reply to Bobby Holley (:bholley) from comment #17)
> > sandboxName is supposed to be a useful human-readable identifier of the
> > sandbox. 
> 
> It just does not feel like we're doing a great job there then... A few
> examples:
> 
> jar:file:///home/gabor/.mozilla/firefox/83p5fgt7.default/extensions/
> flashstopper@byo.co.il.xpi!/bootstrap.js (from:
> resource://gre/modules/addons/XPIProvider.jsm:4174)
> 
> [anonymous sandbox] (from: chrome://flashstopper/content/flashstopper.js:50)
> 
> jar:file:///home/gabor/.mozilla/firefox/83p5fgt7.default/extensions/jid0-
> AWShpy08txla2QGDYvv5bed4sjs@jetpack.xpi!/bootstrap.js (from:
> resource://gre/modules/addons/XPIProvider.jsm:4174)
> 
> resource://gre/modules/commonjs/sdk/core/namespace.js (from:
> resource://gre/modules/addons/XPIProvider.jsm ->
> jar:file:///home/gabor/.mozilla/firefox/83p5fgt7.default/extensions/jid0-
> AWShpy08txla2QGDY
> 
> The last two examples are I think SDK add-ons, but I think following the
> concept you just described it would be nicer to see which add-on is that
> jid0-AWShpy08txla2QGDYvv5bed4s... What do you think? Should we improve
> something here or is it just me?

That's probably a question better-directed at njn. I'm just pointing out that the two concepts are mostly orthogonal. Maybe file a separate bug, CC me, and needinfo him?
(Reporter)

Comment 23

4 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> Alright, if you want to make sure this lands properly, land it on fx-team
> branch or give a heads-up to someone from the jetpack team so it won't be
> overwritten. (I'm not sure who is doing the merges these days.. maybe
> Tomislav is more up to date)

i just landed this to jetpack github in bug 1064252, and Mossop is about to uplift to fx-team in a few hours..
This looks like it's stalled out for more than a month.  Can we get a status update?  Do we think this will fix Bug 972507?
Flagging zombie and Mossop because of https://bugzilla.mozilla.org/show_bug.cgi?id=1062024#c23.
Flags: needinfo?(tomica+amo)
Flags: needinfo?(dtownsend+bugmail)
(Reporter)

Comment 26

4 years ago
the patch i mentioned above was landed (and uplifted), and i can confirm the other part (about xray/cpow permissions) is also fixed, so i'm unsure why Bill put the [leave-open] keyword.

as for bug 972507, i guess that uses page-mod. a partial fix for that (that makes it "mostly work" in e10s) has landed in bug 1058698, so you should be able to test that later this week/next week after we uplift addon sdk from github.

feel free to ni? me in that bug if you have more details (or questions) on what works/doesn't.
Flags: needinfo?(tomica+amo)
Flags: needinfo?(dtownsend+bugmail)

Updated

2 years ago
Blocks: 1246906

Comment 27

2 years ago
looks like it was patched for shims - marking triaged
Whiteboard: CPOW, triaged
With Firefox 57 only WebExtensions are permitted and are, by default, e10s compatible.
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → INCOMPLETE
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.