Change HTMLTokenizer::Attribute::Range.start initial invalid value from 0 -> -1

Change HTMLTokenizer::Attribute::Range.start initial invalid value from 0 -> -1

In rare cases, BackgroundHTMLParser may feed input SegmentedString starting
w/ attribute name=value pairs. This CL fixes ASSERT failures in such cases.

Before this CL, the attribute {name,value} range start offset initial value
was set to 0. This was based on the assumption that attribute name=value pairs
never have input stream offset of 0. However, this assumption doesn't hold
when BackgroundHTMLParser has XSSAuditor disabled and accurate offset
tracking has been disabled.

This CL changes the initial invalid value to -1 to avoid the issue.

BUG=619141
Committed: https://crrev.com/f87027a2bc99f6d40faa7e24174d2b210b58b120
Cr-Commit-Position: refs/heads/master@{#404150}

[kouhei (in TOK), 2016-07-04 08:35:23]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 08:35:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kouhei (in TOK), 2016-07-04 08:36:23]

kouhei@chromium.org changed reviewers:
+ csharrison@chromium.org, dominicc@chromium.org, yoav@yoav.ws

[kouhei (in TOK), 2016-07-04 08:36:24]

Would you take a look?

[commit-bot: I haz the power, 2016-07-04 09:12:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-04 09:12:34]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-07-04 09:19:39]

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLToken.h (left):

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:340:
m_currentAttribute->mutableValueRange().start = offset - m_baseOffset;
Alternatively, maybe DCHECK here that offset is larger or equal to m_baseOffset?

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLToken.h (right):

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:101: DCHECK(end !=
kInvalidOffset);
shouldn't this check that start is not negative?

[dominicc (has gone to gerrit), 2016-07-05 01:08:10]

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLToken.h (right):

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:94: inline void
checkValidStart() const
I'm not sure checkValidStart and checkValidEnd are pulling their weight and
should not just be moved into checkValid. I see places where you only
checkValidStart, but why not just check start and end at those points? When
would start be valid but end invalid?

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:104: inline void
checkValid() const
Is there any relationship between start and end you can check, like start <=
end? If not it is probably worth a comment explaining what values like that
mean.

[kouhei (in TOK), 2016-07-05 03:57:14]

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLToken.h (right):

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:94: inline void
checkValidStart() const
On 2016/07/05 01:08:10, dominicc wrote:
> I'm not sure checkValidStart and checkValidEnd are pulling their weight and
> should not just be moved into checkValid. I see places where you only
> checkValidStart, but why not just check start and end at those points? When
> would start be valid but end invalid?

We want two check methods:
- one for checking Range instances in middle of parsing (req vlaid start, but
may have invalid end)
- one for checking Range instances after parse (req. valid start+end)

https://codereview.chromium.org/2121703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:101: DCHECK(end !=
kInvalidOffset);
On 2016/07/04 09:19:39, Yoav Weiss wrote:
> shouldn't this check that start is not negative?

Done.

[dominicc (has gone to gerrit), 2016-07-06 01:35:55]

LGTM, nice succinct comments.

[Charlie Harrison, 2016-07-06 13:50:38]

Looks like some tests are failing. Otherwise LG.

https://codereview.chromium.org/2121703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLToken.h (right):

https://codereview.chromium.org/2121703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:97: DCHECK(start !=
kInvalidOffset);
nit: Use DCHECK_NE.

https://codereview.chromium.org/2121703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLToken.h:105: DCHECK(end !=
kInvalidOffset);
ditto

[kouhei (in TOK), 2016-07-07 05:59:29]

On 2016/07/06 13:50:38, csharrison wrote:
> Looks like some tests are failing. Otherwise LG.
To be fixed in https://codereview.chromium.org/2127043002/

[kouhei (in TOK), 2016-07-07 09:56:25]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 09:56:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 11:12:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 11:12:22]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-07-07 12:05:21]

lgtm % optional nits.

[kouhei (in TOK), 2016-07-07 13:45:52]

On 2016/07/07 12:05:21, csharrison wrote:
> lgtm % optional nits.

Thanks. Let me TBR the nits tomorrow. VPN isn't working well...

[kouhei (in TOK), 2016-07-07 13:45:59]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2016-07-07 13:46:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 13:50:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-07 13:50:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 13:52:58]

Description was changed from

==========
Change HTMLTokenizer::Attribute::Range.start initial invalid value from 0 -> -1

In rare cases, BackgroundHTMLParser may feed input SegmentedString starting
w/ attribute name=value pairs. This CL fixes ASSERT failures in such cases.

Before this CL, the attribute {name,value} range start offset initial value
was set to 0. This was based on the assumption that attribute name=value pairs
never have input stream offset of 0. However, this assumption doesn't hold
when BackgroundHTMLParser has XSSAuditor disabled and accurate offset
tracking has been disabled.

This CL changes the initial invalid value to -1 to avoid the issue.

BUG=619141
==========

to

==========
Change HTMLTokenizer::Attribute::Range.start initial invalid value from 0 -> -1

In rare cases, BackgroundHTMLParser may feed input SegmentedString starting
w/ attribute name=value pairs. This CL fixes ASSERT failures in such cases.

Before this CL, the attribute {name,value} range start offset initial value
was set to 0. This was based on the assumption that attribute name=value pairs
never have input stream offset of 0. However, this assumption doesn't hold
when BackgroundHTMLParser has XSSAuditor disabled and accurate offset
tracking has been disabled.

This CL changes the initial invalid value to -1 to avoid the issue.

BUG=619141

Committed: https://crrev.com/f87027a2bc99f6d40faa7e24174d2b210b58b120
Cr-Commit-Position: refs/heads/master@{#404150}
==========

[commit-bot: I haz the power, 2016-07-07 13:52:59]

Patchset 2 (id:??) landed as
https://crrev.com/f87027a2bc99f6d40faa7e24174d2b210b58b120
Cr-Commit-Position: refs/heads/master@{#404150}