Last Comment Bug 314980 - DOM element.getAttribute() method error (CAttributeToken::SanitizeKey())
: DOM element.getAttribute() method error (CAttributeToken::SanitizeKey())
Status: RESOLVED FIXED
: verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: P4 minor (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on: 315933
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-03 12:37 PST by Alejandro Torras
Modified: 2007-01-11 19:12 PST (History)
7 users (show)
jaymoz: blocking1.8.1.2+
jaymoz: blocking1.8.0.10+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Potential patch (9.24 KB, patch)
2005-11-04 18:26 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
testcases (1.30 KB, text/html)
2005-11-07 10:30 PST, Blake Kaplan (:mrbkap)
no flags Details
Fixed patch (10.79 KB, patch)
2005-11-07 10:34 PST, Blake Kaplan (:mrbkap)
bzbarsky: review+
jst: superreview+
Details | Diff | Splinter Review
Patch for the 1.8 branches (11.33 KB, patch)
2007-01-02 14:21 PST, Blake Kaplan (:mrbkap)
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review

Description Alejandro Torras 2005-11-03 12:37:42 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050920 Firefox/1.0.7 SUSE/1.0.7-0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050920 Firefox/1.0.7 SUSE/1.0.7-0.1

The call to getAttribute() method in Javascript doesn't work properly.

Reproducible: Always

Steps to Reproduce:
1. Make an HTML page with an element's name attribute set to "main_" (i.e. <td main_="100"> )
2. Try to get the attribute with getAttribute("main_")

Actual Results:  
The call returns NULL

Expected Results:  
Following the example (steps to reproduce) the call should return "main_" 

This is a cut from a post on netscape.public.mozilla.dom news.
Message subject: DOM element.getAttribute() error?



The next code is a cut from an HTML page:

<tr parent_type="head_blind" id="tr_1012" main_="110">
<td class="folder_content"><img onclick="../showResults.do" height="1" src="../skin/xp/img/pixel_empty.gif" width="10"><img style="cursor: pointer;" border="0" src="../skin/xp/img/folder_off.gif"><span type_action="folder">Chiquito</span></td><td class="folder_content" width="1"><img style="cursor: pointer;" border="0" src="../skin/xp/img/arrow_single_up.gif"></td>
</tr>

I wanted to find all the "TR" nodes with the "main_" attribute with:

////
function getElmWithAtt(elm, a, v)
{
    var nList = f.contentDocument.getElementsByTagName(elm);
    var i=0;           
    for (; i<nList.length; i++)
    {
        var m = nList[i];
        if (m.getAttribute(a) != null)
        {
            if (m.getAttribute(a) == v)
            {
                break;
            }
        }
    }
    return nList[i];
}

getElmWithAtt("TR", "main_", 110);
////

But it doesn't work.
It worked with: getElmWithAtt("TR", "main", 110);
Comment 1 Blake Kaplan (:mrbkap) 2005-11-03 14:04:01 PST
Yeah, might as well sync up with IE. It might be worth it to figure out if IE doesn't allow various characters in attribute keys (note: this function was added for bug 7548).
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-11-03 16:04:48 PST
IMHO we should do as little "fixup" as possible of the markup. Dropping ending '/'  seems like a good idea to ease xhtml migration. Other then that I think we should not strip any more characters then what IE does.
Comment 3 Blake Kaplan (:mrbkap) 2005-11-03 22:15:51 PST
I did some testing; IE treats / as a terminator for attributes (which means that in cases like <foo f/g>, it treats foo as having 2 attributes, f and g). Do we want to emulate that?
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-11-04 07:59:18 PST
Ugh, our current behavour is less then ideal. For <div f/b> we create an attribute named 'f/b'. IMHO we should never create attributes or elements with names containing illigal characters. Not sure what qualifies as legal characters in HTML/SGML, but in XML it's pretty clearly defined

http://www.w3.org/TR/2004/REC-xml11-20040204/#NT-Name

Ideally we should drop other charaters, so parsing <div f/b> as <div f b> or something similar seems good to me.

Very imporantly, we should never put an ascii-null inside a element or attribute name. I suspect that that could cause security issues if we compare strings and atoms.
Comment 5 Blake Kaplan (:mrbkap) 2005-11-04 18:26:47 PST
Created attachment 201898 [details] [diff] [review]
Potential patch

This needs to be pounded on, hard. I think it should do the right thing, though.
Comment 6 Blake Kaplan (:mrbkap) 2005-11-07 10:30:31 PST
Created attachment 202114 [details]
testcases

Running these testcases should be self-explanatory. Are there any cases that I'm missing?

Obsoleting attachment 201898 [details] [diff] [review] in preparation for a better patch.
Comment 7 Blake Kaplan (:mrbkap) 2005-11-07 10:34:51 PST
Created attachment 202115 [details] [diff] [review]
Fixed patch

This patch seems to provide the wanted compatibility.
Comment 8 Boris Zbarsky [:bz] 2005-11-07 15:24:09 PST
Comment on attachment 202115 [details] [diff] [review]
Fixed patch

>Index: parser/htmlparser/src/nsHTMLTokens.cpp
>+  NS_ASSERTION(aChar == kApostrophe|| aChar == kQuote || aChar == kForwardSlash,

Space before first ||, please.

>@@ -2029,39 +1994,41 @@ nsresult CAttributeToken::Consume(PRUnic
>+                result = aScanner.SkipOver(aChar); //strip quote.

quote or slash

r=bzbarsky
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-11-07 15:44:03 PST
Comment on attachment 202115 [details] [diff] [review]
Fixed patch

sr=jst
Comment 10 Blake Kaplan (:mrbkap) 2005-11-07 16:11:01 PST
Fix checked into trunk. I'll file a new bug on our potentially allowing NULs in attribute keys in a second.
Comment 11 Blake Kaplan (:mrbkap) 2007-01-02 14:21:36 PST
Created attachment 250215 [details] [diff] [review]
Patch for the 1.8 branches

This patch is simply the result of CVS joins. It applies cleanly to the 1.8 and 1.8.0 branches.
Comment 12 Daniel Veditz [:dveditz] 2007-01-02 15:08:20 PST
Wanted for compatibility with other browser's parsing of attributes; being non-standard here is complicating web apps trying to filter content.
Comment 13 Jay Patel [:jay] 2007-01-03 15:01:29 PST
Comment on attachment 250215 [details] [diff] [review]
Patch for the 1.8 branches

Approved for both branches, a=jay for drivers.
Comment 14 Blake Kaplan (:mrbkap) 2007-01-05 14:19:17 PST
Fixed on the 1.8 branches.
Comment 15 alice nodelman [:alice] [:anode] 2007-01-11 19:12:23 PST
Using provided testcase confirmed existence of bug in 2.0.0.1.

Verified for 2.0.0.2pre and 1.5.0.10pre.  All PASS for testcase.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/2007011103 BonEcho/2.0.0.2pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10pre) Gecko/20070111 Firefox/1.5.0.10pre

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