updated attachment to UTF-8

Deleted: abnamro_console_output.txt 1.6 KB

abnamro_console_output.txt 1.6 KB View Download

Tested issue on Mac OS 10.11.6 using chrome latest beta M55 #55.0.2883.35 and latest canary M56 #56.0.2914.0 and issue is reproduced.

Using the per-revision bisect providing the bisect results,
Good build:55.0.2862.0(Revision: 419063).
Bad build: 55.0.2863.0 (Revision:419330).

You are probably looking for a change made after 419248 (known good), but no later than 419249 (first known bad).

CHANGELOG URL:

The script might not always return single CL as suspect as some perf builds might get missing due to failure.

  https://chromium.googlesource.com/chromium/src/+log/284a2b2247742cfa9e7039dbce4bf75900272bd6..32c0c163726879604e0d63f4a95773a86ebb251c

V8 Roll::

  https://chromium.googlesource.com/v8/v8/+log/7f777213..66c91bb5

From the CL above, assigning the issue to the concern owner 

Suspect URL :
https://chromium.googlesource.com/v8/v8/+/e1341ca8fa486bb2c9e4236672a64ec7756a164d

@marja - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Note : Adding stable blocker as this is recent regression,please feel free to remove if not required.

Thanks!

I'm ooo, routing to verwaest@

The change in question is now behind a flag.

Looking into e1341ca8fa486bb2c9e4236672a64ec7756a164d the cause might be AllowsLazyParsingWithoutUnresolvedVariables replacing previous AllowsLazyParsing and/or the newer difference between inner over outer functions (but by no means I'm some V8 expert here). Makes sense?

The suspected CL was reverted in the same roll right afterwards:
7de8639 Revert of Preparse inner functions. (patchset #23 id:440001 of https://codereview.chromium.org/2322243002/ ) by marja · 8 weeks ago

#6 This revert will make it into Canary or Beta? I want to confirm the fix if is beta.

I'm just saying that the suspected CL / bisect makes no sense. My ToT content_shell works, so now I'm bisecting to find where it was fixed.

The bug was introduced in https://codereview.chromium.org/2314663002 (642d6d314c7934a05cd2386e4b10fca3267769fc) and is still on ToT.

Looking.

Error happens in 'core.js', which seems to contain every single scanner trap known: huge & minified file, regexp + template literals, utf-8 characters (but neither declared encoding nor BOM)...

Current suspicion: utf-8 issues, again.

The source contains in core.js:
- prevText:"...",nextText:"..."
- the latter contains the bytes e2 86 92, which is utf-8 for code point 8594 (RIGHTWARDS ARROW)
- the former contains the bytes e2 86 3f, which is garbage, but almost like e2 86 90/code point 8592 (LEFTWARDS ARROW).

I presume that the stream + bulk copy implementation disagree on how to interpret this, much like  crbug.com/651333  (crrev.com/2391273002).


That said: The input is invalid, and abnamro.nl shouldn't be doing this in the first place.

I agree the page served by abnamro.nl is atrocious (amazing for a banking website...). But how did this worked in v54 then?

#12: Still investigating, but what I presently think is happening is:

When we use script streaming (i.e., JavaScript is being parsed in parallel on a background thread), we use a different implementation than the regular (non-streaming) parse. When these disagree on the character counts, you get funky syntax errors. (As we do here; or did in the referenced older bug.) That's a rather grave error and shouldn't happen; but in the past it has happened when the two implementations disagreed on how exactly to handle invalid input. That is, some sequence of non-valid utf-8 was considered one character by one implementation, and two characters by the other.

I think something similar is going on here. If so, the root cause would be erroneous input, and the follow-on cause would be nonsensical error message due to different interpretations of the same erroneous input at different parts of the code. Of course, that still needs to be fixed on our side.

(Also, I could be wrong about things. That happens, too.)

In core.js, pos 203605, source 'prevText:"..."', with ... being e2 86 3f,
- Utf8ExternalStreamingStream reads 22 65533 3f 22
- TwoByteStringUtf16CharacterStream reads 22 65533 65533 3f 22

Trying to figure out which of the two interpretations is correct.

- RFC 2279 (1998) defines UTF-8. It notes in section 2, "actual implementations of the decoding algorithm above should protect against decoding invalid sequences," but fails to inform us what exactly we should be doing when such invalid sequences are encountered.

- RFC 3629 (2003) re-defines & clarifies UTF-8. It upgrades the advice from RFC 2279 to MUST (section 3 + section 12), but still doesn't tell us what to actually do.

I asked around, and Mark Davis thankfully provided this reference:

The Unicode Standard, 9.0.0, chapter 3.9, p. 128 says:

  "Whenever an unconvertible offset is reached during conversion of a code
   unit sequence:
   1. The maximal subpart at that offset should be replaced by a single
     U+FFFD.
   2. The conversion should proceed at the offset immediately after the
      maximal subpart."

Reference: http://www.unicode.org/versions/Unicode9.0.0/ch03.pdf


This would mean Utf8ExternalStreamingStream - after the fix in crrev.com/2391273002 - is correct, and whatever converts the data going into TwoByteStringUtf16CharacterStream is the implementation that should be fixed.

On spec issues: I was reminded that both versions are technically correct, since the utf-8 RFCs only specify that invalid chars *must* be handled, but not how. I'd like to follow the Unicode recommendation, though.

---------------

It turns out there's three utf-8 decoders involved:
- incremental in v8/src/unicode.cc
- bulk, also in v8/src/unicode.cc
- Blink's, in third_party/WebKit/Source/wtf/text/TextCodecUTF8.cpp

The actual bug is a disagreement between the first and third of these.

If I change all to handle invalid char replacement as described above, the abnamro.nl page indeed loads fine. (Presumably with a 'left arrow' somewhere being replaced with a 'tofu' replacement char and a question mark somewhere, but that is the page author's issue.)

---------------

This will still take a while, since I need changes in two sub-projects. I'd also really like to add some test cases, given that the whole construct is apparently more fragile than I had hoped.

**** Bulk edit -  please ignore if not applicable ****


A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you!

Also due to Thanksgiving holidays in US, please make sure fix is ready and merged to M55 latest by 5:00 PM PT Friday, 11/18/16 (sooner the better).

Fixes are pending & currently in review, and should just barely make it into M55.

v8: crrev.com/2493143003
blink: crrev.com/2498653002

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fd40ebb1e64274ae3529f8bbe6dad6adc76cb391

commit fd40ebb1e64274ae3529f8bbe6dad6adc76cb391
Author: vogelheim <vogelheim@chromium.org>
Date: Wed Nov 16 11:02:54 2016

Return kBadChar for longest subpart of incomplete utf-8 character.

This brings the two utf-8 decoders (bulk + incremental) in line.
Technically, either behaviour was correct, since the utf-8 spec
demands incomplete utf-8 be handled, but does not specify how.
Unicode recommends that "the maximal subpart at that offset
should be replaced by a single U+FFFD," and with this change we
consistently do that. More details + spec references in the bug.

BUG= chromium:662822 

Review-Url: https://codereview.chromium.org/2493143003
Cr-Commit-Position: refs/heads/master@{#41025}

[modify] https://crrev.com/fd40ebb1e64274ae3529f8bbe6dad6adc76cb391/src/unicode.cc
[modify] https://crrev.com/fd40ebb1e64274ae3529f8bbe6dad6adc76cb391/test/cctest/test-parsing.cc

[Blink-side patch is not in yet; it's in commit queue and should be in shortly.]

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a8c03cf61f42e45e4c4d2362d97cc2c5abbdc1f

commit 9a8c03cf61f42e45e4c4d2362d97cc2c5abbdc1f
Author: vogelheim <vogelheim@chromium.org>
Date: Wed Nov 16 13:03:26 2016

Return one U+fffd for longest subpart of incomplete utf-8 character.

This brings the utf-8 decoder in line w/ the Unicode spec
recommendation, and also with the utf-8 decoder in V8. This
latter bit is the motivation for this change, since either this
or the v8 utf-8 decoder gets used for JavaScript source code, and
they should agree with each other.

More details + spec references in the bug.

BUG= chromium:662822 

Review-Url: https://codereview.chromium.org/2498653002
Cr-Commit-Position: refs/heads/master@{#432470}

[modify] https://crrev.com/9a8c03cf61f42e45e4c4d2362d97cc2c5abbdc1f/third_party/WebKit/LayoutTests/fast/encoding/char-decoding-invalid-trail.html
[modify] https://crrev.com/9a8c03cf61f42e45e4c4d2362d97cc2c5abbdc1f/third_party/WebKit/Source/wtf/text/TextCodecUTF8.cpp

Just to update, following error (Screenshot attached) is displayed when loading the URL: https://www.abnamro.nl/nl/prive/index.html in console. 

@vogelheim: Kindly review the attached screenshot and let us know if the issue still persists or if this is a different error. Verified on chrome version 56.0.2923.0 on MAC 10.12.1.

Thanks.!

Deleted: JS-Error.png 541 KB

	JS-Error.png 541 KB View Download

#23 Looks like the same error, but has the patch made it into 56.0.2923.0 ?

Your change meets the bar and is auto-approved for M55 (branch: 2883)

#23 + #24: Thanks for checking.

I think the Blink Cl was in 56.0.2923, but the V8 one wasn't. We really want both. I've committed them at roughly the same time, but there's a several hour delay between V8 commits and those commits showing up in Chromium.

So I'm not concerned, yet. :)

**** Bulk edit -  please ignore if not applicable ****

A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch latest by November 25th, 5:00 PM PST in order to make into the desktop Stable final build cut. Thank you!

Please merge the change to M55 branch 2883 ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9d524bd33dd2e8d861128499b1ffa3b3c6377628

commit 9d524bd33dd2e8d861128499b1ffa3b3c6377628
Author: jbroman <jbroman@chromium.org>
Date: Tue Nov 22 09:27:41 2016

Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG= chromium:667260 ,  chromium:662822 

Review-Url: https://codereview.chromium.org/2520053003
Cr-Commit-Position: refs/heads/master@{#41165}

[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/src/unicode-decoder.h
[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/src/unicode.cc
[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/test/unittests/BUILD.gn
[add] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/test/unittests/unicode-unittest.cc
[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/test/unittests/unittests.gyp

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9d524bd33dd2e8d861128499b1ffa3b3c6377628

commit 9d524bd33dd2e8d861128499b1ffa3b3c6377628
Author: jbroman <jbroman@chromium.org>
Date: Tue Nov 22 09:27:41 2016

Fix out-of-range access in unibrow::Utf8::CalculateValue.

This code should not access bytes out of the permitted range in order to check
the range of a possible UTF-8 value. Instead, the length check should occur
before such checks.

BUG= chromium:667260 ,  chromium:662822 

Review-Url: https://codereview.chromium.org/2520053003
Cr-Commit-Position: refs/heads/master@{#41165}

[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/src/unicode-decoder.h
[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/src/unicode.cc
[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/test/unittests/BUILD.gn
[add] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/test/unittests/unicode-unittest.cc
[modify] https://crrev.com/9d524bd33dd2e8d861128499b1ffa3b3c6377628/test/unittests/unittests.gyp

Rechecked this again on chrome version 57.0.2926.0 on MAC 10.12.1 and below is the screen shot attached for the same. Still seeing some error in console. Not sure if this is related.

@vogelheim: Request you to please confirm.

Thanks.!

Deleted: JS-Error1.png 788 KB

	JS-Error1.png 788 KB View Download

#32 No, these are not JS error but HTTP. Most likely the 401 Unauthorized are the result of some kind of login/cookie check from the website. Page seems loaded - in the previous tests after core.js errors no pictures appeared, I think not even the login button.

@ cristian.amarie: Thanks for the confirmation. 

- I'll merge into M55, today.
- Unfortunately, my fix also caused a buffer overrun (read-only, but still....  crbug.com/667260 ), so I'll merge that fix, too. 
- The utf-8 issue is indeed fixed at tip-of-tree (as confirmed by #33)
- #32 is correct in that the console still shows errors, and the original bug reports that there were none in M54.

So I think there's a second issue. I'll look into that next, once I've finished the things above. I do suspect this second thing is of lesser severity, in that the page now fully loads again.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/aadb1c83fc88c1ad3a82db8405fa6d95f8152054

commit aadb1c83fc88c1ad3a82db8405fa6d95f8152054
Author: Daniel Vogelheim <vogelheim@chromium.org>
Date: Tue Nov 22 09:54:05 2016

Merged: Return kBadChar for longest subpart of incomplete utf-8 character.

Revision: fd40ebb1e64274ae3529f8bbe6dad6adc76cb391

BUG= chromium:662822 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=hablich@chromium.org

Review URL: https://codereview.chromium.org/2526453002 .

Cr-Commit-Position: refs/branch-heads/5.5@{#52}
Cr-Branched-From: 3cbd5838bd8376103daa45d69dade929ee4e0092-refs/heads/5.5.372@{#1}
Cr-Branched-From: b3c8b0ce2c9af0528837d8309625118d4096553b-refs/heads/master@{#40015}

[modify] https://crrev.com/aadb1c83fc88c1ad3a82db8405fa6d95f8152054/src/unicode.cc
[modify] https://crrev.com/aadb1c83fc88c1ad3a82db8405fa6d95f8152054/test/cctest/test-parsing.cc

#35 Super. Can you please drop a short message after 662822 (and 667260) are merged in 55/2883? Ideally with a build number so we can check.
The 401 errors are coming from a XHR and can be due to changes done to the website done meanwhile. The error says "Failed to load resource; the server responded with a status of 401 (Unauthorized)"

Firefox says:
GET XHR https://www.abnamro.nl/session [HTTP/1.1 401 Unauthorized 310ms]
Stack:
.send https://www.abnamro.nl/nl/widgetdelivery/unauthenticated/static/js/lib/lib/require-jquery.js:31:96387
.ajax https://www.abnamro.nl/nl/widgetdelivery/unauthenticated/static/js/lib/lib/require-jquery.js:31:93542
g.ajax https://www.abnamro.nl/nl/widgetdelivery/unauthenticated/static/js/lib/core.js:25:1335
...

and Chrome:
send	@	require-jquery.js:31
ajax	@	require-jquery.js:31
g.ajax	@	core.js:25
...

The same 401 is signaled in both, and the JS call stack is pretty much the same.
I'm not saying an issue should not be filled, but since also Firefox says the same thing I assume this is a good indicator Chrome is at no fault here.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc60468fe8bd421e7e41914992d877a2a1398991

commit cc60468fe8bd421e7e41914992d877a2a1398991
Author: Daniel Vogelheim <vogelheim@chromium.org>
Date: Tue Nov 22 10:56:30 2016

Return one U+fffd for longest subpart of incomplete utf-8 character.

This brings the utf-8 decoder in line w/ the Unicode spec
recommendation, and also with the utf-8 decoder in V8. This
latter bit is the motivation for this change, since either this
or the v8 utf-8 decoder gets used for JavaScript source code, and
they should agree with each other.

More details + spec references in the bug.

BUG= chromium:662822 

Review-Url: https://codereview.chromium.org/2498653002
Cr-Commit-Position: refs/heads/master@{#432470}
(cherry picked from commit 9a8c03cf61f42e45e4c4d2362d97cc2c5abbdc1f)

Review URL: https://codereview.chromium.org/2521943002 .

Cr-Commit-Position: refs/branch-heads/2883@{#647}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/cc60468fe8bd421e7e41914992d877a2a1398991/third_party/WebKit/LayoutTests/fast/encoding/char-decoding-invalid-trail.html
[modify] https://crrev.com/cc60468fe8bd421e7e41914992d877a2a1398991/third_party/WebKit/Source/wtf/text/TextCodecUTF8.cpp

Merge should be done.
(This also requires changes on the corresponding V8 release branch, 5.5-lkgr.)


#37: Thanks for the analysis.

The build to contain all the fixes will likely be >= 55.0.2883.64. 
(I've missed the *.63 by a few hours: https://chromium.googlesource.com/chromium/src.git/+log/refs/branch-heads/2883 )

Well... as it's been back-merged to M55, I suspect it would also need a backmerge to M56.

Follow-up on #35 + #37: There's two more messages in the console. Both occur similarly on Firefox and seem to be genuine issues in the page. Also, the page is fully usable - as far as I can determine without having an account there - with the fixes. So I think the bug is fixed.

I'll still do the M56 backmerge, of course.


-------------------------------

[6351:6351:1122/162843:INFO:CONSOLE(14)] "Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.", source: https://www.abnamro.nl/nl/includes/js/segments.js (14)
[6351:6351:1122/162849:INFO:CONSOLE(17)] "Uncaught TypeError: Cannot set property 'w' of undefined", source: https://www.abnamro.nl/nl/widgetdelivery/unauthenticated/static/js/lib/dep/jquery.resize.js (17)

#42 Exactly what I saw as well. I'll check ASAP with the build 64 or later when I'll can, but I think the issue is fixed.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8a92864905039bfc359a9526b1aeb2085d5c00bf

commit 8a92864905039bfc359a9526b1aeb2085d5c00bf
Author: Daniel Vogelheim <vogelheim@chromium.org>
Date: Wed Nov 23 11:54:53 2016

Merged: Squashed multiple commits.

Merged: Return kBadChar for longest subpart of incomplete utf-8 character.
Revision: fd40ebb1e64274ae3529f8bbe6dad6adc76cb391

Merged: Fix out-of-range access in unibrow::Utf8::CalculateValue.
Revision: 9d524bd33dd2e8d861128499b1ffa3b3c6377628

BUG= chromium:662822 , chromium:667260 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=marja@chromium.org, hablich@chromium.org

Review URL: https://codereview.chromium.org/2522193002 .

Cr-Commit-Position: refs/branch-heads/5.6@{#13}
Cr-Branched-From: bdd3886218dfe76e8560eb8a18401942452ae859-refs/heads/5.6.326@{#1}
Cr-Branched-From: 879f6599eee6e1dfcbe9a24bf688b261c03e9558-refs/heads/master@{#41014}

[modify] https://crrev.com/8a92864905039bfc359a9526b1aeb2085d5c00bf/src/unicode-decoder.h
[modify] https://crrev.com/8a92864905039bfc359a9526b1aeb2085d5c00bf/src/unicode.cc
[modify] https://crrev.com/8a92864905039bfc359a9526b1aeb2085d5c00bf/test/cctest/test-parsing.cc
[modify] https://crrev.com/8a92864905039bfc359a9526b1aeb2085d5c00bf/test/unittests/BUILD.gn
[add] https://crrev.com/8a92864905039bfc359a9526b1aeb2085d5c00bf/test/unittests/unicode-unittest.cc
[modify] https://crrev.com/8a92864905039bfc359a9526b1aeb2085d5c00bf/test/unittests/unittests.gyp

V8 patches are back-merged into 5.6. The Blink-side fix has made it in before the M56 branch, so no backmerge to M56/2924 needed.


[Blink CL: rev 9a8c03cf61f42e45e4c4d2362d97cc2c5abbdc1f, pos 432470; branch for 2924, pos 433059]

Tested this issue in Windows 10, Ubuntu 14.04 and Mac 10.11.6 with chrome beta version M55-55.0.2883.70 

Observed that on loading "https://www.abnamro.nl/nl/prive/index.html" site, there are js query errors in the console.
Please look into the attached screen-cast and let us know your observations.

Deleted: Issue 662822.mp4 1.1 MB

	Issue 662822.mp4 1.1 MB View Download

#47 Check comments 35 and 37. These are not the JS errors reported in bug (in fact, are not errors at all - one is a XHR deprecation warning, and the next 2 are HTTP 401 authorization codes returned also from a XHR).

Verified the fix on Chrome version 55.0.2883.70 on Windows 7,10, Mac and Linux, please find the steps followed below :

Steps followed :
1. Open chrome and navigate to https://www.abnamro.nl/nl/prive/index.html

Please find the attached screenshots from before the fix(55.0.2883.59) and after the fix(55.0.2883.70), Please let me know if I need to verify more which I have missed.

Deleted: 55.0.2883.59.png 379 KB

	55.0.2883.59.png 379 KB View Download

Deleted: 55.0.2883.70.png 174 KB

	55.0.2883.70.png 174 KB View Download

#50 Check comment 35. Reported bug is fixed; what you see is a XHR answered with 401 from server.

Thanks for the update.
Verified this issue on 55.0.2883.71 and didn't see errors except 401 (Unauthorized) 

Hence adding TE verified labels 

Not needed for Node.js as the issue is not present in V8 5.4 or older (correct me if I am wrong here).

Re #52 correct.