Closed Bug 869195 Opened 7 years ago Closed 7 years ago

Flash Block stops working since landing Bug 855971, Exception: document.QueryInterface is not a function

Categories

(Core :: DOM: Core & HTML, defect)

23 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed

People

(Reporter: alice0775, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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?
Flags: needinfo?(amarchesini)
Flags: needinfo?(Ms2ger)
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?
Flags: needinfo?(amarchesini)
Flags: needinfo?(Ms2ger)
Duplicate of this bug: 869724
> 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 #746801 - Flags: review?(bobbyholley+bmo)
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.
Attachment #746801 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/049889b25a79
Flags: in-testsuite+
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)
Attachment #746801 - Attachment is obsolete: true
Attachment #746801 - Flags: approval-mozilla-aurora?
Whiteboard: [need review]
Attachment #749614 - Flags: review?(peterv) → review+
Attachment #749614 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/55620ca9673c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #749614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 898874
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.