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

Status: RESOLVED INCOMPLETE

(Firefox :: Extension Compatibility, defect)

Description

[Tomislav Jovanovic :zombie]

Attachment: 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)

Comment 1

[Tomislav Jovanovic :zombie]

> 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..

Comment 2

[Tom S [:evilpie]]

Attachment: 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.

Comment 3

[Tomislav Jovanovic :zombie]

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..

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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);
});

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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;
}

Comment 6

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

(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)?

Comment 7

[Tomislav Jovanovic :zombie]

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..

Comment 8

[Tomislav Jovanovic :zombie]

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)

Comment 9

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

Attachment: 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?

Comment 10

[Bobby Holley (:bholley)]

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.

Comment 11

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

Attachment: allow-passing-to-cpows v2

Bobby asked me to tighten this up a bit.

Comment 12

[Bobby Holley (:bholley)]

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.

Comment 13

[Tom S [:evilpie]]

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.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c6d2dff9381

Comment 15

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c6d2dff9381

Comment 17

[Bobby Holley (:bholley)]

(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

Comment 18

[Gabor Krizsanits (INACTIVE)]

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)

Comment 19

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

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.

Comment 20

[Gabor Krizsanits (INACTIVE)]

(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/[e-mail]!/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/[e-mail]!/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?

Comment 21

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 22

[Bobby Holley (:bholley)]

(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/
> [e-mail]!/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-
> [e-mail]!/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?

Comment 23

[Tomislav Jovanovic :zombie]

(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..

Comment 24

[Nick Alexander :nalexander [he/him]]

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?

Comment 25

[Nick Alexander :nalexander [he/him]]

Flagging zombie and Mossop because of https://bugzilla.mozilla.org/show_bug.cgi?id=1062024#c23.

Comment 26

[Tomislav Jovanovic :zombie]

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.

Comment 27

[:shell escalante]

looks like it was patched for shims - marking triaged

Comment 28

[Firefox Bug Husbandry Bot]

With Firefox 57 only WebExtensions are permitted and are, by default, e10s compatible.

Comment 29

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.