Open Bug 1061578 Opened 10 years ago Updated 2 years ago

Node.isDefaultNamespace(), Node.lookupNamespaceURI() and Node.lookupPrefix() are wrong

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
34 Branch
Platform:
x86_64
Windows 7
Type:
defect

Tracking

()

People

(Reporter: crimsteam, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Patch v2
6.90 KB, patch
peterv
: review-
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20140716183446

Steps to reproduce:

These three methods have wrong behaviour when we compare to algorithms in DOM:
http://dom.spec.whatwg.org/#dom-node-isdefaultnamespace
http://dom.spec.whatwg.org/#dom-node-lookupnamespaceuri
http://dom.spec.whatwg.org/#dom-node-lookupprefix

Other browser aren't ideal, but in some case support this method more correctly than Firefox.
Component: General → DOM: Core & HTML
What exactly is the issue with isDefaultNamespace?  Can you point to an actual testcase where our implementation doesn't match other browsers or the spec?
Flags: needinfo?(crimsteam)
Sure, look this:

<!DOCTYPE html>
<body>
<script>

	document.write(document.isDefaultNamespace(""));
	document.write("<br>");
	document.write(document.body.isDefaultNamespace(null));
	document.write("<br>");
	document.write(document.isDefaultNamespace("http://www.w3.org/1999/xhtml"));

</script>
</body>

Result:
- Firefox34: true, true, false
- IE11 and Chrome37 and Opera(Presto): true, true, false

Pers spec. second probably is correct.

Next case:

<!DOCTYPE html>
<body>
<script>

	var newDiv = document.createElementNS("www.test.com", "div");
	var newTest = document.createElementNS("", "test");
	var newP = document.createElement("p");
	
	newDiv.appendChild(newTest);
	newDiv.appendChild(newP);
	
	document.write(newDiv.isDefaultNamespace("www.test.com"));
	document.write("<br>");
	document.write(newTest.isDefaultNamespace("www.test.com"));
	document.write("<br>");
	document.write(newP.isDefaultNamespace("www.test.com"));

</script>
</body>

Result:
- Firefox34: false,false, false
- Chrome37: true, false, false
- Opera(Presto): true, true, false
- IE11: true, true, true

Pers spec. what IE11 do probably is correct.

Next case:

<!DOCTYPE html>
<body>
<script>

	var docImp = document.implementation;

	document.write(docImp .createDocument(null, "", null).isDefaultNamespace(""));
	document.write("<br>");
	document.write(docImp .createDocument("www.test.com", "", null).isDefaultNamespace("www.test.com"));
	document.write("<br>");
	document.write(docImp.createDocument("www.test.com", "root", null).isDefaultNamespace("www.test.com"));

</script>
</body>

Result:
- Firefox34: true,false, false
- Chrome37 and Opera(Presto): false, false, true
- IE11: false, (hang on this because I use "" what per spec its correct), true

Per spec should be true, false, true.

Only 3 cases because results are horrible and I supposed that implementation are wrong and making more now don't have sense. The same is for two other methods.
Flags: needinfo?(crimsteam)
So for your first isDefaultNamespace case:

>- Firefox34: true, true, false
>- IE11 and Chrome37 and Opera(Presto): true, true, false

Those are ... the same.

For your second case, looking at the

  var newDiv = document.createElementNS("www.test.com", "div");

case, I guess in the spec we end up hitting step 1 of the Element case in http://dom.spec.whatwg.org/#locate-a-namespace and are supposed to return "www.test.com" for this element.  But it's not clear to me why we'd want to do that.  In Gecko we simply look for xmlns attributes on ancestors, which I expect explains all the remaining differences you see.

Why is the spec looking at the actual namespaces of elements, exactly?  I guess the real question is what the use cases for these methods are...
Flags: needinfo?(annevk)
(In reply to Boris Zbarsky [:bz] from comment #3)
> So for your first isDefaultNamespace case:
> 
> >- Firefox34: true, true, false
> >- IE11 and Chrome37 and Opera(Presto): true, true, false
> 

Sorry, should be:
IE11 and Chrome37 and Opera(Presto): false, false, true
bz, I'm not sure. There's bug 312019 and bug 505178 mentioned in the source of the specification. I don't really care how we implement any of the namespace stuff to be honest. We could even try to deprecate them for all I care.

I am however willing to write out acceptable algorithms, including reverse engineering what Gecko does if we consider that to be better, and don't want to deal with the aforementioned bugs. However, would have to check with other browsers if they are willing to match.
Flags: needinfo?(annevk)
Maybe some telemetry could help, this methods were introduced in DOM Level 3, how they were implemented in browsers we can see above, so deprecate them is worth considering than trying to repair.
I tried rewriting lookupNamespaceURI to match the spec and it broke Firefox (hang on start).  It seems we at least use these methods internally somehow and depend on our current behavior.  I don't have a clue what they're meant to be used for.
I don't see any scripted callers of lookupNamespaceURI in our tree outside of tests.

In C++, we seem to have callers in:

1)  nsContentUtils::SplitQName (used mostly from XSLT, XBL, and SVGAnimationElement)
2)  nsXBLPrototypeBinding::ResolveBaseBinding which uses it for dealing with
    extends="foo:bar".
3)  Some XPath stuff.

My guess given the lack of other information here (like what the actual behavior changes made were) is that the behavior of ResolveBaseBinding got changed.
Attached patch Patch (obsolete) — — Splinter Review 
I didn't look at LookupPrefix, since it seems to be passing the upstream tests.  If it's also wrong, someone should write some better tests to demonstrate that and we can upstream them.  I didn't try to figure them out myself because I don't get at all what these functions are supposed to do.

I converted two C++ callers to use LookupNamespaceURIInternal, which behaves the same as it always did.  To do so I had to make GetNameSpaceElement public.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c11ce07ed0b3
Assignee: nobody → ayg
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8672381 - Flags: review?(bzbarsky)
Comment on attachment 8672381 [details] [diff] [review]
Patch

Review of attachment 8672381 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsINode.cpp
@@ +1178,5 @@
> +    GetParentElement()->LookupNamespaceURI(prefix, aNamespaceURI);
> +    return;
> +  }
> +
> +  if (!IsContent()) {

This should be if (IsNodeOfType(eDOCUMENT)).  I forgot Attr is node but not content!
Comment on attachment 8672381 [details] [diff] [review]
Patch

Review of attachment 8672381 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xbl/nsXBLPrototypeBinding.cpp
@@ +1653,5 @@
>  
>    if (!prefix.IsEmpty()) {
> +    nsCOMPtr<Element> nsElem = mBinding->GetNameSpaceElement();
> +    if (nsElem) {
> +      mBinding->LookupNamespaceURIInternal(prefix, nameSpace);

Obviously mBinding here needs to be nsElem.

::: dom/xslt/xpath/XPathEvaluator.cpp
@@ +226,5 @@
>          } else {
> +            nsCOMPtr<Element> nsElem = mResolverNode->GetNameSpaceElement();
> +            if (nsElem) {
> +              nsElem->LookupNamespaceURIInternal(prefix, ns);
> +            }

And this conditional needs to change to

  if (!nsElem ||
      NS_FAILED(nsElem->LookupNamespaceURIInternal(prefix, ns))) {
      SetDOMStringToNull(ns);
  }

These changes fix the try failures.
Comment on attachment 8672381 [details] [diff] [review]
Patch

I would really rather not have two different algorithms for lookupNamespaceURI in the tree.  What aspect of the old behavior are the C++ consumers relying on, exactly, and why can't we fix that?
Attachment #8672381 - Flags: review?(bzbarsky) → review-
Some differences between us and the spec, from code inspection and test failures:

1) We special-case "xml" and "xmlns", the spec does not.

2) The spec returns the element's namespace URL if the element's own prefix is the given prefix, even if there's no xmlns attribute anywhere with that prefix.

3) The spec checks only for attributes in the XMLNS namespace with prefix "xmlns" (or with no prefix, if the method's argument is null).  We don't care what the attribute's prefix is, only the namespace and local name.

If you want me to fix the existing code, do you have any suggestions for how to go about doing so?  I don't have any idea what this is being used for, practically speaking.
Flags: needinfo?(bzbarsky)
I don't either, sorry.  Someone needs to sit down and trace through the callers, including both in our C++ code and in our chrome.
Flags: needinfo?(bzbarsky)
Attached patch Patch v2 — — Splinter Review 
It looks like this passes try after all, with only one change in a caller: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d09a127a6723
Attachment #8672381 - Attachment is obsolete: true
Attachment #8741326 - Flags: review?(bzbarsky)
(Aryeh Gregor https://www.w3.org/Bugs/Public/show_bug.cgi?id=27456#c4)
> Testing <http://w3c-test.org/dom/nodes/Node-lookupNamespaceURI.html> in
> Firefox, Chrome, and IE 11 shows that all fail a number of tests, but
> there's no single test that all three fail.  This suggests that the current
> spec is probably good enough to implement, given the marginal use of the
> feature.
> 
> (All three browsers pass all the lookupPrefix tests.  I didn't check to see
> how thorough the tests are or inspect Gecko source code to see whether we
> deviate significantly from the spec.)
Comment on attachment 8741326 [details] [diff] [review]
Patch v2

Peter, could you take a look at this, please?
Attachment #8741326 - Flags: review?(bzbarsky) → review?(peterv)
Comment on attachment 8741326 [details] [diff] [review]
Patch v2

Review of attachment 8741326 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/FragmentOrElement.cpp
@@ -274,5 @@
>  
> -  if (aNamespacePrefix.EqualsLiteral("xmlns")) {
> -    // Special-case for xmlns prefix
> -    aNamespaceURI.AssignLiteral("http://www.w3.org/2000/xmlns/");
> -    return NS_OK;

This changes LookupNamespaceURIInternal, but that's also used by nsContentUtils::SplitQName which should probably follow the Namespaces in XML spec. So in that case we do need to special-case xml and xmlns. You could just move this code into SplitQName.

::: dom/xslt/xpath/XPathEvaluator.cpp
@@ +221,5 @@
>          if (rv.Failed()) {
>              return rv.StealNSResult();
>          }
>      } else {
> +        mResolverNode->LookupNamespaceURI(prefix, ns);

I have no idea why you're making this change, could you explain what spec requires it?
Attachment #8741326 - Flags: review?(peterv) → review-
Comment on attachment 8741326 [details] [diff] [review]
Patch v2

Review of attachment 8741326 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/FragmentOrElement.cpp
@@ +283,5 @@
> +    }
> +
> +
> +    RefPtr<Attr> attr =
> +      // XXX fix me

Please file a bug. I guess GetAttributeNodeNS should call GetAttributeMap and return null if that returns null.

@@ +295,5 @@
> +      }
> +      return NS_OK;
> +    }
> +
> +    if (DOMStringIsNull(prefix) &&

I think it makes more sense to do the null-check before calling GetAttributeNodeNS above. So only call GetAttributeNodeNS if the prefix is non-null and else do the code below.

@@ +314,3 @@
>    }
>  
> +  if (IsNodeOfType(eDOCUMENT_FRAGMENT) ||

Be consistent, just check for |NodeType() == nsIDOMNode::DOCUMENT_FRAGMENT_NODE|. It's probably also faster.
(In reply to Peter Van der Beken [:peterv] from comment #19)
> This changes LookupNamespaceURIInternal, but that's also used by
> nsContentUtils::SplitQName which should probably follow the Namespaces in
> XML spec. So in that case we do need to special-case xml and xmlns. You
> could just move this code into SplitQName.

Do you want just these two special-cases in SplitQName, or the whole old implementation to be copied there?

> ::: dom/xslt/xpath/XPathEvaluator.cpp
> @@ +221,5 @@
> >          if (rv.Failed()) {
> >              return rv.StealNSResult();
> >          }
> >      } else {
> > +        mResolverNode->LookupNamespaceURI(prefix, ns);
> 
> I have no idea why you're making this change, could you explain what spec
> requires it?

Before the change, if prefix was "xmlns" and mResolver was not null, we would wind up calling LookupNamespaceURIInternal somehow, and get back the XML namespace.  Now that that special case is removed, we get something else, and it breaks tests.  This change restores the special case for this caller to maintain previous behavior.
Flags: needinfo?(peterv)
(In reply to :Aryeh Gregor (working until May 8) from comment #21)
> Do you want just these two special-cases in SplitQName, or the whole old
> implementation to be copied there?

I wanted just the special cases, but I looked at all the callers and maybe we should just ignore it. Using xml or xmlns with xbl:inherits or SVG animations would be weird (turns out XSLT doesn't actually call this).

> Before the change, if prefix was "xmlns" and mResolver was not null, we
> would wind up calling LookupNamespaceURIInternal somehow, and get back the
> XML namespace.  Now that that special case is removed, we get something
> else, and it breaks tests.  This change restores the special case for this
> caller to maintain previous behavior.

That change is wrong, not all XPathNSResolvers use a node to map prefixes to namespaces and we only want to resolve xml for the ones that do. This is not trivial to solve, since we don't have a concrete implementation for XPathNSResolver. I guess one way would be to change all WebIDL methods that take a XPathNSResolver to take (XPathNSResolver or Node) instead.
Flags: needinfo?(peterv)
(In reply to Peter Van der Beken [:peterv:] from comment #22)
> That change is wrong, not all XPathNSResolvers use a node to map
> prefixes to namespaces and we only want to resolve xml for the
> ones that do. This is not trivial to solve, since we don't have
> a concrete implementation for XPathNSResolver. I guess one way
> would be to change all WebIDL methods that take a
> XPathNSResolver to take (XPathNSResolver or Node) instead.

I don't understand this yet.  Are you saying that previously the mResolver case would not necessarily special-case an xml prefix?  Because at least in some cases I'm pretty sure it did, insofar as there were test failures that were fixed by the change under discussion.  I assume it somehow got to LookupNamespaceURIInternal.  The problem is this won't be true for all resolvers?  Couldn't I find the resolver implementation that really does want this behavior and change just it instead, in that case?
Flags: needinfo?(peterv)
XPathNSResolver is just a callback interface. Passing in anything that has a lookupNamespaceURI property or just a JS function with the same signature will work. Node has a lookupNamespaceURI function, so it is accepted for a XPathNSResolver. If one passes in a node, then it's expected that the xml prefix is special-cased. If one passes in something else then that has to deal with the xml prefix. There is no special resolver implementation to change, hence my suggestion at the end of comment 22. The other alternative is to add a new WebIDL interface (NodeXPathNSResolver), make createNSResolver return that and do the xml special case in that implementation.
Flags: needinfo?(peterv)
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Blocks: 505178
Priority: -- → P5
See Also: → 1726705
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: