Last Comment Bug 771202 - Custom-spliced plugin prototypes disappear after transplant
: Custom-spliced plugin prototypes disappear after transplant
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on: 774052 777098
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-07-05 09:56 PDT by Andrew McCreight [:mccr8]
Modified: 2012-07-24 15:05 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
bsmedberg's test case (2.50 KB, patch)
2012-07-05 10:01 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
Add a PostTransplant nsIXPCScriptable hook. v1 (10.47 KB, patch)
2012-07-12 02:59 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
Add a PostTransplant nsIXPCScriptable hook. v2 (11.26 KB, patch)
2012-07-12 03:22 PDT, Bobby Holley (busy)
peterv: review+
Details | Diff | Review
Part 1 - Add a PostTransplant nsIXPCScriptable hook. v3 r=peterv (10.34 KB, patch)
2012-07-12 06:27 PDT, Bobby Holley (busy)
bobbyholley: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
Bonus Patch - Don't double-call OnWrapperDestroyed. v1 (891 bytes, patch)
2012-07-12 06:27 PDT, Bobby Holley (busy)
benjamin: review+
Details | Diff | Review
Part 3 - Implement post-transplant plugin behavior. v2 (3.43 KB, patch)
2012-07-12 06:28 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
Part 4 - Unwrap object arguments passed to plugins. v2 (2.40 KB, patch)
2012-07-12 06:28 PDT, Bobby Holley (busy)
benjamin: review+
Details | Diff | Review
Part 5 - Tests. v1 (3.41 KB, patch)
2012-07-12 06:28 PDT, Bobby Holley (busy)
benjamin: review+
Details | Diff | Review
Part 2 - Make the prototype climbing code in nsJSNPRuntime unwrap security wrappers. v1 (7.59 KB, patch)
2012-07-12 08:17 PDT, Bobby Holley (busy)
benjamin: review+
Details | Diff | Review
Part 3 - Implement post-transplant plugin behavior. v3 (1.79 KB, patch)
2012-07-12 08:17 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
Part 3 - Implement post-transplant plugin behavior. v3 (1.83 KB, patch)
2012-07-12 08:23 PDT, Bobby Holley (busy)
benjamin: review-
Details | Diff | Review
Part 3 - Implement post-transplant plugin behavior. v4 (2.09 KB, patch)
2012-07-12 13:02 PDT, Bobby Holley (busy)
benjamin: review+
Details | Diff | Review

Description Andrew McCreight [:mccr8] 2012-07-05 09:56:45 PDT

    
Comment 1 Andrew McCreight [:mccr8] 2012-07-05 10:01:20 PDT
Created attachment 639384 [details] [diff] [review]
bsmedberg's test case
Comment 2 Andrew McCreight [:mccr8] 2012-07-05 10:01:51 PDT
When I recently checked this, we passed everything but the last case:
  ok(plugin.checkObjectValue(testvalue), "Is this the same plugin instance?");
Comment 3 Benjamin Smedberg [:bsmedberg] 2012-07-05 10:14:49 PDT
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.
Comment 4 Andrew McCreight [:mccr8] 2012-07-05 10:19:13 PDT
Oops, I misunderstood you.  I'm guessing it was compartment-per-global.
Comment 5 Benjamin Smedberg [:bsmedberg] 2012-07-05 10:28:30 PDT
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.
Comment 6 Benjamin Smedberg [:bsmedberg] 2012-07-05 10:34:36 PDT
http://de.futbol24.com/national/Algeria/Algerian-Cup/2011-2012/ was the URL where this was found originally.
Comment 7 Andrew McCreight [:mccr8] 2012-07-05 11:18:54 PDT
The error message is "uncaught JS exception reported through window.onerror - TypeError: plugin.checkObjectValue is not a function".
Comment 8 Benjamin Smedberg [:bsmedberg] 2012-07-05 11:25:37 PDT
Yeah, that means the proto chain is broken and this really needs to be fixed for proper web compat.
Comment 9 Bobby Holley (busy) 2012-07-05 13:14:19 PDT
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.
Comment 10 Bobby Holley (busy) 2012-07-06 06:45:35 PDT
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.
Comment 11 Jason Smith [:jsmith] 2012-07-06 11:34:01 PDT
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.
Comment 12 Benjamin Smedberg [:bsmedberg] 2012-07-06 16:07:32 PDT
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.
Comment 13 Bobby Holley (busy) 2012-07-12 02:59:55 PDT
Created attachment 641412 [details] [diff] [review]
Add a PostTransplant nsIXPCScriptable hook. v1

This is the first part, which peterv needs to review. I'll post the rest of the patches (for bsmedberg to review) shortly.
Comment 14 Bobby Holley (busy) 2012-07-12 03:00:54 PDT
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.
Comment 15 Bobby Holley (busy) 2012-07-12 03:22:30 PDT
Created attachment 641416 [details] [diff] [review]
Add a PostTransplant nsIXPCScriptable hook. v2

Added to nsWindowSH, per IRC conversation.
Comment 16 Peter Van der Beken [:peterv] 2012-07-12 03:29:18 PDT
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).
Comment 17 Bobby Holley (busy) 2012-07-12 06:27:33 PDT
Created attachment 641439 [details] [diff] [review]
Part 1 - Add a PostTransplant nsIXPCScriptable hook. v3 r=peterv

Addressed peter's comments - carrying over review.
Comment 18 Bobby Holley (busy) 2012-07-12 06:27:48 PDT
Created attachment 641440 [details] [diff] [review]
Bonus Patch - Don't double-call OnWrapperDestroyed. v1

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 19 Bobby Holley (busy) 2012-07-12 06:28:05 PDT
Created attachment 641441 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v2
Comment 20 Bobby Holley (busy) 2012-07-12 06:28:21 PDT
Created attachment 641442 [details] [diff] [review]
Part 4 - Unwrap object arguments passed to plugins. v2
Comment 21 Bobby Holley (busy) 2012-07-12 06:28:36 PDT
Created attachment 641443 [details] [diff] [review]
Part 5 - Tests. v1
Comment 22 Bobby Holley (busy) 2012-07-12 06:34:44 PDT
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 23 Benjamin Smedberg [:bsmedberg] 2012-07-12 07:16:21 PDT
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 24 Bobby Holley (busy) 2012-07-12 08:11:42 PDT
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 25 Bobby Holley (busy) 2012-07-12 08:12:28 PDT
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.
Comment 26 Bobby Holley (busy) 2012-07-12 08:17:06 PDT
Created attachment 641479 [details] [diff] [review]
Part 2 - Make the prototype climbing code in nsJSNPRuntime unwrap security wrappers. v1

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.
Comment 27 Bobby Holley (busy) 2012-07-12 08:17:48 PDT
Created attachment 641480 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v3

And this is the new part 3 - much less scary. :-)
Comment 28 Bobby Holley (busy) 2012-07-12 08:23:34 PDT
Created attachment 641487 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v3

Small update.
Comment 29 Bobby Holley (busy) 2012-07-12 08:28:39 PDT
Pushed the new set to try: https://tbpl.mozilla.org/?tree=Try&rev=cae1aa5f8d04
Comment 30 Benjamin Smedberg [:bsmedberg] 2012-07-12 10:51:17 PDT
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 31 Benjamin Smedberg [:bsmedberg] 2012-07-12 11:00:53 PDT
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.
Comment 32 Bobby Holley (busy) 2012-07-12 11:09:01 PDT
(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.
Comment 33 Bobby Holley (busy) 2012-07-12 13:02:55 PDT
Created attachment 641570 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v4

Trying again.
Comment 34 Bobby Holley (busy) 2012-07-12 14:31:05 PDT
https://tbpl.mozilla.org/?tree=Try&rev=486ca378dc48
Comment 35 Alex Keybl [:akeybl] 2012-07-12 15:27:36 PDT
(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.
Comment 36 Bobby Holley (busy) 2012-07-13 00:25:32 PDT
(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.
Comment 37 Bobby Holley (busy) 2012-07-13 01:50:11 PDT
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.
Comment 39 Bobby Holley (busy) 2012-07-13 02:02:40 PDT
(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.
Comment 40 Bobby Holley (busy) 2012-07-13 04:16:54 PDT
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.
Comment 41 Bobby Holley (busy) 2012-07-13 04:21:12 PDT
Relanded the tests: http://hg.mozilla.org/integration/mozilla-inbound/rev/9f4294d7f12e
Comment 42 Jorge Villalobos [:jorgev] 2012-07-13 08:32:08 PDT
(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.
Comment 43 Benjamin Smedberg [:bsmedberg] 2012-07-13 10:03:26 PDT
If we can get approval today before it gets to beta, that would be even better!
Comment 44 Alex Keybl [:akeybl] 2012-07-13 13:35:32 PDT
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.
Comment 46 Bobby Holley (busy) 2012-07-15 14:55:01 PDT
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.
Comment 47 Alex Keybl [:akeybl] 2012-07-15 15:30:30 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.