Closed
Bug 869195
Opened 12 years ago
Closed 11 years ago
Flash Block stops working since landing Bug 855971, Exception: document.QueryInterface is not a function
Categories
(Core :: DOM: Core & HTML, defect)
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)
(deleted),
patch
|
peterv
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
https://addons.mozilla.org/ja/firefox/addon/flashblock/ The offending code is in flashblock.xml, 113 document.QueryInterface(Components.interfaces.nsIDOMDocument)
tracking-firefox23:
--- → ?
Keywords: regression
![]() |
Assignee | |
Comment 1•12 years ago
|
||
Is this non-chrome code so it's not seeing the [ChromeOnly] QueryInterface on there?
![]() |
Assignee | |
Comment 2•12 years ago
|
||
More to the point, should xrays from the xbl compartment see ChromeOnly methods?
Updated•12 years ago
|
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
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
![]() |
Assignee | |
Comment 5•12 years ago
|
||
Bobby, any idea how to test this sanely?
Attachment #746181 -
Flags: review?(bobbyholley+bmo)
![]() |
Assignee | |
Updated•12 years ago
|
Assignee: nobody → bzbarsky
![]() |
Assignee | |
Updated•12 years ago
|
Whiteboard: [need review]
Comment 6•12 years ago
|
||
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+
![]() |
Assignee | |
Comment 7•12 years ago
|
||
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-
![]() |
Assignee | |
Comment 8•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 9•12 years ago
|
||
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•12 years ago
|
||
Is there a workaround I can use in Flashblock in the meantime, or wait for a fix to land before uplift to Aurora?
![]() |
Assignee | |
Comment 11•12 years ago
|
||
> 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•12 years ago
|
||
Andrea will be looking at form, but AIUI you're not blocked on that?
Flags: needinfo?(amarchesini)
Flags: needinfo?(Ms2ger)
![]() |
||
Comment 14•11 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 15•11 years ago
|
||
Attachment #746801 -
Flags: review?(bobbyholley+bmo)
![]() |
Assignee | |
Updated•11 years ago
|
Attachment #746181 -
Attachment is obsolete: true
Comment 16•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #746801 -
Flags: review?(peterv) → review+
![]() |
Assignee | |
Comment 17•11 years ago
|
||
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?
![]() |
Assignee | |
Comment 18•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/049889b25a79
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Comment 19•11 years ago
|
||
Backed out for mochitest-bc, mochitest-other, and xpcshell orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/9182c3e6a967
![]() |
Assignee | |
Comment 20•11 years ago
|
||
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.
![]() |
Assignee | |
Comment 21•11 years ago
|
||
Should probably get another review
Attachment #749614 -
Flags: review?(peterv)
![]() |
Assignee | |
Comment 22•11 years ago
|
||
Try run for that last is green: https://tbpl.mozilla.org/?tree=Try&rev=9e00fd5bb547
![]() |
Assignee | |
Updated•11 years ago
|
Attachment #746801 -
Attachment is obsolete: true
Attachment #746801 -
Flags: approval-mozilla-aurora?
![]() |
Assignee | |
Updated•11 years ago
|
Whiteboard: [need review]
Updated•11 years ago
|
Attachment #749614 -
Flags: review?(peterv) → review+
![]() |
Assignee | |
Comment 23•11 years ago
|
||
Let's try that again: https://hg.mozilla.org/integration/mozilla-inbound/rev/55620ca9673c
Whiteboard: [need review]
![]() |
Assignee | |
Updated•11 years ago
|
Attachment #749614 -
Flags: approval-mozilla-aurora?
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/55620ca9673c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Attachment #749614 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
![]() |
Assignee | |
Comment 25•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c177454b9e74
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•