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

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

https://addons.mozilla.org/ja/firefox/addon/flashblock/

The offending code is in flashblock.xml, 113
document.QueryInterface(Components.interfaces.nsIDOMDocument)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is this non-chrome code so it's not seeing the [ChromeOnly] QueryInterface on there?

Comment 2

[Boris Zbarsky [:bzbarsky]]

More to the point, should xrays from the xbl compartment see ChromeOnly methods?

Comment 3

[Bobby Holley (:bholley)]

(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.

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make QueryInterface be exposed for both chrome and xbl scopes, not just in chrome.

Bobby, any idea how to test this sanely?

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Philip Chee]

Is there a workaround I can use in Flashblock in the meantime, or wait for a fix to land before uplift to Aurora?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Andrea will be looking at form, but AIUI you're not blocked on that?

Comment 14

[Philip Chee]

> 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: This could probably use another review...

Comment 16

[Bobby Holley (:bholley)]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/049889b25a79

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc, mochitest-other, and xpcshell orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9182c3e6a967

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: With that issue fixed

Should probably get another review

Comment 22

[Boris Zbarsky [:bzbarsky]]

Try run for that last is green: https://tbpl.mozilla.org/?tree=Try&rev=9e00fd5bb547

Comment 23

[Boris Zbarsky [:bzbarsky]]

Let's try that again: https://hg.mozilla.org/integration/mozilla-inbound/rev/55620ca9673c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55620ca9673c

Comment 25

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c177454b9e74