Closed
Bug 771202
Opened 12 years ago
Closed 12 years ago
Custom-spliced plugin prototypes disappear after transplant
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: mccr8, Assigned: bholley)
References
Details
Attachments
(7 files, 5 obsolete files)
2.50 KB,
patch
|
Details | Diff | Splinter Review | |
10.34 KB,
patch
|
bholley
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
891 bytes,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
2.40 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
7.59 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
2.09 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•12 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•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
When I recently checked this, we passed everything but the last case:
ok(plugin.checkObjectValue(testvalue), "Is this the same plugin instance?");
Comment 3•12 years ago
|
||
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 | ||
Comment 4•12 years ago
|
||
Oops, I misunderstood you. I'm guessing it was compartment-per-global.
Comment 5•12 years ago
|
||
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:
--- → ?
Comment 6•12 years ago
|
||
http://de.futbol24.com/national/Algeria/Algerian-Cup/2011-2012/ was the URL where this was found originally.
Reporter | ||
Comment 7•12 years ago
|
||
The error message is "uncaught JS exception reported through window.onerror - TypeError: plugin.checkObjectValue is not a function".
Comment 8•12 years ago
|
||
Yeah, that means the proto chain is broken and this really needs to be fixed for proper web compat.
Assignee | ||
Comment 9•12 years ago
|
||
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
Assignee | ||
Comment 10•12 years ago
|
||
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
Comment 11•12 years ago
|
||
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•12 years ago
|
tracking-firefox16:
? → ---
Comment 12•12 years ago
|
||
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•12 years ago
|
blocking-kilimanjaro: ? → ---
Assignee | ||
Comment 13•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Summary: Custom-spliced prototypes (XBL and Plugins) disappear after transplant → Custom-spliced plugin prototypes disappear after transplant
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
Added to nsWindowSH, per IRC conversation.
Attachment #641412 -
Attachment is obsolete: true
Attachment #641412 -
Flags: review?(peterv)
Attachment #641416 -
Flags: review?(peterv)
Comment 16•12 years ago
|
||
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+
Assignee | ||
Comment 17•12 years ago
|
||
Addressed peter's comments - carrying over review.
Attachment #641416 -
Attachment is obsolete: true
Attachment #641439 -
Flags: review+
Assignee | ||
Comment 18•12 years ago
|
||
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)
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #641441 -
Flags: review?(benjamin)
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #641442 -
Flags: review?(benjamin)
Assignee | ||
Comment 21•12 years ago
|
||
Attachment #641443 -
Flags: review?(benjamin)
Assignee | ||
Comment 22•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #641440 -
Flags: review?(benjamin) → review+
Comment 23•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #641442 -
Flags: review?(benjamin) → review+
Updated•12 years ago
|
Attachment #641443 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 24•12 years ago
|
||
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
Assignee | ||
Comment 25•12 years ago
|
||
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)
Assignee | ||
Comment 26•12 years ago
|
||
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)
Assignee | ||
Comment 27•12 years ago
|
||
And this is the new part 3 - much less scary. :-)
Attachment #641480 -
Flags: review?(benjamin)
Assignee | ||
Comment 28•12 years ago
|
||
Small update.
Attachment #641480 -
Attachment is obsolete: true
Attachment #641480 -
Flags: review?(benjamin)
Attachment #641487 -
Flags: review?(benjamin)
Assignee | ||
Comment 29•12 years ago
|
||
Pushed the new set to try: https://tbpl.mozilla.org/?tree=Try&rev=cae1aa5f8d04
Comment 30•12 years ago
|
||
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 31•12 years ago
|
||
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-
Assignee | ||
Comment 32•12 years ago
|
||
(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.
Assignee | ||
Comment 33•12 years ago
|
||
Trying again.
Attachment #641487 -
Attachment is obsolete: true
Attachment #641570 -
Flags: review?(benjamin)
Updated•12 years ago
|
Attachment #641570 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 34•12 years ago
|
||
Comment 35•12 years ago
|
||
(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.
Assignee | ||
Comment 36•12 years ago
|
||
(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.
Assignee | ||
Comment 37•12 years ago
|
||
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.
Assignee | ||
Comment 38•12 years ago
|
||
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
Assignee | ||
Comment 39•12 years ago
|
||
(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.
Assignee | ||
Comment 40•12 years ago
|
||
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.
Assignee | ||
Comment 41•12 years ago
|
||
Relanded the tests: http://hg.mozilla.org/integration/mozilla-inbound/rev/9f4294d7f12e
Comment 42•12 years ago
|
||
(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•12 years ago
|
||
If we can get approval today before it gets to beta, that would be even better!
Updated•12 years ago
|
Comment 44•12 years ago
|
||
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+
Comment 45•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Assignee | ||
Comment 46•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Comment 48•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•