Closed Bug 827546 Opened 11 years ago Closed 11 years ago

|non editable element|.QueryInterface(Components.interfaces.nsIDOMNSEditableElement) does not throw anymore

Categories

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

20 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- unaffected
firefox20 + fixed
firefox21 --- fixed

People

(Reporter: alice0775, Assigned: peterv)

References

Details

(Keywords: addon-compat, dev-doc-needed, regression)

Attachments

(2 files)

Attached file sample html
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/605ae260b7c8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130107 Firefox/20.0 ID:20130107030932

|non editable element|.QueryInterface(Components.interfaces.nsIDOMNSEditableElement) does not throw an error anymore

Steps to reproduce:
1. Open Attached html

<input type="text" id="editable">
<div id="non-editable"></div>

2. Open Error Console
3. Evaluate the following code

top.opener.content.document.getElementById("non-editable").QueryInterface(Components.interfaces.nsIDOMNSEditableElement)

Actual results:
  [object HTMLDivElement]

Expected results:
  Throw an error:

  Error: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]


Regression window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/98dddd0da122
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121221 Firefox/20.0 ID:20121221100222
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/06661265d9e9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121221 Firefox/20.0 ID:20121221102122
Pushlog;
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=98dddd0da122&tochange=06661265d9e9

Triggered by: bug 821606
This is not good.  There's code which uses this QI to figure out whether it's dealing with editable content.
Can we get that code fixed somehow?   The goal here is to remove the various XPCOM goop hanging off elements as time goes on...  Shouldn't this code just be using instanceof anyway, not QI, if it's testing whether the interface is supported?

Certainly I don't think we plan to support a useful QI on WebIDL objects forever; the only question is what part of it we drop when.
Looks like some 80 extensions are affected...

<https://mxr.mozilla.org/addons/search?string=nsIDOMNSEditableElement>
(In reply to :Ehsan Akhgari from comment #3)
> Looks like some 80 extensions are affected...
> 
> <https://mxr.mozilla.org/addons/search?string=nsIDOMNSEditableElement>

Ah, well, that's all occurrences of that name, of course.  The real problem is only in QI calls which are meant to do an editability check, so that 80 number is an overestimate.
Well, not quite.

For example, the very first one in that list is doing:

  node instanceof Components.interfaces.nsIDOMNSEditableElement

which continues to work fine for now.  And a bunch of others are calling this on known HTMLInputElement elements and whatnot.

But yes, some of those 80 might be affected.  I suppose we can put in a hack for this thing (or maybe for all interfaces on elements?) for now and start working on getting these deprecated now that we have a way better solution.
Added dev-doc-needed to remind me to document this in Fx 20 for dev, depending of the outcome of the analysis. 

Small question: does this affect only add-ons, or may it also hits Web sites? (I don't think so but want to double check)
Keywords: dev-doc-needed
For XPConnected objects, QueryInterface() was available from Web content. For WebDIL binding objects, it is available only from the chrome (but it will be completely removed someday). So in theory, it may affect the Web sites. But in practice, such site would be very very rare. Those sites must be fixed anyway because they are depending on a Firefox specific feature.
Attached patch v1Splinter Review
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attachment #701030 - Flags: review?(bzbarsky)
To add to what comment 7 said:  Web sites used to be able to make this QI call and have it throw or not depending on the element type.  But the actual methods on the interface are only callable without throwing from chrome, so the chance of web sites doing anything with this is low.
Comment on attachment 701030 [details] [diff] [review]
v1

r=me
Attachment #701030 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/0b615d3bae9c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 701030 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 821606
User impact if declined: add-on breakage
Testing completed (on m-c, etc.): landed on m-c on Jan 14, has testcase
Risk to taking this patch (and alternatives if risky): low-risk
String or UUID changes made by this patch: none
Attachment #701030 - Flags: approval-mozilla-aurora?
Comment on attachment 701030 [details] [diff] [review]
v1

Approving the low risk patch on aurora to avoid add-on/web site breakage caused due to bug 821606 in FF20
Attachment #701030 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Masatoshi Kimura [:emk] from comment #7)
> For XPConnected objects, QueryInterface() was available from Web content.
> For WebDIL binding objects, it is available only from the chrome (but it
> will be completely removed someday). So in theory, it may affect the Web
> sites. But in practice, such site would be very very rare. Those sites must
> be fixed anyway because they are depending on a Firefox specific feature.
(In reply to Masatoshi Kimura [:emk] from comment #7)
> For XPConnected objects, QueryInterface() was available from Web content.
> For WebDIL binding objects, it is available only from the chrome (but it
> will be completely removed someday). So in theory, it may affect the Web
> sites. But in practice, such site would be very very rare. Those sites must
> be fixed anyway because they are depending on a Firefox specific feature.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.