Bug 15960 - View source mode displays the attributes after an attribute with an empty value incorrectly.
Summary: View source mode displays the attributes after an attribute with an empty val...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-12 15:35 PST by Anyang Ren
Modified: 2008-01-12 01:17 PST (History)
3 users (show)

See Also:


Attachments
Preliminary patch for discussion (1.38 KB, patch)
2007-11-12 15:46 PST, Anyang Ren
mjs: review-
Details | Formatted Diff | Diff
Proposed patch v1 (2.12 KB, patch)
2007-12-19 19:07 PST, Anyang Ren
no flags Details | Formatted Diff | Diff
Proposed patch v1 (with a new regression test) (15.61 KB, patch)
2007-12-21 17:38 PST, Anyang Ren
no flags Details | Formatted Diff | Diff
Proposed patch v2 (15.85 KB, patch)
2007-12-22 18:02 PST, Anyang Ren
no flags Details | Formatted Diff | Diff
Proposed patch v2.1 (17.47 KB, patch)
2007-12-22 22:01 PST, Anyang Ren
no flags Details | Formatted Diff | Diff
Proposed patch v3 (18.58 KB, patch)
2007-12-23 14:56 PST, Anyang Ren
mjs: review-
Details | Formatted Diff | Diff
Proposed patch v4 (6.48 KB, patch)
2008-01-02 16:40 PST, Anyang Ren
no flags Details | Formatted Diff | Diff
Proposed patch v4.1 (6.47 KB, patch)
2008-01-07 10:55 PST, Anyang Ren
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anyang Ren 2007-11-12 15:35:06 PST
When in view source mode, the attributes of a tag after an attribute with an empty value
are not displayed correctly.

For example, the HTML page at http://alioth.debian.org/ has this img tag:

<img src="/themes/gforge/images/logo.png" border="0" alt="" width="198" height="52" />

Note that the alt attribute has an empty value.

In the view source mode, that img tag is displayed like this:
<img src="/themes/gforge/images/logo.png" border="0" alt="198" height="52" ="" />

Note that the alt attribute and the attributes after it are incorrect.

The problem occurs in HTMLViewSourceDocument::addViewSourceToken:

                             Attribute* attr = token->attrs->attributeItem(currAttr);
                            String name = attr->name().toString();
                            if (doctype)
                                addText(name, "webkit-html-doctype");
                            else {
                                m_current = addSpanWithClassName("webkit-html-attribute-name");
                                addText(name, "webkit-html-attribute-name");
                                if (m_current != m_tbody)
                                    m_current = static_cast<Element*>(m_current->parent());
                            }
                            if (attr->value().isNull() || attr->value().isEmpty())
                                currAttr++;                                    <=== PROBLEM

The last statment, currAttr++, skips an empty attribute value, causing attribute (name, value)
pairs to get out of sync.
Comment 1 Anyang Ren 2007-11-12 15:46:00 PST
Created attachment 17215 [details]
Preliminary patch for discussion

This patch is intended to indicate the location of the bug and suggest a possible fix.
I don't know the code well enough to know if this fix is correct.

I first tried removing the currAttr++ if statement, but that broke the "!DOCTYPE" tag.
So in this patch I moved the currAttr++ if statement into the if (doctype) block.

I wonder if we can simply do currAttr++ without checking if attr->value() is
null or empty:

                            if (doctype) {
                                addText(name, "webkit-html-doctype");
                                currAttr++;
                            } else {
Comment 2 Anyang Ren 2007-11-13 13:27:06 PST
David, I'd appreciate your comments on this view source mode
bug and my fix.  Thanks.
Comment 3 David Kilzer (:ddkilzer) 2007-11-13 19:44:07 PST
What version of Safari/WebKit are you seeing this on?

I suggest setting the "review?" flag on the patch even though it's not for committing.  That's how they get reviewed.  Thanks!

Comment 4 Anyang Ren 2007-11-14 11:35:53 PST
Comment on attachment 17215 [details]
Preliminary patch for discussion

I'm using the latest revision of WebKit/WebCore/html/HTMLViewSourceDocument.cpp, r21704.
I'm not using Safari.  Safari's "View Source" command doesn't seem to be implemented using the
HTMLViewSourceDocument class.
Comment 5 mitz 2007-11-14 11:37:55 PST
(In reply to comment #4)
> (From update of attachment 17215 [details] [edit])
> Safari's "View Source" command doesn't seem to be
> implemented using the
> HTMLViewSourceDocument class.

The Source view in the Web Inspector uses it.
Comment 6 Anyang Ren 2007-11-14 14:39:17 PST
Thanks.  Yes, you can confirm this bug using the Source view in the Web Inspector.
Comment 7 Eric Seidel (no email) 2007-11-20 05:47:25 PST
I don't think hyatt is the right reviewer for these.  (I don't see any comments from him in this bug, and he's certainly not the only person to have worked on the HTML Tokenizer or HTML serialization).  Marking these as "review by hyatt" will only make them slower to be reviewed.
Comment 8 Anyang Ren 2007-11-20 17:19:06 PST
Eric, could you suggest the right reviewer for my patches?  I found that
hyatt is the only person who checked in changes to HTMLViewSourceDocument.cpp
in the svn log.  That's why I asked him to review my patches.
Comment 9 Alexey Proskuryakov 2007-11-20 22:58:53 PST
Usually, we just leave the name empty, so any reviewer who feels qualified can review the patch.
Comment 10 Maciej Stachowiak 2007-11-22 23:01:28 PST
Comment on attachment 17215 [details]
Preliminary patch for discussion

Looks generally ok, but needs a ChangeLog and test cases.

r-
Comment 11 Anyang Ren 2007-12-19 19:07:51 PST
Created attachment 18001 [details]
Proposed patch v1

I added a ChangeLog entry.

Where are the test cases for the Web Inspector located?
Comment 12 David Kilzer (:ddkilzer) 2007-12-19 23:15:38 PST
(In reply to comment #11)
> Where are the test cases for the Web Inspector located?

Since the Web Inspector uses WebKit itself to do rendering, there are (currently) no special web-inspector-specific tests.

Having said that, if you figure out a way to write web-inspector-specific tests (especially if they may be automated), they would certainly be appreciated!

Comment 13 mitz 2007-12-19 23:19:53 PST
(In reply to comment #11)
> Where are the test cases for the Web Inspector located?

There are a couple in fast/inspector, and there is a View Source test in fast/frames.
Comment 14 Anyang Ren 2007-12-21 17:38:20 PST
Created attachment 18053 [details]
Proposed patch v1 (with a new regression test)

Thank you for the hints.  I added a new regression test for this
bug.  Please review the patch.  Thanks!
Comment 15 mitz 2007-12-21 20:37:15 PST
Comment on attachment 18053 [details]
Proposed patch v1 (with a new regression test)

I think the patch breaks the no-value case
<element attrA attrB="valB">
Comment 16 Anyang Ren 2007-12-22 18:02:31 PST
Created attachment 18063 [details]
Proposed patch v2

Mitz, you're right.  Thanks for pointing that out.
I studied the code more and I think I understand the
code now.  The key is to study HTMLTokenizer::parseTag.

This patch should be correct, although it may not be the
best fix.

1. I believe that it is not necessary to test attr->value().isNull()
because in HTMLTokenizer::parseTag an attribute with no value is
added with:

  currToken.addAttribute(m_doc, attrName, emptyAtom, inViewSourceMode());

2. I use 'begin' because it equals 'i + 1', which is what I need.
If you think using 'i + 1' is clearer than 'begin', I can do that.
Comment 17 Anyang Ren 2007-12-22 22:01:24 PST
Created attachment 18065 [details]
Proposed patch v2.1

I improved the regression test a little bit.
Comment 18 Anyang Ren 2007-12-23 14:56:52 PST
Created attachment 18076 [details]
Proposed patch v3

I figured out a better fix.  It's much easier to determine when
we start processing the next attribute than to determine when we're
done with the current attribute.  So whenever we see the guide
character 'a', signaling the beginning of an attribute, we increment
currAttr.

This requires initializing currAttr to -1 so that after the first
increment it starts at 0.  So I have to declare currAttr as 'int',
and have to add a static cast to unsigned when I compare it with
token->attrs->length(), an unsigned value.
Comment 19 mitz 2007-12-30 19:30:42 PST
Comment on attachment 18076 [details]
Proposed patch v3

This patch is correct, but it leaves the code in poor shape. I think it would be better to define the 'attr' variable outside the the for loop, then when you see an 'a', assign the current attribute to 'attr' and increment 'currAttr'. Then you won't need the assignment in the else clause (since 'attr' will have been assigned before) and you won't need to start the counter at -1.

The regression test should not dump the render tree, but instead do a text dump. This is accomplished by adding something like

<script>
    if (window.layoutTestController) {
        layoutTestController.dumpAsText();
        layoutTestController.dumpChildFramesAsText();
    }
</script>

to the test. layoutTestController is an interface DumpRenderTree provides to tests to control its behavior.
Comment 20 Maciej Stachowiak 2008-01-01 18:30:03 PST
Comment on attachment 18076 [details]
Proposed patch v3

r- based on comments from Mitz - please address those and repost. Thanks!
Comment 21 Anyang Ren 2008-01-02 16:40:20 PST
Created attachment 18245 [details]
Proposed patch v4

Mitz, thanks a lot for the review.  I made your suggested changes in this patch.
Note that I set attr to NULL if (token->attrs && currAttr < token->attrs->length())
is false, and test for a non-NULL attr to match the behavior of the original code as
much as possible.  I considered making the simpler change below, but am not sure
if it's safe:

Index: HTMLViewSourceDocument.cpp
===================================================================
--- HTMLViewSourceDocument.cpp  (revision 29095)
+++ HTMLViewSourceDocument.cpp  (working copy)
@@ -128,6 +128,7 @@
             unsigned size = guide->size();
             unsigned begin = 0;
             unsigned currAttr = 0;
+            Attribute* attr = NULL;
             for (unsigned i = 0; i < size; i++) {
                 if (guide->at(i) == 'a' || guide->at(i) == 'x' || guide->at(i) == 'v') {
                     // Add in the string.
@@ -135,9 +136,8 @@
                      
                     begin = i + 1;
 
-                    if (token->attrs && currAttr < token->attrs->length()) {
                         if (guide->at(i) == 'a') {
-                            Attribute* attr = token->attrs->attributeItem(currAttr);
+                            attr = token->attrs->attributeItem(currAttr++);
                             String name = attr->name().toString();
                             if (doctype)
                                 addText(name, "webkit-html-doctype");
@@ -147,10 +147,7 @@
                                 if (m_current != m_tbody)
                                     m_current = static_cast<Element*>(m_current->parent());
                             }
-                            if (attr->value().isNull() || attr->value().isEmpty())
-                                currAttr++;
                         } else {
-                            Attribute* attr = token->attrs->attributeItem(currAttr);
                             String value = attr->value().domString();
                             if (doctype)
                                 addText(value, "webkit-html-doctype");
@@ -164,9 +161,7 @@
                                 if (m_current != m_tbody)
                                     m_current = static_cast<Element*>(m_current->parent());
                             }
-                            currAttr++;
                         }
-                    }
                 }
             }
Comment 22 Eric Seidel (no email) 2008-01-04 14:00:09 PST
Comment on attachment 18245 [details]
Proposed patch v4

In general this fix looks fine.  The style guidelines require using 0 instead of NULL for new C++ code.  Also, an ideal test case would say "PASS" or "FAILURE" so as not to require user interaction.  However the change as a whole looks fine.  I'll let someone who actually has knowledge about the viewsource code give the final review.
Comment 23 Anyang Ren 2008-01-07 10:55:09 PST
Created attachment 18315 [details]
Proposed patch v4.1

The only difference from patch v4 is to use 0 instead of NULL for null
pointers.

I think I can improve the patch more if I understand the code better.  Specifically, I suspect that the condition
(token->attrs && currAttr < token->attrs->length()) must be true
because the guide string and the attribute array should be created
in sync in HTMLTokenizer.cpp; if the condition
(token->attrs && currAttr < token->attrs->length()) is false, I
believe it indicates a bug in HTMLTokenizer::parseTag().
Comment 24 Darin Adler 2008-01-11 18:40:13 PST
Comment on attachment 18315 [details]
Proposed patch v4.1

r=me

But can't we also fix this by distinguishing null attribute values and empty ones?
Comment 25 mitz 2008-01-11 22:48:23 PST
Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>.
Comment 26 David Kilzer (:ddkilzer) 2008-01-12 00:43:54 PST
(In reply to comment #25)
> Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>.

Is this bug resolved then?  :)

Comment 27 mitz 2008-01-12 01:17:28 PST
(In reply to comment #26)
> (In reply to comment #25)
> > Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>.
> 
> Is this bug resolved then?  :)
> 

Yes. Sorry, and thank you Dave and Eric.