When I recently checked this, we passed everything but the last case: ok(plugin.checkObjectValue(testvalue), "Is this the same plugin instance?");

What was the failure mode? Was plugin.checkObjectValue a function or undefined? If it was undefined, then we definitely need to fix the prototype chain for plugin objects after document moves, and this should probably block a release; we know that it . If plugin.checkObjectValue is a function but returns a different result, that *may* be ok (I'm not sure). If that's the case we have an issue with failing to unwrap plugin JS objects that cross document boundaries, and I don't know whether that would show up as an error in the wild or not. What was the original regression bug? I lost that information when we filed this new bug.

Oops, I misunderstood you. I'm guessing it was compartment-per-global.

We know that it is used in the wild, which led to the discovery of the initial bug. I don't know if there is a keyword for web-compat issues, but that's why I'm nominating this for tracking.

http://de.futbol24.com/national/Algeria/Algerian-Cup/2011-2012/ was the URL where this was found originally.

The error message is "uncaught JS exception reported through window.onerror - TypeError: plugin.checkObjectValue is not a function".

Yeah, that means the proto chain is broken and this really needs to be fixed for proper web compat.

bsmedberg and I talked about this on IRC. I'm going to try to figure how XBL manages to get this right and crib that approach.

So, it turns out that XBL doesn't handle this either. We're going to need a new nsIXPCScriptable hook to tell nsDOMClassInfo that the wrapper object changed and that it needs to rebind stuff like XBL.

k9o nomination - this is likely causing bug 749792 according to John, which so happens to be a blocker, so fixing this could solve the very bad flash failing to initialize problem we are seeing in bug 749792.

I don't think there's a need to track this for Kilimanjaro; it's a functional regression in FF15 that we need to fix well before the k9o timeline.

This is the first part, which peterv needs to review. I'll post the rest of the patches (for bsmedberg to review) shortly.

As it turns out, XBL actually _does_ do the right thing here, just asynchronously. I think that's probably fine, and I'll just update the test accordingly.

Added to nsWindowSH, per IRC conversation.

Comment on attachment 641416 [details] [diff] [review] Add a PostTransplant nsIXPCScriptable hook. v2 Review of attachment 641416 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/idl/nsIXPCScriptable.idl @@ +90,5 @@ > > + // Both methods here are protected by WANT_POSTCREATE. If you want to do > + // something after a wrapper is created, there's a good chance you also > + // want to do something when the wrapper is transplanted to a new > + //compartment. Space after /. ::: js/xpconnect/src/XPCWrappedNative.cpp @@ +1657,5 @@ > + XPCNativeScriptableInfo* si = wrapper->GetScriptableInfo(); > + if (si->GetFlags().WantPostCreate()) { > + nsresult rv = si->GetCallback()->PostTransplant(wrapper, ccx, flat); > + if (NS_FAILED(rv)) > + NS_WARNING("Uh oh - PostTransplant failed!"); Do we think we'll have hooks that fail? Otherwise I'd make them notxpcom and put a MOZ_NOT_REACHED in nsDOMClassInfo::PostTransplant (and not add the nsWindowSH one).

Addressed peter's comments - carrying over review.

Calling OnWrapperDestroyed at this point in OnDestroy doesn't make sense, because the JS objects have a finalize hook that also calls OnWrapperDestroyed regardless of whether or not they still have a pointer stashed in their private. So when we do this, we get a bunch of assertions about unmatched calls to OnWrapperDestroyed. AFAICT the only reason this worked before is that this code never ran: I put a MOZ_ASSERT just before call to OnWrappedDestroyed in OnDestroy, and it never fired during the dom/plugins mochitests.

Comment on attachment 641439 [details] [diff] [review] Part 1 - Add a PostTransplant nsIXPCScriptable hook. v3 r=peterv Preemptively flagging this particular patch for aurora approval. The patch itself is trivial (just adds some empty hooks), but involves an IID rev we'll need for the rest of this stuff. The rest of this stuff could do with a few days bake-time on m-c before uplifting, but that runs up against the beta uplift, after which the IID rev becomes a problem. So I want to land the safe no-op IID rev patch to aurora right away. [Approval Request Comment] Bug caused by (feature/regressing bug #): CPG User impact if declined: Will be more difficult to fix this bug after uplift, and this bug breaks plugin stuff. Testing completed (on m-c, etc.): None Risk to taking this patch (and alternatives if risky): Not risky String or UUID changes made by this patch: UUID rev

Comment on attachment 641441 [details] [diff] [review] Part 3 - Implement post-transplant plugin behavior. v2 ClearJSObject at least needs more comments, in particular: * we are aware that NPPVpluginScriptableNPObject may not give us the same scriptable as last time, but that's ok as long as it doesn't give us the same scriptable next time we call it (from PostCreate). * The _releaseobject call is there to match the retain which is inherent in NPPVpluginScriptableNPObject * I had some questions on IRC about potential ways to break this because we still have cached JS objects pointing at the wrong compartment. reflection from bholley: Maybe this would just be safer if we called JS_WrapObject on the existing object? This is the one that makes me nervous, so bumping to jst.

Comment on attachment 641440 [details] [diff] [review] Bonus Patch - Don't double-call OnWrapperDestroyed. v1 This patch is no longer necessary with the new approach I'm taking, but it's still probably worth landing to avoid biting people in the future. Certainly no need to backport it though.

Comment on attachment 641441 [details] [diff] [review] Part 3 - Implement post-transplant plugin behavior. v2 jst is out of contact for the next few days, and I think I prefer the wrapped-proto approach anyway. Obsoleting this patch.

This is the new part 2, necessary now that we're using wrapped protos. Unwrapping all over the place isn't great, and tends to produce compartment mismatches. But I think we're safe here, since we're mostly just grabbing the private and not passing these objects anywhere to JSAPI.

And this is the new part 3 - much less scary. :-)

Small update.

Pushed the new set to try: https://tbpl.mozilla.org/?tree=Try&rev=cae1aa5f8d04

Comment on attachment 641479 [details] [diff] [review] Part 2 - Make the prototype climbing code in nsJSNPRuntime unwrap security wrappers. v1 It might be good to get a second set of eyes on this: I think I understand what's going on and that it's ok, but I'm not a JSAPI expert.

Comment on attachment 641487 [details] [diff] [review] Part 3 - Implement post-transplant plugin behavior. v3 I think that it's necessary to do this wrapping in nsNPObjWrapper::GetNewOrUsed, not here.

(In reply to Peter Van der Beken [:peterv] from comment #16) > Do we think we'll have hooks that fail? Otherwise I'd make them notxpcom and > put a MOZ_NOT_REACHED in nsDOMClassInfo::PostTransplant (and not add the > nsWindowSH one). Ugh, making this [notxpcom] borks NS_FORWARD_SAFE_NSIXPCSCRIPTABLE. I'm just going to skip it for now.

Trying again.

(In reply to Bobby Holley (:bholley) from comment #22) > Comment on attachment 641439 [details] [diff] [review] > Part 1 - Add a PostTransplant nsIXPCScriptable hook. v3 r=peterv > > Preemptively flagging this particular patch for aurora approval. The patch > itself is trivial (just adds some empty hooks), but involves an IID rev > we'll need for the rest of this stuff. The rest of this stuff could do with > a few days bake-time on m-c before uplifting, but that runs up against the > beta uplift, after which the IID rev becomes a problem. So I want to land > the safe no-op IID rev patch to aurora right away. Thanks for thinking ahead like this. Do we have any alternatives to the IID change? If not, I'd like Jorge's final sign-off on taking this change (since it'd presumably could have add-on/plugin fallout?). Thanks in advance.

(In reply to Alex Keybl [:akeybl] from comment #35) > Thanks for thinking ahead like this. Do we have any alternatives to the IID > change? If not, I'd like Jorge's final sign-off on taking this change (since > it'd presumably could have add-on/plugin fallout?). Thanks in advance. Yes. It's not strictly necessary, and we could hack around it in an ugly way to accomplish the same thing (though that would mean a different patch on beta). The thing is though, nsIXPCScriptable is about as far away from a public API interface as they come. It's binary-only, we change its behavior (if not its signatures) all the time, and using it correctly requires an incredibly intimate knowledge of XPConnect. It's more or less just a conduit for us to communicate between XPConnect and the DOM. I just checked, and there are no references to it on any of the addons hosted on AMO. So I think we're fine.

The test here fails on android XUL opt, but I just discovered that the plugin mochitests don't even run for android (my test did, because I foolishly put it in the xpconnect mochitest dir). I'm just going to move it to dom/plugins and then push.

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/776f008404bf http://hg.mozilla.org/integration/mozilla-inbound/rev/ff73dacf8eb3 http://hg.mozilla.org/integration/mozilla-inbound/rev/101bc799fc73 http://hg.mozilla.org/integration/mozilla-inbound/rev/0d561abceeb3 http://hg.mozilla.org/integration/mozilla-inbound/rev/d3e8af05b074 With a separate push for the bonus patch, which is unrelated to the above but is good to have in the long run: http://hg.mozilla.org/integration/mozilla-inbound/rev/e9203900ce6c

(In reply to Alex Keybl [:akeybl] from comment #35) > Thanks for thinking ahead like this. Do we have any alternatives to the IID > change? If not, I'd like Jorge's final sign-off on taking this change (since > it'd presumably could have add-on/plugin fallout?). Thanks in advance. Also, there's not likely to be any plugin fallout here (this is a regression fix for DOM plugin behavior, but the IID change doesn't affect that). The only addon fallout would occur if someone was using nsIXPCScriptable, and as mentioned in comment 36 I think the chances of that are low.

When I moved the test into dom/plugins, we started getting weird XBL error reports in subsequent tests about <marquee>: https://tbpl.mozilla.org/php/getParsedLog.php?id=13486749&tree=Mozilla-Inbound#error0 It looks like the XBL binding can become unbound after the page navigates in certain cases. so we backed out the test: https://hg.mozilla.org/integration/mozilla-inbound/rev/f658278070ac I'm just going to reland the test without the XBL stuff for now since the patch didn't actually end up having to touch XBL. It's probably worth fixing, but it doesn't block this bug.

Relanded the tests: http://hg.mozilla.org/integration/mozilla-inbound/rev/9f4294d7f12e

(In reply to Bobby Holley (:bholley) from comment #36) > The thing is though, nsIXPCScriptable is about as far away from a public API > interface as they come. It's binary-only, we change its behavior (if not its > signatures) all the time, and using it correctly requires an incredibly > intimate knowledge of XPConnect. It's more or less just a conduit for us to > communicate between XPConnect and the DOM. > > I just checked, and there are no references to it on any of the addons > hosted on AMO. So I think we're fine. Yes, I wouldn't expect any AMO add-ons to be affected. My main concern would be those binary add-ons that are externally hosted, like AV tools. I guess it's unlikely that they'll break because of this, though, and if we can get the patch in early in the beta process, the impact should be minimal.

If we can get approval today before it gets to beta, that would be even better!

Comment on attachment 641439 [details] [diff] [review] Part 1 - Add a PostTransplant nsIXPCScriptable hook. v3 r=peterv [Triage Comment] Approving for Aurora 15 to get as much feedback ahead of Beta as possible. I acknowledge that this is not normal engineering process (landing on m-c and m-a at the same time), but I'd like to get more feedback ahead of the Beta uplift next week. Thanks for the risk assessment Bobby/Jorge.

https://hg.mozilla.org/mozilla-central/rev/d3e8af05b074 https://hg.mozilla.org/mozilla-central/rev/0d561abceeb3 https://hg.mozilla.org/mozilla-central/rev/101bc799fc73 https://hg.mozilla.org/mozilla-central/rev/ff73dacf8eb3 https://hg.mozilla.org/mozilla-central/rev/e9203900ce6c https://hg.mozilla.org/mozilla-central/rev/9f4294d7f12e

Alex, can you clarify whether the approval in comment 44 corresponds just to the no-op patch that revs the IID, or to the entire stack? I think that landing the whole thing before the uplift is probably a good idea (so that we don't ship this regression in a beta). It's baked on m-c the last few days with no reported regressions.

(In reply to Bobby Holley (:bholley) from comment #46) > Alex, can you clarify whether the approval in comment 44 corresponds just to > the no-op patch that revs the IID, or to the entire stack? I think that > landing the whole thing before the uplift is probably a good idea (so that > we don't ship this regression in a beta). It's baked on m-c the last few > days with no reported regressions. Agreed - let's land the whole thing at this point. Thanks for checking in.

Pushed this all to m-a: http://hg.mozilla.org/releases/mozilla-aurora/rev/308cccec8e82 http://hg.mozilla.org/releases/mozilla-aurora/rev/8e507aacb936 http://hg.mozilla.org/releases/mozilla-aurora/rev/a444f0d27400 http://hg.mozilla.org/releases/mozilla-aurora/rev/1e8af398d83c http://hg.mozilla.org/releases/mozilla-aurora/rev/434a32b42d93 Huzzah!