Custom-spliced plugin prototypes disappear after transplant

RESOLVED FIXED in Firefox 15

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: bholley)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15+ fixed)

Details

Attachments

(7 attachments, 5 obsolete attachments)

2.50 KB, patch
Details | Diff | Splinter Review
10.34 KB, patch
bholley
: review+
Details | Diff | Splinter Review
891 bytes, patch
bsmedberg
: review+
Details | Diff | Splinter Review
2.40 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
3.41 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
7.59 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
2.09 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

5 years ago
Summary: Can'moving a plugin into a subdocument makes it impossible to access any functions exported by plugin → Can't access any functions exported by a plugin after it is moved into a subdocument
(Reporter)

Comment 1

5 years ago
Created attachment 639384 [details] [diff] [review]
bsmedberg's test case
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Updated

5 years ago
Depends on: 752340
(Reporter)

Comment 4

5 years ago
Oops, I misunderstood you.  I'm guessing it was compartment-per-global.
Blocks: 650353
No longer depends on: 752340
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.
tracking-firefox15: --- → ?
http://de.futbol24.com/national/Algeria/Algerian-Cup/2011-2012/ was the URL where this was found originally.
(Reporter)

Comment 7

5 years ago
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.
Assignee: nobody → bobbyholley+bmo
Blocks: 749792
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.
Summary: Can't access any functions exported by a plugin after it is moved into a subdocument → Custom-spliced prototypes (XBL and Plugins) disappear after transplant
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.
blocking-kilimanjaro: --- → ?
tracking-firefox16: --- → ?

Updated

5 years ago
tracking-firefox16: ? → ---
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.

Updated

5 years ago
blocking-kilimanjaro: ? → ---

Updated

5 years ago
No longer blocks: 749792
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.
Attachment #641412 - Flags: review?(peterv)
Summary: Custom-spliced prototypes (XBL and Plugins) disappear after transplant → Custom-spliced plugin prototypes disappear after transplant
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.
Created attachment 641416 [details] [diff] [review]
Add a PostTransplant nsIXPCScriptable hook. v2

Added to nsWindowSH, per IRC conversation.
Attachment #641412 - Attachment is obsolete: true
Attachment #641412 - Flags: review?(peterv)
Attachment #641416 - Flags: review?(peterv)
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).
Attachment #641416 - Flags: review?(peterv) → review+
Created attachment 641439 [details] [diff] [review]
Part 1 - Add a PostTransplant nsIXPCScriptable hook. v3 r=peterv

Addressed peter's comments - carrying over review.
Attachment #641416 - Attachment is obsolete: true
Attachment #641439 - Flags: review+
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.
Attachment #641440 - Flags: review?(benjamin)
Created attachment 641441 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v2
Attachment #641441 - Flags: review?(benjamin)
Created attachment 641442 [details] [diff] [review]
Part 4 - Unwrap object arguments passed to plugins. v2
Attachment #641442 - Flags: review?(benjamin)
Created attachment 641443 [details] [diff] [review]
Part 5 - Tests. v1
Attachment #641443 - Flags: review?(benjamin)
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
Attachment #641439 - Flags: approval-mozilla-aurora?
Attachment #641440 - Flags: review?(benjamin) → review+
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.
Attachment #641441 - Flags: review?(benjamin) → review?(jst)
Attachment #641442 - Flags: review?(benjamin) → review+
Attachment #641443 - Flags: review?(benjamin) → review+
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.
Attachment #641440 - Attachment description: Part 2 - Don't double-call OnWrapperDestroyed. v1 → Bonus Patch - Don't double-call OnWrapperDestroyed. v1
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.
Attachment #641441 - Attachment is obsolete: true
Attachment #641441 - Flags: review?(jst)
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.
Attachment #641479 - Flags: review?(benjamin)
Created attachment 641480 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v3

And this is the new part 3 - much less scary. :-)
Attachment #641480 - Flags: review?(benjamin)
Created attachment 641487 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v3

Small update.
Attachment #641480 - Attachment is obsolete: true
Attachment #641480 - Flags: review?(benjamin)
Attachment #641487 - Flags: review?(benjamin)
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.
Attachment #641479 - Flags: review?(benjamin) → review+
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.
Attachment #641487 - Flags: review?(benjamin) → review-
(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.
Created attachment 641570 [details] [diff] [review]
Part 3 - Implement post-transplant plugin behavior. v4

Trying again.
Attachment #641487 - Attachment is obsolete: true
Attachment #641570 - Flags: review?(benjamin)
Attachment #641570 - Flags: review?(benjamin) → review+
https://tbpl.mozilla.org/?tree=Try&rev=486ca378dc48
(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!

Updated

5 years ago
tracking-firefox15: ? → +
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.
Attachment #641439 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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!
status-firefox15: --- → fixed

Updated

5 years ago
Depends on: 774052
Depends on: 777098
You need to log in before you can comment on or make changes to this bug.