Bug 15960 – View source mode displays the attributes after an attribute with an empty value incorrectly.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Anyang Ren]

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.

[Anyang Ren]

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 {

[Anyang Ren]

David, I'd appreciate your comments on this view source mode
bug and my fix.  Thanks.

[David Kilzer (:ddkilzer)]

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!

[Anyang Ren]

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.

(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.

[Anyang Ren]

Thanks.  Yes, you can confirm this bug using the Source view in the Web Inspector.

[Eric Seidel (no email)]

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.

[Anyang Ren]

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.

[Alexey Proskuryakov]

Usually, we just leave the name empty, so any reviewer who feels qualified can review the patch.

[Maciej Stachowiak]

Comment on attachment 17215 [details]
Preliminary patch for discussion

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

r-

[Anyang Ren]

Created attachment 18001 [details]
Proposed patch v1

I added a ChangeLog entry.

Where are the test cases for the Web Inspector located?

[David Kilzer (:ddkilzer)]

(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!

(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.

[Anyang Ren]

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 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">

[Anyang Ren]

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.

[Anyang Ren]

Created attachment 18065 [details]
Proposed patch v2.1

I improved the regression test a little bit.

[Anyang Ren]

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 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.

[Maciej Stachowiak]

Comment on attachment 18076 [details]
Proposed patch v3

r- based on comments from Mitz - please address those and repost. Thanks!

[Anyang Ren]

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++;
                         }
-                    }
                 }
             }

[Eric Seidel (no email)]

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.

[Anyang Ren]

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().

[Darin Adler]

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?

Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>.

[David Kilzer (:ddkilzer)]

(In reply to comment #25)
> Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>.

Is this bug resolved then?  :)

(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.