Closed
Bug 693615
Opened 13 years ago
Closed 13 years ago
element.lookupNamespaceURI(null) in text/html page cause NS_ERROR_FAILURE error on XrayWrappers
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: xKhorasan, Assigned: imphil)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
5.87 KB,
patch
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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...
Blocks: 659053
Status: UNCONFIRMED → NEW
Component: XPConnect → DOM
Ever confirmed: true
Keywords: regression
QA Contact: xpconnect → general
Assignee | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #579164 -
Flags: feedback?(bzbarsky)
![]() |
||
Comment 4•13 years ago
|
||
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?
Attachment #579164 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 5•13 years ago
|
||
thanks Boris, now with LookupNamespaceURIInternal as method name.
Assignee: nobody → mail
Attachment #579164 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #579260 -
Flags: review?(jonas)
![]() |
||
Comment 6•13 years ago
|
||
Comment on attachment 579260 [details] [diff] [review] patch with method name LookupNamespaceURIInternal Could use a test too....
Assignee | ||
Comment 7•13 years ago
|
||
Same patch as before, but now with a mochitest. Thanks to mrbkap for the idea with deleting the prototype!
Attachment #579260 -
Attachment is obsolete: true
Attachment #579260 -
Flags: review?(jonas)
Attachment #579308 -
Flags: review?(jonas)
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.
Assignee | ||
Updated•13 years ago
|
Attachment #579308 -
Flags: review?(jonas) → review?(bzbarsky)
![]() |
||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
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.
Attachment #579308 -
Flags: review?(bzbarsky) → review+
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).
Assignee | ||
Comment 12•13 years ago
|
||
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.
Attachment #579308 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•13 years ago
|
||
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.
Assignee | ||
Comment 14•13 years ago
|
||
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.
Attachment #582918 -
Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7d75e0057998
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•