We suspect a bug in computed properties and speculative autocomplete

Able to reproduce issue on reported chrome version 69.0.3497.81 & on latest chrome 71.0.3548.0 using Windows 10 and Ubuntu 14.04 and Mac 10.13.6. Hence providing bisect information below.

Bisect Info:
================
Good build: 69.0.3450.0
Bad build: 69.0.3451.0

CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/73c072de3f8263c327379a0eae67adcbfdc5b8f5..fb9e6c3849372fb88824199e1d076b6a921d8fcc 

https://chromium.googlesource.com/chromium/src/+/fb9e6c3849372fb88824199e1d076b6a921d8fcc

suspect: https://chromium.googlesource.com/v8/v8/+log/45c9adae..acb93ae9

Dan Elphick , jgruber and Simon Zünd@: CC'ing Dev for confirm the issue and help in re-assigning. As we are not sure about the CL and adding RBS to it please remove if not required. chrome crash ID(Uploaded Crash Report ID e9d7923a4f8e1d6c (Local Crash ID: 6fffe12b-809b-4c53-a2f6-64d47008a6bf)

Thanks!

Deleted: crash.png 25.0 KB

	crash.png 25.0 KB View Download

I can repro locally. The link the crash report from #3: https://crash.corp.google.com/browse?q=ReportID%3D%27e9d7923a4f8e1d6c%27&stbtiq=&reportid=&index=0

The CL range doesn't contain a clear culprit. A possible guess is that we fail the 

 IsValidPositiveSmi(UncheckedCast<IntPtrT>(size_in_bytes))

check in CSA::AllocateRaw, but the other CLs could also cause a crash.

I'll dig deeper tomorrow.

Bisected this to

 9461aa5 [Interpreter] Enable sharing of load / store named property feedback by Mythri · 3 months ago
 https://chromium-review.googlesource.com/1065737

Since this is the only related bug report I don't think we need to block the stable release, but Mythri will have better information.

Mythri, ptal. 

Thanks Jakob. I agree that this need not block stable release. I am looking into it, but I am not entirely sure my cl is the root cause. It might have caused some indirect impact. I am still checking to see what is the problem. 

Digging more into it, it crashes with following DCHECK failure:

# Fatal error in ../../v8/src/interpreter/bytecode-array-accessor.cc, line 55
# Debug check failed: !Bytecodes::IsPrefixScalingBytecode(current_bytecode).

So either there is a bug in bytecode generation or while iterating the bytecode or may be some corner case related to debugger and bytecode iterator (since we need to execute something in the console to reproduce this bug). If there is a bug in bytecode generation or iteration it should have a more wide range impact. So I suspect we are hitting some corner case. 

I am still not sure what is the exact problem. I am less convinced that the suspect cl caused this problem though.  

I can reproduce this problem even with my cl reverted. So, it is not related to that cl. The failing stack trace:

#FailureMessage Object: 0x7fffce3940a0#0 0x7f62312f29fd base::debug::StackTrace::StackTrace()
#1 0x7f6230ffc9ac base::debug::StackTrace::StackTrace()
#2 0x7f62166297b7 gin::(anonymous namespace)::PrintStackTrace()
#3 0x7f61fe2ea8f8 V8_Fatal()
#4 0x7f61fe2ea665 v8::base::(anonymous namespace)::DefaultDcheckHandler()
#5 0x7f6215b4337e v8::internal::interpreter::BytecodeArrayAccessor::current_bytecode()
#6 0x7f6215b433d7 v8::internal::interpreter::BytecodeArrayAccessor::current_bytecode_size()
#7 0x7f6215b764d3 v8::internal::interpreter::BytecodeArrayIterator::Advance()
#8 0x7f621599b255 v8::internal::Debug::ClearSideEffectChecks()
#9 0x7f621599adc1 v8::internal::Debug::UpdateDebugInfosForExecutionMode()
#10 0x7f621597ec10 v8::internal::DebugEvaluate::Global()
#11 0x7f62155ad415 v8::debug::EvaluateGlobal()


May be related to side effect free checks. adding Alexey if can add any more info. I will dig deeper tomorrow.

+Yang for side-effect-free debug eval.

Moving to "M-70" as this is not M69 blocker per comments #7 and #8. Pls target fix for M70.

I can reproduce as well. It would be useful to print the bytecode before and after Debug::ApplySideEffectChecks.

Printing after calling Debug::ApplySideEffectChecks crashes while printing, as we run into an unreacheable part of Bytecodes::ToString, which probably because we corrupted the bytecode during patching. Looking a bit further into this.

Found the issue. We are patching the scaling prefix, but with a regular debug break bytecode, not with a prefix-specific debug break bytecode.

I'll find a simpler repro and upload a fix.

The NextAction date has arrived: 2018-09-12

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

commit ac1660c660e42b91adfb38c951fe3dd054ab8114
Author: Yang Guo <yangguo@chromium.org>
Date: Fri Sep 14 14:33:07 2018

[debug] fix scaling prefix patching for debug evaluate

R=jgruber@chromium.org

Bug:  chromium:882664 
Change-Id: I12248de9a01839433daa40e8273a18a15a9867bb
Reviewed-on: https://chromium-review.googlesource.com/1221547
Commit-Queue: Yang Guo <yangguo@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55910}
[modify] https://crrev.com/ac1660c660e42b91adfb38c951fe3dd054ab8114/src/debug/debug-evaluate.cc
[modify] https://crrev.com/ac1660c660e42b91adfb38c951fe3dd054ab8114/src/debug/debug.cc
[modify] https://crrev.com/ac1660c660e42b91adfb38c951fe3dd054ab8114/src/interpreter/bytecode-array-accessor.cc
[modify] https://crrev.com/ac1660c660e42b91adfb38c951fe3dd054ab8114/src/interpreter/bytecode-array-accessor.h
[add] https://crrev.com/ac1660c660e42b91adfb38c951fe3dd054ab8114/test/debugger/regress/regress-crbug-882664.js

[Auto-generated comment by a script] We noticed that this issue is targeted for M-70; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-70 label, otherwise remove Merge-TBD label. Thanks.

Do we need to land this in M70? Seems like a stable blocker. If this is a safe merge and necessary for M70, can you please request a merge?

Issue 863925 has been merged into this issue.

yangguo@ can you please confirm if this is verified and safe enough to be merged to M70?

Issue 862314 has been merged into this issue.

Sorry for not replying. I somehow overlooked this.

Yes. This should be safe for merging now. I will do this asap.

This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit c823665ee889819b1e8a6e9c8331722caaf4bdd7
Author: Yang Guo <yangguo@chromium.org>
Date: Fri Oct 19 07:31:27 2018

Merged: [debug] fix scaling prefix patching for debug evaluate

Revision: ac1660c660e42b91adfb38c951fe3dd054ab8114

BUG= chromium:882664 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
TBR=jgruber@chromium.org

Change-Id: Idcf912987b0cbe0a96e904de65779ee8c5391098
Reviewed-on: https://chromium-review.googlesource.com/c/1290789
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/branch-heads/7.0@{#63}
Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1}
Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424}
[modify] https://crrev.com/c823665ee889819b1e8a6e9c8331722caaf4bdd7/src/debug/debug-evaluate.cc
[modify] https://crrev.com/c823665ee889819b1e8a6e9c8331722caaf4bdd7/src/debug/debug.cc
[modify] https://crrev.com/c823665ee889819b1e8a6e9c8331722caaf4bdd7/src/interpreter/bytecode-array-accessor.cc
[modify] https://crrev.com/c823665ee889819b1e8a6e9c8331722caaf4bdd7/src/interpreter/bytecode-array-accessor.h
[add] https://crrev.com/c823665ee889819b1e8a6e9c8331722caaf4bdd7/test/debugger/regress/regress-crbug-882664.js

 Issue 897661  has been merged into this issue.