https://addons.mozilla.org/ja/firefox/addon/flashblock/ The offending code is in flashblock.xml, 113 document.QueryInterface(Components.interfaces.nsIDOMDocument)
Is this non-chrome code so it's not seeing the [ChromeOnly] QueryInterface on there?
More to the point, should xrays from the xbl compartment see ChromeOnly methods?
(In reply to Boris Zbarsky (:bz) from comment #2) > More to the point, should xrays from the xbl compartment see ChromeOnly > methods? They most certainly should not.
So yeah, XBL definitely needs QueryInterface (that was one of the main motivators to XBL scopes - Ci is needed by XBL, but not the web). So it should be behind IsChromeOrXBL, similar to: http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Node.webidl#99
Bobby, any idea how to test this sanely?
Attachment #746181 - Flags: review?(bobbyholley+bmo)
Comment on attachment 746181 [details] [diff] [review] Make QueryInterface be exposed for both chrome and xbl scopes, not just in chrome. Assuming |cx| is in the caller's compartment, this is the function you want to call. I'll trust you on the rest of the codegen. As for testing, it should be straightforward, unless I'm missing something. Just make sure you can document.QueryInterface from any XBL in our test suite (file_bug821850.xhtml would be fine), and that you can't from a plain old content scope.
Attachment #746181 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 746181 [details] [diff] [review] Make QueryInterface be exposed for both chrome and xbl scopes, not just in chrome. This doesn't actually work.... And the reason it doesn't work is that Window is not on WebIDL bindings but EventTarget is, and is quickstubbed. So if we put a QueryInterface in the non-chrome bits on EventTarget, as this patch does, it will clobber the XPConnect QueryInterface on Window.prototype. But unlike the XPConnect one this one is designed to work with WebIDL objects only and does not do interface flattening. That means that things like window.QueryInterface(Ci.nsIInterfaceRequestor) don't produce a useful getInterface method on the object.
Attachment #746181 - Flags: review+ → review-
So if we think we'll get form on WebIDL bindings before we ship 23, we could just have nsINode::IsChromeOrXBL check for a WebIDL prototype and return false if not to avoid defining this QueryInterface on non-WebIDL protos... Andrea, Ms2ger, is that feasible?
Alternately, I could define QueryInterface on the rootmost interface that does not have hasXPConnectImpls set, instead of on the rootmost interface period... that seems like it might work as well.
Is there a workaround I can use in Flashblock in the meantime, or wait for a fix to land before uplift to Aurora?
> Is there a workaround I can use in Flashblock in the meantime Not doing the QI to nsIDOMDocument? That should be a no-op anyway; why does it need to be done?
Andrea will be looking at form, but AIUI you're not blocked on that?
> Not doing the QI to nsIDOMDocument? That should be a no-op anyway; why does it need > to be done? Probably dates back to Firefox 1.0. I'll have a look. Thanks.
Attachment #746181 - Attachment is obsolete: true
Comment on attachment 746801 [details] [diff] [review] This could probably use another review... Review of attachment 746801 [details] [diff] [review]: ----------------------------------------------------------------- the IsChromeOrXBL stuff looks correct, but as for the rest, I think peter would be a better reviewer here.
Attachment #746801 - Flags: review?(bobbyholley+bmo) → review?(peterv)
Attachment #746801 - Flags: review?(peterv) → review+
Comment on attachment 746801 [details] [diff] [review] This could probably use another review... [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 855971 User impact if declined: FlashBlock not working right unless they work around, probably other addon breakage. Testing completed (on m-c, etc.): Code inspection, attached tests. Risk to taking this patch (and alternatives if risky): Risk should be low, I think. String or IDL/UUID changes made by this patch: None.
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Backed out for mochitest-bc, mochitest-other, and xpcshell orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/9182c3e6a967
OK, the orange was from this patch. Clearly my memory of running it through Try after fixing the Window thing was mistaken. The approach here fails any time we have an object whose most-derived interface is hasXPConnectImpls (since in that case all its ancestors will also be hasXPConnectImpls and that object will have no QueryInterface defined). In particular, that happens with Element instances. I'm going to try a different approach: define QueryInterface on the root non-abstract things. That should make sure all actual WebIDL objects have it, while keeping it off Window.prototype.
Should probably get another review
Attachment #749614 - Flags: review?(peterv)
Try run for that last is green: https://tbpl.mozilla.org/?tree=Try&rev=9e00fd5bb547
Attachment #749614 - Flags: review?(peterv) → review+
Let's try that again: https://hg.mozilla.org/integration/mozilla-inbound/rev/55620ca9673c
Whiteboard: [need review]
Attachment #749614 - Flags: approval-mozilla-aurora?
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #749614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.