We have an extension that overrides mailto by using a chrome URL (long story). Anyway, previous to 1030420, the extension works fine. After 1030420, while global variables are still defined when the mailto handler is run, our JSM imports are no longer defined. Something seems to be causing us to unload sooner than it did. We do something kind of odd in that we open a tab with the chrome URL and then close it, but previous to the 1030420 change, everything worked find. The extension is here: https://addons.mozilla.org/en-US/firefox/addon/webde-mailcheck/?src=search The code in question is mailto-handler.js in the webapps directory. A reduced testcase isn't really possible here because everything that's happening is integral to our code. A test account to use is mailcheck_qs6@web.de qstest12!

Here is the actual file in question, along with repro steps. To reproduce the problem, install the extension and then type mailto:foo@bar.com in the URL bar. Login using the credentials mailcheck_qs6@web.de/qstest12!. You'll get the error "logic is undefined" You'll see at the top, we do: var logic = {} importJSM("email/webapp-start.js", logic); If you are not logged in, we do an async login call and at line 55, we use logic to start a usecase. In the failing case, logic has become undefined. Oddly, other global variables like firefoxWindow and unitedFromAbove are still defined.

Thanks for the excellent test case Mike! I think I know what's going on. We're creating one of our add-on scopes here. We first run some code that adds a property to the sandbox global. That does the usual thing of adding a shim property to the sandbox global and adding the real property to the XUL window. Then, at some point, I think we navigate away from the original XUL window. Later, we run some code in the sandbox scope. When it tries to find its old property, it finds the shim property on the sandbox and runs the getter for it. That getter fetches the sandbox global's proto, which is a CCW around an outer window proxy. However, the inner window for that outer is now different, so the property is not found. Anyway, the basic problem is that scope chains are supposed to go to the inner window. But, somewhere in the process of our whole writeToGlobalProto setup, we're injecting an outer window proxy in the mix, and that breaks lookups when you navigate. The obvious fix would be to change the sandbox global's proto to point to the inner window. This seems risky though. Do you have any ideas, Bobby?

(In reply to Bill McCloskey (:billm) from comment #2) > Anyway, the basic problem is that scope chains are supposed to go to the > inner window. But, somewhere in the process of our whole writeToGlobalProto > setup, we're injecting an outer window proxy in the mix Well, it's not just "somewhere in the process" - we have a fundamental in JS/SM that direct references to inner windows are never exposed to script. The SandboxPrototypeProxy is just an additional this-fixing wrapper layer around the value passed as sandboxPrototype, which, if a window, will always be an outer. > The obvious fix would be to change the sandbox global's proto to point to > the inner window. This seems risky though. Do you have any ideas, Bobby? In general, trying to operate on inners like this is going to be a royal PITA, and involve either a lot of hackery or a significant redesign of how we do this stuff. I'm not saying that it's out of the question, but it's definitely a pile of work that we should avoid if we're able. Do we have a sense of how badly this is biting us? One potential trick would be to invert the home of the "canonical property" - put the property on the sandbox global, and put the forwarding propertyops on the window. This wouldn't work for addon code that's looking for global properties set by other code, but might solve this particular case. Anyway, let me know how deep we want to go here and whether Boris and I should spend time trying to cook something up here.

The whole "barewords are looked up on the no-longer-current inner window" thing is not something people should really depend on if they can avoid it. It sounds like this extension is depending on that, though? Exposing inners directly to script would require some careful thinking about security and invariants, and could only be done with chrome, not content. I'm not too excited about adding yet another way in which chrome and content differ. That said, given comment 2 and comment 3 I don't understand why compartment-per-addon matters here; the proto of a sandbox would be the global anyway. Or is the issue that in the non-compartment-per-addon case there is no sandbox involved?

I think there might be a simple solution here. What makes this situation a little unusual is that the add-on is creating its own XUL window. There's no actual reason for us to have a separate add-on scope for this window since the whole window belongs to the add-on. So we could just turn off all our machinery in this case. The only reason we want separate add-on scopes is for Firefox windows that also have add-on XUL or XBL attached. As far as I know, these windows never navigate. So maybe there's no problem. We'll just have to be more careful about when we use add-on scopes. I'll see if this works.

(In reply to Boris Zbarsky [:bz] from comment #4) > That said, given comment 2 and comment 3 I don't understand why > compartment-per-addon matters here; the proto of a sandbox would be the > global anyway. Or is the issue that in the non-compartment-per-addon case > there is no sandbox involved? Without compartment-per-addon there is no sandbox, yes. In that case the code just runs with the inner window directly on its scope chain and everything works fine.

(In reply to Boris Zbarsky [:bz] from comment #4) > The whole "barewords are looked up on the no-longer-current inner window" > thing is not something people should really depend on if they can avoid it. > It sounds like this extension is depending on that, though? Yes. > Exposing inners directly to script would require some careful thinking about > security and invariants, and could only be done with chrome, not content. > I'm not too excited about adding yet another way in which chrome and content > differ. Agreed. > That said, given comment 2 and comment 3 I don't understand why > compartment-per-addon matters here; > the proto of a sandbox would be the global anyway. I don't understand what you mean. The proto of the sandbox is set to { sandboxPrototype: someChromeWindow }, which means that it gets set up as an outer. > Or is the issue that in the non-compartment-per-addon case > there is no sandbox involved? Well, yes. The sandbox in question is the "compartment" in "compartment-per-addon". In the regular case, it all just lives in the scope of the Window.

(In reply to Bill McCloskey (:billm) from comment #5) > I think there might be a simple solution here. What makes this situation a > little unusual is that the add-on is creating its own XUL window. There's no > actual reason for us to have a separate add-on scope for this window since > the whole window belongs to the add-on. So we could just turn off all our > machinery in this case. Ahah! That seems promising. > The only reason we want separate add-on scopes is for Firefox windows that > also have add-on XUL or XBL attached. As far as I know, these windows never > navigate. That matches my understanding. Let's give it a try!

> Without compartment-per-addon there is no sandbox, yes OK. So modulo the solution proposed in comment 5, it sounds like what we would really want to do is to run code in the sandbox in such a way that: 1) The "this" value for the code is the window, not the sandbox global. 2) The scope chain has the window on it. The sticking point there is that we want to compile the code in the sandbox compartment, so we have to have a cross-compartment wrapper for the window on the scope chain, and we can't have direct cross-compartment wrappers for inner windows... Changing just _that_ for the special case of scope chains may not be too scary. But I agree that the solution from comment 5 is likely to be simpler at least in terms of wrapper machinery.

This turned out to be pretty simple. We just need to associate an add-on ID with normal windows, and we also need to avoid creating a special add-on scope if the scope is already associated with the add-on.

Comment on attachment 8516996 [details] [diff] [review] fix-webmail.de Review of attachment 8516996 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +2264,5 @@ > if (aNewInner->GetOuterWindow()) { > top = aNewInner->GetTop(); > } > JS::CompartmentOptions options; > + options.setAddonId(MapURIToAddonID(aURI)); So this only handles the case where the addon actually navigates a window to one of its URIs, and not random windows it might open pointing to other things, right? Please indicate why we're doing this, and reference this bug. ::: js/xpconnect/src/XPCWrappedNativeScope.cpp @@ +346,5 @@ > MOZ_ASSERT(addonId); > MOZ_ASSERT(nsContentUtils::IsSystemPrincipal(GetPrincipal())); > > + // If the global is already part of the add-on then there's no reason to > + // create a new one. Describe the case this handles in more detail here.

(In reply to Bobby Holley (:bholley) from comment #11) > ::: dom/base/nsGlobalWindow.cpp > @@ +2264,5 @@ > > if (aNewInner->GetOuterWindow()) { > > top = aNewInner->GetTop(); > > } > > JS::CompartmentOptions options; > > + options.setAddonId(MapURIToAddonID(aURI)); > > So this only handles the case where the addon actually navigates a window to > one of its URIs, and not random windows it might open pointing to other > things, right? Please indicate why we're doing this, and reference this bug. Yes, although that's a broader issue with compartment-per-addon. Even without this patch, we don't treat code as part of an add-on if the URL doesn't map to the add-on.

Shoot, somehow the extra comments didn't get folded into the patch I landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/f786dcb23eca

https://hg.mozilla.org/mozilla-central/rev/9fa06a2e1a98 https://hg.mozilla.org/mozilla-central/rev/f786dcb23eca

backed out on request from bill in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-central&revision=b62ccf3228ba

Here's a new patch that includes the principal check and a testcase. I checked that it crashes without the principal check and works with it.

Comment on attachment 8519318 [details] [diff] [review] addon-navigation Review of attachment 8519318 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

Bill, thanks for fixing this!