[scanner] Fix bom handling

[scanner] Fix bom handling

The bug used to show up when
- we were streaming a script starting with 0xef 0xbb 0xbf
- we aborted preparsing a function (and reset to a bookmark)

BUG=chromium:685618
Review-Url: https://codereview.chromium.org/2663773002
Cr-Commit-Position: refs/heads/master@{#42790}
Committed: https://chromium.googlesource.com/v8/v8/+/d293656481cfefd69dd26301c20e480bb4878b54

[marja, 2017-01-30 13:03:54]

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

[commit-bot: I haz the power, 2017-01-30 13:04:05]

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

[marja, 2017-01-30 13:04:40]

marja@chromium.org changed reviewers:
+ vogelheim@chromium.org

[marja, 2017-01-30 13:04:40]

ptal

I didn't have time to investigate why the existing tests don't catch this or add
a new one, or even run tests. But this fixes the bug in question.

[marja, 2017-01-30 13:09:37]

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

[commit-bot: I haz the power, 2017-01-30 13:09:41]

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

[jochen (gone - plz use gerrit), 2017-01-30 13:11:34]

On 2017/01/30 at 13:04:40, marja wrote:
> ptal
> 
> I didn't have time to investigate why the existing tests don't catch this or
add a new one, or even run tests. But this fixes the bug in question.

Meaning you'll later upload a new patch set with a test?

[commit-bot: I haz the power, 2017-01-30 13:34:28]

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

[commit-bot: I haz the power, 2017-01-30 13:34:29]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2017-01-30 17:40:51]

lgtm.

Thanks for the fix. I thought I had handled that case once and for all.. .:-/

I'll also be happy to accept a TODO(vogelheim) or bugfor adding a test case, if
that should block this CL.

[vogelheim, 2017-01-30 18:59:32]

I

https://codereview.chromium.org/2663773002/diff/1/src/parsing/scanner-charact...
File src/parsing/scanner-character-streams.cc (right):

https://codereview.chromium.org/2663773002/diff/1/src/parsing/scanner-charact...
src/parsing/scanner-character-streams.cc:271: // BOM detected at beginning of
the stream. Don't copy it.
copy -> count. This loop doesn't copy anything; it merely counts characters to
determine the correct position.

[vogelheim, 2017-01-30 19:19:29]

To help out, I've uploaded a separate regression test for this, and verified
that it breaks without Marja's test, but is fixed with it:
https://codereview.chromium.org/2663883002

Marja, either just include something like this in your CL, or I'll just submit
mine after your CL lands; depending on just how much Jochen wants the test to be
in this CL.


Jochen, Marja: There was a unit test to check for specifically this case
(cctest/test-scanner-streams/Utf8StreamBOM), but it was 'defeated' by the
various attempts at optimizing the stream implementations. That is, the test
worked fine, but didn't cover all conceivable code paths for the situation:

Depending on where the current character position is, we take slightly different
code paths. The old test read the stream, then issued Seek(0) (before the BOM)
and then Seek(5) (behind the BOM), and checks that either works. It does, but
the 2nd seek is simpler (since we're already in the right block and already have
all the characters in the current bugger). What I've added now is that we read
to the end-of-stream right after this, and then just repeat the Seek(5) test.
This currently breaks, but works with Marja's fix.

The situation is IMHO sufficiently contrived that I don't think there was much
of a chance to cover all of this ahead of time with unit tests, but.. YMMV.

[jochen (gone - plz use gerrit), 2017-01-30 20:01:00]

On 2017/01/30 at 19:19:29, vogelheim wrote:
> To help out, I've uploaded a separate regression test for this, and verified
that it breaks without Marja's test, but is fixed with it:
https://codereview.chromium.org/2663883002
> 
> Marja, either just include something like this in your CL, or I'll just submit
mine after your CL lands; depending on just how much Jochen wants the test to be
in this CL.
> 
> 
> Jochen, Marja: There was a unit test to check for specifically this case
(cctest/test-scanner-streams/Utf8StreamBOM), but it was 'defeated' by the
various attempts at optimizing the stream implementations. That is, the test
worked fine, but didn't cover all conceivable code paths for the situation:
> 
> Depending on where the current character position is, we take slightly
different code paths. The old test read the stream, then issued Seek(0) (before
the BOM) and then Seek(5) (behind the BOM), and checks that either works. It
does, but the 2nd seek is simpler (since we're already in the right block and
already have all the characters in the current bugger). What I've added now is
that we read to the end-of-stream right after this, and then just repeat the
Seek(5) test. This currently breaks, but works with Marja's fix.
> 
> The situation is IMHO sufficiently contrived that I don't think there was much
of a chance to cover all of this ahead of time with unit tests, but.. YMMV.

Feel free to land, lgtm

[amineer, 2017-01-30 20:14:28]

The CQ bit was checked by amineer@chromium.org

[commit-bot: I haz the power, 2017-01-30 20:14:42]

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, 2017-01-30 20:18:27]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485807268929500, "parent_rev":
"6c84b93ad88d44e2f46306041baf5bbdb06a0a86", "commit_rev":
"d293656481cfefd69dd26301c20e480bb4878b54"}

[commit-bot: I haz the power, 2017-01-30 20:18:33]

Description was changed from

==========
[scanner] Fix bom handling

The bug used to show up when
- we were streaming a script starting with 0xef 0xbb 0xbf
- we aborted preparsing a function (and reset to a bookmark)

BUG=chromium:685618
==========

to

==========
[scanner] Fix bom handling

The bug used to show up when
- we were streaming a script starting with 0xef 0xbb 0xbf
- we aborted preparsing a function (and reset to a bookmark)

BUG=chromium:685618

Review-Url: https://codereview.chromium.org/2663773002
Cr-Commit-Position: refs/heads/master@{#42790}
Committed:
https://chromium.googlesource.com/v8/v8/+/d293656481cfefd69dd26301c20e480bb48...
==========

[commit-bot: I haz the power, 2017-01-30 20:18:34]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/v8/v8/+/d293656481cfefd69dd26301c20e480bb48...