Last Comment Bug 693615 - element.lookupNamespaceURI(null) in text/html page cause NS_ERROR_FAILURE error on XrayWrappers
: element.lookupNamespaceURI(null) in text/html page cause NS_ERROR_FAILURE err...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla11
Assigned To: Philipp Wagner [:imphil]
:
Mentors:
Depends on:
Blocks: 659053 694754
  Show dependency treegraph
 
Reported: 2011-10-11 08:18 PDT by :xKhorasan
Modified: 2011-12-20 07:00 PST (History)
6 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch proposal v1 (method name subject to change) (3.48 KB, patch)
2011-12-05 17:36 PST, Philipp Wagner [:imphil]
bzbarsky: feedback+
Details | Diff | Splinter Review
patch with method name LookupNamespaceURIInternal (3.84 KB, patch)
2011-12-06 00:55 PST, Philipp Wagner [:imphil]
no flags Details | Diff | Splinter Review
patch, now with testcase (5.69 KB, patch)
2011-12-06 08:39 PST, Philipp Wagner [:imphil]
bzbarsky: review+
Details | Diff | Splinter Review
patch for mozilla-central r=bz (6.51 KB, patch)
2011-12-19 12:26 PST, Philipp Wagner [:imphil]
no flags Details | Diff | Splinter Review
patch now really for mozilla-central r=bz (5.87 KB, patch)
2011-12-20 06:56 PST, Philipp Wagner [:imphil]
no flags Details | Diff | Splinter Review

Description :xKhorasan 2011-10-11 08:18:03 PDT
User Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0a1) Gecko/20111009 Firefox/10.0a1
Build ID: 20111009034931

Using lookupNamespaceURI() with XrayWrapper-ed element in text/html page causes NS_ERROR_FAILURE error.

Therefore, scripts which use XrayWrapper-ed element, such as
- Greasemonkey or Scriptish script
- Add-on SDK's content script
cause error when they use lookupNamespaceURI on text/html page.
For example, this problem affects AutoPagerize[1].

[1] http://userscripts.org/scripts/show/8551


Reproducible: Always


Step to reproduce:

1. Set 'devtools.errorconsole.enabled' preference to true in about:config, then restart Firefox.
2. Input 'http://misc-xkyrgyzstan.dotcloud.com/html/lookupnamespaceuri' in the address bar, then press Enter.
3. Press Ctrl + Shift + K to launch web console.
4. Input 'document.lookupNamespaceURI(null)' in console, then press Enter to evaluate the code.
5. Input 'document.getElementsByTagName('div')[0].lookupNamespaceURI(null)' in console, then press Enter to evaluate the code.
6. Press Ctrl + Shift + K to close web console.
6. Press Ctrl + Shift + J to launch error console, then press Alt + C to clear messages.
8. Input 'top.opener.gBrowser.contentDocument.defaultView.document.lookupNamespaceURI(null)' in console, then press Alt + V to evaluate the code, and delete inputed code in console.
9. Input 'top.opener.gBrowser.contentDocument.defaultView.document.getElementsByTagName('div')[0].lookupNamespaceURI(null)' in console, then press Alt + V to evaluate the code.


Actual results:

null
null
null
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLDivElement.lookupNamespaceURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: javascript:%20top.opener.gBrowser.contentDocument.defaultView.document.getElementsByTagName('div')[0].lookupNamespaceURI(null) :: <TOP_LEVEL> :: line 1"  data: no]


Expected results:

null
null
null
null


Notes:

result of Firefox 6.0.2 (User Agent: Mozilla/5.0 (X11; Linux i686; rv:6.0.2) Gecko/20100101 Firefox/6.0.2)

null
null
null
null

result of Firefox 7.0.1 (User Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20100101 Firefox/7.0.1)

null
null
null
エラー: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLDivElement.lookupNamespaceURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: javascript:%20top.opener.gBrowser.contentDocument.defaultView.document.getElementsByTagName('div')[0].lookupNamespaceURI(null) :: <TOP_LEVEL> :: line 1"  data: no]

So it seems that it is a regression.
Comment 1 Boris Zbarsky [:bz] 2011-10-11 11:58:53 PDT
This is actually a DOM bug... and it was introduced in bug 659053.

The non xraywrapper codepath calls nsINode::LookupNamespaceURI (directly, via quickstubs).

The xraywrapper codepath used to call nsNode3Tearoff::LookupNamespaceURI which then called nsINode::LookupNamespaceURI.  But the changes in bug 659053 added a LookupNamespaceURI directly on elements which forwards to nsGenericHTMLElement, like all the other DOM gunk.  So it ends up calling nsIContent::LookupNamespaceURI directly, which is the wrong thing to do.

I think the right fix here is to just rename nsIContent::LookupNamespaceURI to something else...
Comment 2 Philipp Wagner [:imphil] 2011-12-05 17:33:52 PST
I have the same problem that calls LookupNamespaceURI from some XForms code. 

Before the DOM refractoring (bug 659053), it was like this (like bz said):

#0  nsIContent::LookupNamespaceURI
    at content/base/src/nsGenericElement.cpp:942
#1  nsINode::LookupNamespaceURI
    at content/base/src/nsGenericElement.cpp:786
#2  nsNode3Tearoff::LookupNamespaceURI
    at content/base/src/nsGenericElement.cpp:1309
#3  nsXFormsModelElement::GetTypeFromNode
    at extensions/xforms/nsXFormsModelElement.cpp:2027

Now it's like this:

#0  nsIContent::LookupNamespaceURI
    at ontent/base/src/nsGenericElement.cpp:1378
#1  nsXMLElement::LookupNamespaceURI
    at content/base/src/../../xml/content/src/nsXMLElement.h:58
[-> only a forward here to the one above]
#2  nsXFormsModelElement::GetTypeFromNode
    at extensions/xforms/nsXFormsModelElement.cpp:2026

Without having much idea about all the side-effects, I see three possible solutions:
a) Rename nsIContent::LookupNamespaceURI so that it doesn't override nsINode::LookupNamespaceURI any more (bz's suggestion)
b) Add the "return an empty string on any error and NS_OK"-code which is inside nsINode::LookupNamespaceURI (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#1061) to nsIContent::LookupNamespaceURI - but I don't know how that might break other things.
c) Remove the forwarding in nsXMLElement to nsGenericElement, but forward to nsINode instead

For solution a I attached a sample patch, a better name for the renamed method is still needed (perhaps lookupNamespaceURIInternal or something like that?); with that patch, the STR in this bug work as expected and the code in XForms does as well, so it seems to solve the problem.
Comment 3 Philipp Wagner [:imphil] 2011-12-05 17:36:24 PST
Created attachment 579164 [details] [diff] [review]
patch proposal v1 (method name subject to change)
Comment 4 Boris Zbarsky [:bz] 2011-12-05 19:11:58 PST
Comment on attachment 579164 [details] [diff] [review]
patch proposal v1 (method name subject to change)

"Internal" is probably better than "2".  Other than that, looks ok, but have Jonas review, please?
Comment 5 Philipp Wagner [:imphil] 2011-12-06 00:55:32 PST
Created attachment 579260 [details] [diff] [review]
patch with method name LookupNamespaceURIInternal

thanks Boris, now with LookupNamespaceURIInternal as method name.
Comment 6 Boris Zbarsky [:bz] 2011-12-06 00:58:32 PST
Comment on attachment 579260 [details] [diff] [review]
patch with method name LookupNamespaceURIInternal

Could use a test too....
Comment 7 Philipp Wagner [:imphil] 2011-12-06 08:39:58 PST
Created attachment 579308 [details] [diff] [review]
patch, now with testcase

Same patch as before, but now with a mochitest. Thanks to mrbkap for the idea with deleting the prototype!
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-16 01:26:31 PST
I don't quite understand what's going wrong here, but bz's review is absolutely enough for me. The change looks ok to me but I don't quite understand why it fixes things.
Comment 9 Boris Zbarsky [:bz] 2011-12-16 05:48:36 PST
Jonas, the issue is that we have the following methods:

1) nsINode::LookupNamespaceURI (non-virtual, never throws)
2) nsIContent::LookupNamespaceURI (non-virtual, can throw)
3) nsIDOMNode::LookupNamespaceURI (pure-virtual)
4) ns***Element::LookupNamespaceURI (on each concrete element class, implementing the
   nsIDOMNode thing, virtual).

Quickstubs call #1, which checks whether we have a namespace element and if we do calls #2 on it.  If there is no namespace element or #2 throws, it returns null.

The XPConnect code used to call #1 as well, via Node3Tearoff.  But now it calls #4, which does the usual NS_FORWARD thing and lands directly in #2, which can throw.

So the proposed solution is to change the name of #2, so the forward from #4 will land in #1 instead, just like quickstubs.
Comment 10 Boris Zbarsky [:bz] 2011-12-16 05:58:33 PST
Comment on attachment 579308 [details] [diff] [review]
patch, now with testcase

r=me, but that test will likely break with new dom bindings.  We'll fix it when we get to that, I guess.
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-16 13:02:21 PST
Ah! I'm with you. Sounds good.

Please add a comment in the test describing why it does what it does (i.e. what you're trying to test).
Comment 12 Philipp Wagner [:imphil] 2011-12-19 12:26:27 PST
Created attachment 582918 [details] [diff] [review]
patch for mozilla-central r=bz

now updated for mozilla-central and with comment describing what we test (as requested by Jonas). 

Went through try (https://tbpl.mozilla.org/?tree=Try&rev=9db0b4951321)
which looks good to me, all test failures seems to be random orange.
Comment 13 Philipp Wagner [:imphil] 2011-12-20 06:43:33 PST
I'm sorry to be spamming here, but could someone (bz, jonas?) get this checked in into mozilla-central so it makes the uplift today on time? I cannot do it myself due to missing commit access.
Comment 14 Philipp Wagner [:imphil] 2011-12-20 06:56:49 PST
Created attachment 583151 [details] [diff] [review]
patch now really for mozilla-central r=bz

oh, I took the patch from my release repository instead of the mozilla-central one. Now the patch applies cleanly to mozilla-central as of now.
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-20 07:00:38 PST
https://hg.mozilla.org/mozilla-central/rev/7d75e0057998

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