[webkit-reviews] review granted: [Bug 176357] Missing break in URLParser : [Attachment 319889] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tomas Popela <tpopela at redhat.com>'s
request for review:
Bug 176357: Missing break in URLParser
https://bugs.webkit.org/show_bug.cgi?id=176357

Attachment 319889: Patch

https://bugs.webkit.org/attachment.cgi?id=319889&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 319889
  --> https://bugs.webkit.org/attachment.cgi?id=319889
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319889&action=review

>>>> Source/WebCore/ChangeLog:9
>>>> +	      after the block. Found by Coverity scan.
>>> 
>>> Does this change observable behavior? If so, please add a test.
>> 
>> Yes, it’s a great catch, but we do want a regression test to show what we
were doing wrong!
> 
> I don't think we need test case if I got the context right:
> 
> From looking at the code the sequence to get to the part where the 'break' is
missing we have to process the input that contains one character. We will start
with state SchemeStart (log this state), process the character (that has to be
ASCII lowercase alpha) and append to the buffer. Now we will move to the next
character and as there is none (c.atEnd() is true) we will move to the branch
with missing 'break'. The state is set to NoScheme, the input restarted, buffer
cleared and the state is again changed, but to Scheme (due to missing 'break').
We continue in the loop (!c.atEnd()), now with the Scheme state. The state is
logged and the first condition for Scheme case is isValidSchemeCharacter(). We
know that the input contains ASCII lowercase alpha and that means that it is
also a valid scheme character. The character is appended to the buffer and we
move to the next character in the input, but as there is none then the state is
set to NoScheme, the input restarted, buffer cleared. We again continue in the
loop, now with the state NoScheme. So we got to the same point where we would
end with the 'break' added in this patch. So the main difference is that
without the 'break' there is an extra log message mentioning Scheme state. I
should note that the url parser debugging is disabled by default, so in the
default configuration there is no difference while running URLParser with or
without this patch.

Yes, makes sense. This mistake did not result in incorrect behavior, just a
*tiny* bit less efficient.