Last Comment Bug 827546 - |non editable element|.QueryInterface(Components.interfaces.nsIDOMNSEditableElement) does not throw anymore
: |non editable element|.QueryInterface(Components.interfaces.nsIDOMNSEditableE...
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 20 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla21
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 821606
  Show dependency treegraph
 
Reported: 2013-01-07 15:09 PST by Alice0775 White
Modified: 2013-04-22 15:41 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
fixed


Attachments
sample html (64 bytes, text/html)
2013-01-07 15:09 PST, Alice0775 White
no flags Details
v1 (3.06 KB, patch)
2013-01-11 05:37 PST, Peter Van der Beken [:peterv]
bzbarsky: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Alice0775 White 2013-01-07 15:09:25 PST
Created attachment 698894 [details]
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
Comment 1 :Ehsan Akhgari 2013-01-07 15:12:12 PST
This is not good.  There's code which uses this QI to figure out whether it's dealing with editable content.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2013-01-07 17:27:47 PST
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.
Comment 3 :Ehsan Akhgari 2013-01-07 17:39:04 PST
Looks like some 80 extensions are affected...

<https://mxr.mozilla.org/addons/search?string=nsIDOMNSEditableElement>
Comment 4 :Ehsan Akhgari 2013-01-07 17:41:23 PST
(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.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2013-01-07 17:44:44 PST
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.
Comment 6 Jean-Yves Perrier [:teoli] 2013-01-11 02:32:39 PST
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)
Comment 7 Masatoshi Kimura [:emk] 2013-01-11 04:00:42 PST
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.
Comment 8 Peter Van der Beken [:peterv] 2013-01-11 05:37:37 PST
Created attachment 701030 [details] [diff] [review]
v1
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2013-01-11 09:06:52 PST
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 10 Boris Zbarsky [:bz] (still a bit busy) 2013-01-11 09:08:52 PST
Comment on attachment 701030 [details] [diff] [review]
v1

r=me
Comment 11 Peter Van der Beken [:peterv] 2013-01-14 02:45:37 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b615d3bae9c
Comment 12 Ed Morley [:emorley] 2013-01-14 09:06:28 PST
https://hg.mozilla.org/mozilla-central/rev/0b615d3bae9c
Comment 13 Peter Van der Beken [:peterv] 2013-01-21 09:52:57 PST
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
Comment 14 bhavana bajaj [:bajaj] 2013-01-22 13:14:51 PST
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
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-01-23 12:08:24 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/9d9abb80b40c
Comment 16 glorychan1981 2013-04-22 15:37:05 PDT
(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.
Comment 17 glorychan1981 2013-04-22 15:41:03 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.