Closed Bug 693615 Opened 11 years ago Closed 11 years ago
.lookup Namespace URI(null) in text/html page cause NS _ERROR _FAILURE error on Xray Wrappers
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...
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 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+
thanks Boris, now with LookupNamespaceURIInternal as method name.
Comment on attachment 579260 [details] [diff] [review] patch with method name LookupNamespaceURIInternal Could use a test too....
Same patch as before, but now with a mochitest. Thanks to mrbkap for the idea with deleting the prototype!
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.
Attachment #579308 - Flags: review?(jonas) → review?(bzbarsky)
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 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).
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
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.
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.