Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236963003:
Throw when case mapping result > max string length (Closed)
|
Created:
4 years, 4 months ago by jungshik at Google Modified:
4 years, 4 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionThrow when case mapping result > max string length
Throw 'Range Error: invalid string length' when the result of
case mapping is longer than the max string length (kMaxLength in
objects.h = 1 << 28 - 16).
This is for case mapping with ICU.
A new test (case-mapping-slow.js) is added with PASS,SLOW. It's
configured to skip unless arch=x64 and mode=release and not on
simulator.
This is a reattempt to land
https://codereview.chromium.org/2236593002 that was reverted.
BUG=v8:5271
TEST=intl/general/case-mapping-slow.js with --icu_case_mapping
Committed: https://crrev.com/4e8ebeb03c3cf08ee8b857410b056a57f5542992
Cr-Commit-Position: refs/heads/master@{#38626}
Patch Set 1 #Patch Set 2 : remove empty line #Patch Set 3 : remove test #Messages
Total messages: 22 (9 generated)
jshin@chromium.org changed reviewers: + littledan@chromium.org
How about this? PTAL
lgtm
jshin@chromium.org changed reviewers: + machenbach@chromium.org
machenbach@, would this work?
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
machenbach@chromium.org changed reviewers: + jkummerow@chromium.org
Would work, but would still add a slow test to x64 release. It was also in the tops there: https://build.chromium.org/p/client.v8/builders/V8%20Linux64/builds/12062/ste... Referring to jkummerow as I'll be OOO now. At the very least, please split the test in two, handling three cases each. Then please also skip the stress variant. You need to add an extra section to the status file for this like: https://chromium.googlesource.com/v8/v8/+/b8c050424ea92/test/mjsunit/mjsunit.... I'd not skip debug mode as our trybots run the tests in a pseudo debug mode anyways (release + dchecks =~ debug in v8). Furthermore, I think skipping simulator is not necessary as you already restricted it to v8's x64 arch. A real solution would probably be to lower the max string limit for testing purposes to get a lightning fast test. Guess we have no support for this though, Jakob do you know if that's possible?
No, we don't have a flag to reduce the max string length for testing. Maybe we should have one... I would drop the test entirely. All it really guards against is someone reverting this CL. Using .ToHandleChecked() is dangerous in general, and should always be accompanied by a comment explaining why it's safe. Nobody should feel tempted to convert an ASSIGN_RETURN_FAILURE_ON_EXCEPTION into a .ToHandleChecked(); so the risk of (accidental) revert of this change seems low. Instead of a test, I'd add a comment to ASSIGN_RETURN_FAILURE_ON_EXCEPTION instead, roughly: // Converting to upper case can increase the string's length, // e.g. for ß -> SS, so we have to handle RangeError exceptions here.
On 2016/08/12 11:06:49, Jakob wrote: > No, we don't have a flag to reduce the max string length for testing. Maybe we > should have one... https://bugs.chromium.org/p/v8/issues/detail?id=5288 > > I would drop the test entirely. All it really guards against is someone > reverting this CL. > > Using .ToHandleChecked() is dangerous in general, and should always be > accompanied by a comment explaining why it's safe. Nobody should feel tempted to > convert an ASSIGN_RETURN_FAILURE_ON_EXCEPTION into a .ToHandleChecked(); so the > risk of (accidental) revert of this change seems low. > > Instead of a test, I'd add a comment to ASSIGN_RETURN_FAILURE_ON_EXCEPTION > instead, roughly: > // Converting to upper case can increase the string's length, > // e.g. for ß -> SS, so we have to handle RangeError exceptions here. Ok. I dropped the test. BTW, it's strange that it takes over a minute on trybots. On my machine, it takes only a few seconds with release build.
> BTW, it's strange that it takes over a minute on trybots. On my machine, it > takes only a few seconds with release build. Like I explained above, release trybots turn on dchecks, which in v8 defines debug, making them near-debug builds.
lgtm
The CQ bit was checked by jshin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/12 17:13:46, machenbach (OOO until Aug 28) wrote: > > BTW, it's strange that it takes over a minute on trybots. On my machine, it > > takes only a few seconds with release build. > > Like I explained above, release trybots turn on dchecks, which in v8 defines > debug, making them near-debug builds. Thanks for the explanation and sorry for overlooking your previous explanation !
np - lgtm
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Throw when case mapping result > max string length Throw 'Range Error: invalid string length' when the result of case mapping is longer than the max string length (kMaxLength in objects.h = 1 << 28 - 16). This is for case mapping with ICU. A new test (case-mapping-slow.js) is added with PASS,SLOW. It's configured to skip unless arch=x64 and mode=release and not on simulator. This is a reattempt to land https://codereview.chromium.org/2236593002 that was reverted. BUG=v8:5271 TEST=intl/general/case-mapping-slow.js with --icu_case_mapping ========== to ========== Throw when case mapping result > max string length Throw 'Range Error: invalid string length' when the result of case mapping is longer than the max string length (kMaxLength in objects.h = 1 << 28 - 16). This is for case mapping with ICU. A new test (case-mapping-slow.js) is added with PASS,SLOW. It's configured to skip unless arch=x64 and mode=release and not on simulator. This is a reattempt to land https://codereview.chromium.org/2236593002 that was reverted. BUG=v8:5271 TEST=intl/general/case-mapping-slow.js with --icu_case_mapping Committed: https://crrev.com/4e8ebeb03c3cf08ee8b857410b056a57f5542992 Cr-Commit-Position: refs/heads/master@{#38626} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4e8ebeb03c3cf08ee8b857410b056a57f5542992 Cr-Commit-Position: refs/heads/master@{#38626} |
