New issue
Advanced search Search tips

Issue 867958 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Aug 1
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Regression



Sign in to add a comment

DCHECK failure in *source_map != *feedback in feedback-vector.cc

Project Member Reported by ClusterFuzz, Jul 26

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=6744125769252864

Fuzzer: ochang_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  *source_map != *feedback in feedback-vector.cc
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=54594:54595

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6744125769252864

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, Jul 26

Labels: Test-Predator-Auto-Owner
Owner: ca...@igalia.com
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/v8/v8/+/b6f7ea580595f98b89fc47c50f9ccfbbd3b9c448 ([runtime] use new CloneObject bytecode for some ObjectLiteralSpread cases).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Status: Started (was: Assigned)
Cc: jkummerow@chromium.org
jkummerow, question:

I had originally written this to miss if it encounters a deprecated Map, e.g.

```
var obj1 = { x: 1 };
var obj2 = { x: 2 }; // same map
obj2.x = null; // deprecate map

function f() { return { ...obj1 } };
f(); // uninitialized with deprecated map -> miss
f(); // monomorphic with deprecated map -> miss (hit this DCHECK)
```

But, it seems like in this case it should be completely fine to fast-clone the object with the deprecated map.

Is it safe to get rid of the miss-if-deprecated line? It's really hard for me to tell, the only thing that seems to mark a map as deprecated (that I can find from a quick grep) is Map::TransitionToDataProperty(), but if an object is still holding the deprecated map, then doesn't that de facto mean that the map is still valid for that object, and should not result in a miss?
Labels: -Type-Bug-Security -Restrict-View-SecurityTeam -Security_Severity-High Pri-1 Type-Bug-Regression
It's true that deprecated maps are still safe/valid to use, but we generally want to migrate all objects away from them ASAP to avoid polymorphism elsewhere. So please keep missing when a deprecated map is encountered.

(If anything, maybe we should also miss when the target map has been deprecated...)

You can extend the checks for IsClearedWeakHeapObject in both MONOMORPHIC and POLYMORPHIC to also take the overwriting branch for deprecated maps.

I'm pretty sure that this DCHECK failure does not have security implications, so lifting restrictions.
Ok, fair enough
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 27

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

commit 670fa86fd171670c86528fc0fad2b262ea19be74
Author: Caitlin Potter <caitp@igalia.com>
Date: Fri Jul 27 19:37:39 2018

[CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated

An object with a deprecated Map which has already been cached in
CloneObjectIC feedback is still a valid Map for fast cloning --- but
to be consistent with other ICs, deprecated maps are ignored, and are
expected to be transitioned away from.

If the source object has a deprecated map, the instance is migrated.

BUG=v8:7611,  chromium:867958 
R=jkummerow@chromium.org, mvstanton@chromium.org

Change-Id: I9771b00400fb4dda45a62e874a31d9b50630d847
Reviewed-on: https://chromium-review.googlesource.com/1152414
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Caitlin Potter <caitp@igalia.com>
Cr-Commit-Position: refs/heads/master@{#54758}
[modify] https://crrev.com/670fa86fd171670c86528fc0fad2b262ea19be74/src/feedback-vector.cc
[modify] https://crrev.com/670fa86fd171670c86528fc0fad2b262ea19be74/src/ic/ic.cc
[add] https://crrev.com/670fa86fd171670c86528fc0fad2b262ea19be74/test/mjsunit/es9/regress/regress-867958.js

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 27

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

commit 43098ecbe846d4af5537987f68b61f2f504aca5e
Author: Deepti Gandluri <gdeepti@chromium.org>
Date: Fri Jul 27 21:39:00 2018

Revert "[CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated"

This reverts commit 670fa86fd171670c86528fc0fad2b262ea19be74.

Reason for revert: Causes gc-stress bots to fail - 
https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket.appspot.com/8939819990688690880/+/steps/Check_-_d8/0/logs/object-spread-ic/0
https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket.appspot.com/8939819696982258192/+/steps/Check_-_d8/0/logs/object-spread-ic/0

Original change's description:
> [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated
> 
> An object with a deprecated Map which has already been cached in
> CloneObjectIC feedback is still a valid Map for fast cloning --- but
> to be consistent with other ICs, deprecated maps are ignored, and are
> expected to be transitioned away from.
> 
> If the source object has a deprecated map, the instance is migrated.
> 
> BUG=v8:7611,  chromium:867958 
> R=​jkummerow@chromium.org, mvstanton@chromium.org
> 
> Change-Id: I9771b00400fb4dda45a62e874a31d9b50630d847
> Reviewed-on: https://chromium-review.googlesource.com/1152414
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Commit-Queue: Caitlin Potter <caitp@igalia.com>
> Cr-Commit-Position: refs/heads/master@{#54758}

TBR=jkummerow@chromium.org,mvstanton@chromium.org,caitp@igalia.com

Change-Id: Id17652ad6f3f09adb43848069549ad146d48b2d7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:7611,  chromium:867958 
Reviewed-on: https://chromium-review.googlesource.com/1153747
Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Commit-Queue: Deepti Gandluri <gdeepti@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54760}
[modify] https://crrev.com/43098ecbe846d4af5537987f68b61f2f504aca5e/src/feedback-vector.cc
[modify] https://crrev.com/43098ecbe846d4af5537987f68b61f2f504aca5e/src/ic/ic.cc
[delete] https://crrev.com/6cd560b265cc3af5e31bcc883c235e7680b8d0a8/test/mjsunit/es9/regress/regress-867958.js

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 31

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

commit d9f6c685f008ab8b669492b1c10ec44a9883de76
Author: Caitlin Potter <caitp@igalia.com>
Date: Tue Jul 31 04:11:05 2018

Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated

An object with a deprecated Map which has already been cached in
CloneObjectIC feedback is still a valid Map for fast cloning --- but
to be consistent with other ICs, deprecated maps are ignored, and are
expected to be transitioned away from.

If the source object has a deprecated map, the instance is migrated.

BUG=v8:7611,  chromium:867958 ,  chromium:868586 
R=jkummerow@chromium.org, mvstanton@chromium.org

Change-Id: I477aec6c8d0ae1e1648a70e85d2fd46146521d1c
Reviewed-on: https://chromium-review.googlesource.com/1154143
Commit-Queue: Caitlin Potter <caitp@igalia.com>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54799}
[modify] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/src/feedback-vector.cc
[modify] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/src/ic/ic.cc
[add] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/test/mjsunit/es9/regress/regress-867958.js

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 31

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

commit 3879e8bfe3c8c3fdf05e772f741feda3209304cf
Author: Michael Achenbach <machenbach@chromium.org>
Date: Tue Jul 31 06:29:49 2018

Revert "Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated"

This reverts commit d9f6c685f008ab8b669492b1c10ec44a9883de76.

Reason for revert:
https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux%20-%20gc%20stress/17584

Original change's description:
> Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated
> 
> An object with a deprecated Map which has already been cached in
> CloneObjectIC feedback is still a valid Map for fast cloning --- but
> to be consistent with other ICs, deprecated maps are ignored, and are
> expected to be transitioned away from.
> 
> If the source object has a deprecated map, the instance is migrated.
> 
> BUG=v8:7611,  chromium:867958 ,  chromium:868586 
> R=​jkummerow@chromium.org, mvstanton@chromium.org
> 
> Change-Id: I477aec6c8d0ae1e1648a70e85d2fd46146521d1c
> Reviewed-on: https://chromium-review.googlesource.com/1154143
> Commit-Queue: Caitlin Potter <caitp@igalia.com>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#54799}

TBR=jkummerow@chromium.org,mvstanton@chromium.org,caitp@igalia.com

Change-Id: Ifcb422c3a692543490710d450590323524a6359a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:7611,  chromium:867958 ,  chromium:868586 
Reviewed-on: https://chromium-review.googlesource.com/1155593
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54800}
[modify] https://crrev.com/3879e8bfe3c8c3fdf05e772f741feda3209304cf/src/feedback-vector.cc
[modify] https://crrev.com/3879e8bfe3c8c3fdf05e772f741feda3209304cf/src/ic/ic.cc
[delete] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/test/mjsunit/es9/regress/regress-867958.js

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 1

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

commit 5caee70b664e81ccd57453a64757573aeb070107
Author: Caitlin Potter <caitp@igalia.com>
Date: Wed Aug 01 00:30:11 2018

Reland "Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated"

An object with a deprecated Map which has already been cached in
CloneObjectIC feedback is still a valid Map for fast cloning --- but
to be consistent with other ICs, deprecated maps are ignored, and are
expected to be transitioned away from.

If the source object has a deprecated map, the instance is migrated.

BUG=v8:7611,  chromium:867958 ,  chromium:868586 ,  chromium:869342 ,  chromium:869347 ,  chromium:869293 
R=jkummerow@chromium.org, mvstanton@chromium.org

Reviewed-on: https://chromium-review.googlesource.com/1154143
Commit-Queue: Caitlin Potter <caitp@igalia.com>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#54799}
Change-Id: I6e2f7b28c41bb9bd6255441da0f209a97bce5e8f
Reviewed-on: https://chromium-review.googlesource.com/1157142
Cr-Commit-Position: refs/heads/master@{#54830}
[modify] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/src/feedback-vector.cc
[modify] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/src/ic/ic.cc
[add] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/test/mjsunit/es9/regress/regress-867958.js
[add] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/test/mjsunit/es9/regress/regress-869342.js

Project Member

Comment 11 by ClusterFuzz, Aug 1

ClusterFuzz has detected this issue as fixed in range 54829:54830.

Detailed report: https://clusterfuzz.com/testcase?key=6744125769252864

Fuzzer: ochang_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  *source_map != *feedback in feedback-vector.cc
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=54594:54595
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=54829:54830

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6744125769252864

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 12 by ClusterFuzz, Aug 1

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 6744125769252864 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment