1153920 - Bring htmlparser ampersand-error-reporting into conformance with HTML spec

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Michael[tm] Smith [:sideshowbarker]]

see https://bugzilla.validator.nu/show_bug.cgi?id=841

This is a long-standing known issue for which we continue to get validator bug reports. This is the issue that, e.g., ampersands in URLs in href and src values etc. such as http://example.com?foo=bar&hoge=moge are not errors according to the current HTML spec. So reporting errors for them is non-conforming.

Comment 1

[Michael[tm] Smith [:sideshowbarker]]

Attachment: patch-validator-ampersands

Comment 2

[Michael[tm] Smith [:sideshowbarker]]

Attachment: patch for Java sources

Comment 3

[Michael[tm] Smith [:sideshowbarker]]

Attachment: patch for Gecko sources

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8591774 [details] [diff] [review]
patch for Java sources

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

::: src/nu/validator/htmlparser/impl/Tokenizer.java
@@ +3244,5 @@
>                      // XXX reorder point
> +                case AMBIGUOUS_AMPERSAND:
> +                    ampersandloop: for (;;) {
> +                        if (reconsume) {
> +                            reconsume = false;

It seems that this state is special in the sense that it's always entered into with reconsume set to true. Perhaps that should be asserted here and the 'else' branch be removed.

@@ +3259,5 @@
> +                                || (c >= 'a' && c <= 'z')) {
> +                            appendLongStrBuf(c);
> +                            continue;
> +                        }
> +                        pos--;

This is a pattern that doesn't occur elsewhere in the tokenizer. It seems that what's wanted here is
reconsume = true;

reconsume is set to false near the start of this state. Right now, it seems to me that this state should never consume a character and should always be entered and left with reconsume set to true. This makes this state rather unusual. I will need to think more to understand why it is OK to call appendLongStrBuf(c) in this kind of state that doesn't consume anything and exists mainly for error reporting.

Comment 5

[Michael[tm] Smith [:sideshowbarker]]

Attachment: Patch for Java sources.

Comment 6

[Michael[tm] Smith [:sideshowbarker]]

Attachment: Patch for Gecko sources.

Comment 7

[Michael[tm] Smith [:sideshowbarker]]

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> Comment on attachment 8591774 [details] [diff] [review]
> patch for Java sources
> 
> Review of attachment 8591774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: src/nu/validator/htmlparser/impl/Tokenizer.java
> @@ +3244,5 @@
> >                      // XXX reorder point
> > +                case AMBIGUOUS_AMPERSAND:
> > +                    ampersandloop: for (;;) {
> > +                        if (reconsume) {
> > +                            reconsume = false;
> 
> It seems that this state is special in the sense that it's always entered
> into with reconsume set to true. Perhaps that should be asserted here and
> the 'else' branch be removed.

OK, I've made it so in the updated patches most-recently attached.
 
> @@ +3259,5 @@
> > +                                || (c >= 'a' && c <= 'z')) {
> > +                            appendLongStrBuf(c);
> > +                            continue;
> > +                        }
> > +                        pos--;
> 
> This is a pattern that doesn't occur elsewhere in the tokenizer. It seems
> that what's wanted here is
> reconsume = true;
> 
> reconsume is set to false near the start of this state. Right now, it seems
> to me that this state should never consume a character and should always be
> entered and left with reconsume set to true. This makes this state rather
> unusual. I will need to think more to understand why it is OK to call
> appendLongStrBuf(c) in this kind of state that doesn't consume anything and
> exists mainly for error reporting.

For that, I've not made any change yet because I couldn't tell if you were saying that we should go ahead and make a change here or if you wanted to hold off any make a change for this until you've had a chance to consider it further.

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 8600565 [details] [diff] [review]
Patch for Java sources.

Review moved to GitHub after extreme delay. Sorry.

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 8600590 [details] [diff] [review]
Patch for Gecko sources.

Review moved to GitHub after extreme delay. Sorry.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1153920 - Conform ampersand error reporting to HTML spec.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1153920 addendum - Limit ambiguous-ampersand wasteful buffering.

This refines 9ce4bd4 by only buffering if we’re actually inside an
attribute value.

Comment 12

[Henri Sivonen (:hsivonen)]

This patch appears to change the DOM in an unwanted way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59a15640760bc1c1a4629c90b849cfa43a20c057&selectedTaskRun=GdCS5FrVRbCXm2jscZdZkw.0

Comment 13

[Michael[tm] Smith [:sideshowbarker]]

(In reply to Henri Sivonen (:hsivonen) (not reading bugmail until 2020-08-03) from comment #12)

This patch appears to change the DOM in an unwanted way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59a15640760bc1c1a4629c90b849cfa43a20c057&selectedTaskRun=GdCS5FrVRbCXm2jscZdZkw.0

Using the current Java sources for the parser — including the https://phabricator.services.mozilla.com/D81992 patch to the Java sources — I can’t reproduce the test failures shown at https://treeherder.mozilla.org/#/jobs?repo=try&revision=59a15640760bc1c1a4629c90b849cfa43a20c057&selectedTaskRun=GdCS5FrVRbCXm2jscZdZkw.0

For example, given this input:

<html><head><body>&ammmp;

…the Java parser produces the expected output — specifically, it outputs the &ammmp; as expected.

See https://validator.w3.org/nu/parsetree/?parser=html5&content=%3Chtml%3E%3Chead%3E%3Cbody%3E%26ammmp%3B&submit=Print+Tree

So I’m wondering if it’s possible that some difference could be getting introduced during the Java-to-C++ translation.

Comment 14

[Michael[tm] Smith [:sideshowbarker]]

The Java parser can also be tested by doing this:

curl -L -O https://github.com/validator/validator/releases/download/jar/vnu.jar \
  && echo "<html><head><body>&ammmp;" > TEST.html 
  && java -cp vnu.jar nu.validator.htmlparser.test.TreePrinter TEST.html

That outputs this tree:

| <html>
|   <head>
|   <body>
|     "&ammmp;
"

Comment 15

[Henri Sivonen (:hsivonen)]

For Bugzilla record: This is being discussed on GitHub. The difference is not from C++ but from where the test harness puts buffer boundaries.

Comment 16

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9f43fb9c89f65549d7746c7d6a5f22b7ee6b8f

Comment 17

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6899e3b9cbc6854802996e0fb14370c3890ea6aa

Comment 18

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1153920 test - Avoid marking non-error ampersands as errors in View Source.

Comment 19

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acbaa4bfc7df3ef277aec828b230c90d114e462a

Comment 20

[Henri Sivonen (:hsivonen)]

https://github.com/validator/htmlparser/commit/44427c0df60ec899c52b149bba03e1abffad2f5e

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8b2924ea4a3
Conform ampersand error reporting to HTML spec. r=smaug

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/30301 for changes under testing/web-platform/tests

Comment 23

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/f8b2924ea4a3

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot