REGRESSION(r93858): Can't type anything into input elements when maxlength is greater than 2^31

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=98117

We use parseHTMLInteger() since r93858. However parseHTMLInteger() doesn't fail by overflow.

[Kent Tamura]

Created attachment 108981 [details]
Patch

[Darin Adler]

Comment on attachment 108981 [details]
Patch

Is this enough test coverage? What about all the other places that use the HTML parser? Do any of them change behavior based on this? Can we make tests covering them?

[Kent Tamura]

(In reply to comment #2)
> (From update of attachment 108981 [details])
> Is this enough test coverage? What about all the other places that use the HTML parser? Do any of them change behavior based on this? Can we make tests covering them?

parseHTMLNonNegativeInteger() is used only for border attribute parsing.
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLElement.cpp#L146

parseHTMLInteger() is used for tabindex attribute parsing, and maxlength attribute parsing.
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLElement.cpp#L180

I'll add tests for them.

[Kent Tamura]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 108981 [details] [details])
> > Is this enough test coverage? What about all the other places that use the HTML parser? Do any of them change behavior based on this? Can we make tests covering them?
> 
> parseHTMLNonNegativeInteger() is used only for border attribute parsing.
> http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLElement.cpp#L146
> 
> parseHTMLInteger() is used for tabindex attribute parsing, and maxlength attribute parsing.
> http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLElement.cpp#L180
> 
> I'll add tests for them.

Filed https://bugs.webkit.org/show_bug.cgi?id=69055 and https://bugs.webkit.org/show_bug.cgi?id=69056.

[WebKit Review Bot]

Comment on attachment 108981 [details]
Patch

Clearing flags on attachment: 108981

Committed r96290: <http://trac.webkit.org/changeset/96290>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mounir Lamouri]

When maxlength value isn't parsed correctly as a non-negative integer, maxlength shouldn't apply. Currently, it seems that Webkit is using 0 as the default value isntead of -1. It seems that fixing that would have fix this bug.

[Kent Tamura]

(In reply to comment #7)
> When maxlength value isn't parsed correctly as a non-negative integer, maxlength shouldn't apply. Currently, it seems that Webkit is using 0 as the default value isntead of -1. It seems that fixing that would have fix this bug.

Do you mean this bug is not fixed yet?  What revision of WebKit are you using?

[Mounir Lamouri]

Hmm, I just realized that some information related to this bug where actually in the Chromium bug tracker. I thought the parsing was failing because of the overflow and 0 was used as a default value in that case. Looks like the parsing algorithm was returning 0 instead of failing. Things make a bit more sense now.

Ignore my previous comment then. Though, Webkit should probably consider using -1 as the default maxlength value instead of 524288 as it appears to do in my build.

[Kent Tamura]

(In reply to comment #9)
> Hmm, I just realized that some information related to this bug where actually in the Chromium bug tracker. I thought the parsing was failing because of the overflow and 0 was used as a default value in that case. Looks like the parsing algorithm was returning 0 instead of failing. Things make a bit more sense now.

Yeah, the number parsing code returns an error, instead of 0, now.

> Ignore my previous comment then. Though, Webkit should probably consider using -1 as the default maxlength value instead of 524288 as it appears to do in my build.

This issue is tracked in Bug 44883.