16603 - DOM doesn't throw INVALID_CHARACTER_ERR for arguments with invalid characters

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

DESCRIPTION:  The three functions createAttribute(),
createProcessingInstruction(), and createEntityReference() seem never to throw
an INVALID_CHARACTER_ERR exception.  The latter two functions are XML-only (they
correctly throw the NOT_SUPPORTED_ERR exception in HTML).

STEPS TO REPRODUCE:
 * Load http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/exceptions
 * Hit "test"
 * Load http://www.fas.harvard.edu/~dbaron/dom/test/one-core-html/exceptions
 * Hit "test"

ACTUAL RESULTS:
 * In the XML test page, the tests (2) through (7) are red, because exceptions
are not thrown.  In the HTML test page, test (4) is red.  (By test, I mean a
line that is either red or green.)

EXPECTED RESULTS:
 * Exceptions are thrown and lines are green.

DOES NOT WORK CORRECTLY ON:
 * Linux, viewer, 1999-10-15-11-M11

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Element::setAttribute has the exact same problem.  This causes the first test
under the section in the above pages testing the Element interface (near the end
of the pages) to incorrectly be red.

Changing title from "3" to "4".

Comment 2

[vidur (gone)]

In an attempt to get my bug list in order again, marking all the bugs I have
currently as ASSIGNED.

Comment 3

[buster]

in DOM Level 2, I believe the attribute name must be an NCName, defined at
http://www.w3.org/TR/REC-xml-names/#NT-NCName
given by
NCName ::= (Letter | '_') (NCNameChar)*
which I take to mean (any number of letters and underscores)

"Letter" is defined at:
http://www.w3.org/TR/REC-xml#NT-Letter

This check should go in nsDocument::CreateAttribute(), and return
NS_ERROR_DOM_INVALID_CHARACTER_ERR if the string is 0 length, or if the string
contains any character that is not a letter or an underscore.  Actually, the
check should be written as a utility routine, because it needs to be called from
several other places.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Those rules apply to XML, but what are the rules for SGML-based HTML?

Comment 5

[jst]

We probably need a simple HTML name validator function in layout, something
that checks for white spaces and other odd chars, won't be perfect but hey,
it's HTML ;).

Comment 6

[vidur (gone)]

It's a bit more complicated than buster implies, but I guess it's possible for 
us to create a XML name validator in layout. I'd prefer being able to get the 
service from the parser if possible. I suspect that we'd be safe using the XML 
name validation rules for HTML as well. 

Related to bug 16591. Can we consolidate the bugs?

Comment 7

[Johnny Stenback (:jst)]

*** Bug 34995 has been marked as a duplicate of this bug. ***

Comment 8

[Bob Clary [:bc] (inactive)]

This bug seems to affect most if not all methods that create Nodes:

DOMImplementation.createDocument
DOMImplementation.createDocumentType
Document.createAttribute (already reported)
Document.createAttributeNS
Document.createElement
Document.createElementNS
Document.createEntityReference (already reported)
Document.createProcessingInstruction (already reported)
Element.setAttribute (already reported)
Element.setAttributeNS (not tested, but I assume this as well)

I believe that this bug is also platform independent.

See http://www.mindspring.com/~bobclary/TestFrame_dom_core.html (M17+ required).
Select any implementation, all tests, press Go!, wait a couple of minutes, then
search for INVALID_CHARACTER_ERR.

I am not familiar with the actual implementation, but it seems to me that the
proper place to test for invalid characters is in the Node constructor.
Assuming you are implementing the DOM Node related objects as classes inheriting
from Node, then fixing it in Node will fix it everywhere else automagically.

Comment 9

[Johnny Stenback (:jst)]

This bug has been marked "future" because the original netscape engineer
working on this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work
in some way -- please attach your concern to the bug for reconsideration.

Comment 10

[Jan Carpenter]

Mass update of qa contact

Comment 11

[balwinder]

To place on record,this bug exists for Intel - Solaris 2.7 as well .Tested on
Mozilla Build 20000100217 .

Comment 12

[Prashant Desale]

QA contact Update

Comment 13

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 14

[Brian Clark]

Confirmed that this is still broken in Mozilla 0.9.3 on MacOS 9.1.  

Tested using the Netscape DOM HTML Level 2 Conformance test document "tc_hdoc500
Test Exceptions for Document.createXXXX methods".  This test is available by
clicking on the link for DOM HTML Level 2 Conformance on this page:

http://developer.netscape.com/evangelism/tools/testsuites/

Comment 15

[vidur (gone)]

Handing over to the DOM owner [e-mail]

Comment 16

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 17

[Peter Van der Beken [:peterv]]

Taking.

Comment 18

[Peter Van der Beken [:peterv]]

*** Bug 16591 has been marked as a duplicate of this bug. ***

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: v1

This patch adds the name check to the necessary DOM functions (we still need to
verify with the DOM WG that this is what is intended by the spec). It also
consolidates the different CreateElement(NS) implementations.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 140688 [details] [diff] [review]
v1

>Index: html/content/src/nsGenericHTMLElement.cpp
>@@ -343,6 +343,9 @@ NS_IMETHODIMP
> nsGenericHTMLElement::SetAttribute(const nsAString& aName,
>                                    const nsAString& aValue)
> {
>+  nsresult rv = nsContentUtils::CheckQName(aName, PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   const nsAttrName* name = InternalGetExistingAttrNameFromQName(aName);

You should probably use PR_TRUE as second argument for xhtml. And do this check
only if no existing attribute is found to save cycles. Same in
nsGenericElement::SetAttribute and nsXULElement::SetAttribute.

With that, r=me

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 140688 [details] [diff] [review]
v1

What sicking said. sr=jst

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: Correct stupid error

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 141151 [details] [diff] [review]
Correct stupid error

r+sr=jst

Comment 24

[chris hofmann]

Comment on attachment 141151 [details] [diff] [review]
Correct stupid error

a=chofmann for 1.7a

Comment 25

[Kathleen :Brade]

This bug appears to have regressed the editor (bug 244392).

Comment 26

[Yves Lambert]

Please reopen...

From about:version

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060909 MultiZilla/1.8.2.0i Mnenhy/0.7.4.0 SeaMonkey/1.5a

Some tests fail again in trunk:
No exeption raised when it should:
Trying document.createProcessingInstruction("xml-stylesheet", "containing?>is bad") :Incorrect:  document.createProcessingInstruction("xml-stylesheet", "containing?>is bad") did not throw an exception, but instead returned the value [object XMLStylesheetProcessingInstruction].  It should have thrown the exception INVALID_CHARACTER_ERR.

Trying document.createEntityReference("bad entity name") :Incorrect:  document.createEntityReference("bad entity name") did not throw an exception, but instead returned the value null.  It should have thrown the exception INVALID_CHARACTER_ERR.

Trying document.createEntityReference("bad;entity;name") :Incorrect:  document.createEntityReference("bad;entity;name") did not throw an exception, but instead returned the value null.  It should have thrown the exception INVALID_CHARACTER_ERR.

Throwing another exception than it should (?):

Trying document.createProcessingInstruction("2style", "notmuch") :Incorrect:  document.createProcessingInstruction("2style", "notmuch") threw the exception [Exception... "An attempt was made to create or change an object in a way which is incorrect with regard to namespaces"  code: "14" nsresult: "0x8053000e (NS_ERROR_DOM_NAMESPACE_ERR)"  location: "http://dbaron.org/dom/test/base.js Line: 269"](code 14) when it should have thrown INVALID_CHARACTER_ERR.

Comment 27

[Peter Van der Beken [:peterv]]

(In reply to comment #26)
> No exeption raised when it should:
> Trying document.createProcessingInstruction("xml-stylesheet", "containing?>is
> bad") :Incorrect:  document.createProcessingInstruction("xml-stylesheet",
> "containing?>is bad") did not throw an exception, but instead returned the
> value [object XMLStylesheetProcessingInstruction].  It should have thrown the
> exception INVALID_CHARACTER_ERR.

I filed bug 352728 about this and other cases. Note that the DOM spec is silent about this.

> Trying document.createEntityReference("bad entity name") :Incorrect: 

> Trying document.createEntityReference("bad;entity;name") :Incorrect: 

Gecko doesn't have support for entity nodes.

> Throwing another exception than it should (?):

Feel free to file this, it's certainly not a high priority for me to fix this.

Comment 28

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/986864f6024f
[fenix] Closes https://github.com/mozilla-mobile/fenix/issues/16603: Disable pull down refresh when activity is in immersive mode (https://github.com/mozilla-mobile/fenix/pull/16793)
https://hg.mozilla.org/releases/mozilla-beta/rev/b12000aa3504
[fenix] Closes https://github.com/mozilla-mobile/fenix/issues/16603: Disable pull down when in fullscreen (https://github.com/mozilla-mobile/fenix/pull/17314)