Transitioning store of unbox double field races with concurrent marker. |
|||||||||||||||||
Issue descriptionRacing scenario: 1) Map A is a JSObject map with unused in-object fields. 2) Object X with map A is created, initialized, and published. 3) Concurrent marker sees object X and starts snapshotting all its fields (including the unused fields). 4) At the same time object X transitions to map B by appending an unboxed double field, which overwrites an unused field to an unboxed double value. 5) Concurrent marker snapshots the unboxed double value. 6) Concurrent marker transitions the object from grey to black. 6) Concurrent marker iterates the snapshot and treats unboxed double value as tagged pointer. The bug here is that the concurrent marker snapshots all fields starting from offset 0 to A.instance_size(). Instead of that, it must snapshot only the fields that are in-use according to map A. For that we would need to add A.last_inobject_field_index() counter to maps.
,
Oct 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/2a3cab7aa64a242abc4c0d472c5804ce22408126 commit 2a3cab7aa64a242abc4c0d472c5804ce22408126 Author: Ulan Degenbaev <ulan@chromium.org> Date: Thu Oct 19 14:45:43 2017 [runtime] Refactor initialization of in-object property count of a map. This patch moves initialization of inobject_properties and unused_property_fields of a map to the construction time of the map. Map::AppendDescriptor now properly decrements unused_property_fields and thus maintains the invariant for property field counters. Bug: chromium:774644 Change-Id: I78e5d5c767e22148cb64e8cabe0564e7a13988f5 Reviewed-on: https://chromium-review.googlesource.com/725726 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#48751} [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/bootstrapper.cc [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/factory.cc [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/factory.h [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/heap/heap.cc [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/heap/heap.h [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/objects-inl.h [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/objects.cc [modify] https://crrev.com/2a3cab7aa64a242abc4c0d472c5804ce22408126/src/objects/map.h
,
Oct 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92 commit f13dcb49da767b29673de0d8b38d1e4ad5e5cc92 Author: Ulan Degenbaev <ulan@chromium.org> Date: Fri Oct 20 15:41:11 2017 Add a byte field to Map that tracks used in-object property fields. The motivation for the new field is to provide race-free way to iterate used in-object properties of a JSObject in concurrent marker. This CL keeps the new field in sync with the unused_property_fields and subsequent CL will remove unused_property_fields. Bug: chromium:774644 Change-Id: I0971f079094d58d3a57415834c43c09427dacc77 Reviewed-on: https://chromium-review.googlesource.com/726639 Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Tobias Tebbi <tebbi@chromium.org> Cr-Commit-Position: refs/heads/master@{#48795} [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/bootstrapper.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/compiler/js-native-context-specialization.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/factory.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/heap/heap.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/ic/handler-configuration.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/objects-debug.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/objects-inl.h [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/objects-printer.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/objects.cc [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/objects/map.h [modify] https://crrev.com/f13dcb49da767b29673de0d8b38d1e4ad5e5cc92/src/runtime/runtime-literals.cc
,
Oct 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/80442c0c42204cb9472e65483e6d8a8d72c24671 commit 80442c0c42204cb9472e65483e6d8a8d72c24671 Author: Ulan Degenbaev <ulan@chromium.org> Date: Mon Oct 23 09:14:25 2017 [heap] Iterate the only used fields of JSObject in concurrent marker. Currently the concurrent marker iterates all fields in JSObjects up to the instance size defined by the map. This can lead to a race when the object transitions to unboxed double field. Bug: chromium:774644 Change-Id: I01a69240869217127769bba9ff1c49dc5a81fa9c Reviewed-on: https://chromium-review.googlesource.com/730717 Reviewed-by: Hannes Payer <hpayer@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#48817} [modify] https://crrev.com/80442c0c42204cb9472e65483e6d8a8d72c24671/src/heap/concurrent-marking.cc [modify] https://crrev.com/80442c0c42204cb9472e65483e6d8a8d72c24671/src/objects-inl.h [modify] https://crrev.com/80442c0c42204cb9472e65483e6d8a8d72c24671/src/objects/map.h
,
Oct 25 2017
,
Oct 26 2017
Issue 772369 has been merged into this issue.
,
Oct 26 2017
Users experienced this crash on the following builds: Win Dev 63.0.3239.9 - 0.61 CPM, 267 reports, 229 clients (signature v8::base::AsAtomic32::SetBits<unsigned int>) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Oct 26 2017
ulan@ please let us know if we need any Cl's to get merged to M63 to address the crashes.
,
Oct 30 2017
Requesting merge to M63 of V8 CL in comment 2, 3, 4. They have canary coverage and fix some of the crashes in 768127 and 772369.
,
Oct 30 2017
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 30 2017
+danno@ and adamk@ for M63 merge review as hablich@ is on vacation.
,
Oct 30 2017
These changes look a little bigger and more invasive than I'd normally like to see for a merge. And my understanding (and ulan should correct me if I'm wrong) is that these crashes could also be avoided by turning off --concurrent-marking flag on the M63 branch. Can ulan@ or verwaest@ (who reviewed the Map changes) make an argument that these changes are less risky than their size might suggest? Or for why we should patch them in vs. turning off the feature and waiting for another milestone?
,
Nov 2 2017
Just to update, current beta# 63.0.3239.30 has reported 19 instances of this crash. Previous Beta# 63.0.3239.18 has reported 652 number of instances of this crash. On M64 builds, no instances are reported after build#64.0.3251.1. Below link gives in detail for the total number of instances in which the crash has occurred for associated builds. https://goto.google.com/mxzxu Thanks.!
,
Nov 2 2017
Adam, I think the changes are safe because: - CL in #2 does not change behavior. - CL in #3 does not change behavior. It re-purposes an unused byte in Map, but the byte is still unused. - CL in #4 is the real change. It is local to the concurrent marker and uses the byte introduced in #3. - The CLs landed 9-11 days after the branch, there were no big changes in the affected code. - We have canary coverage. Turning concurrent marking off is an option, but it is not risk-free. Most of the crashes in 772369 and 768127 come from the old "Memory corruption (v8)" bucket (which filters crashes in incremental marking). "Memory corruption (v8)" crashes dropped from top #5 in M62 to top #16 in M63: https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20product.version%3D%2762.0.3202.62%27%20AND%20custom_data.ChromeCrashProto.channel%3D%27beta%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27renderer%27&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#magicsignature:20 https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20product.version%3D%2763.0.3239.18%27%20AND%20custom_data.ChromeCrashProto.channel%3D%27beta%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27renderer%27&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#magicsignature:30 Turing concurrent marking off will bring those crashes back.
,
Nov 2 2017
I'm also in favor of just disabling concurrent marking. The changes look safe enough; but are fairly large, changing the workings of a core part of V8, so who knows. In general we shouldn't implement new features on a branch, even to fix issues. We should at most merge back minor bugfixes. If it's easy enough to just disable the feature, as it is here, then just do that instead. There's always the next release to stabilize the feature and get it in.
,
Nov 2 2017
Thanks, Toon. Let's go with disabling concurrent marking. I created a CL: https://chromium-review.googlesource.com/c/v8/v8/+/749811 I'll merge it back after canary coverage.
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/a9a50dc9a89b4e9a78e7b0d048466f8717157a86 commit a9a50dc9a89b4e9a78e7b0d048466f8717157a86 Author: Ulan Degenbaev <ulan@chromium.org> Date: Thu Nov 02 14:20:32 2017 [heap] Temporarily disable concurrent marking. The 6.3 branch has a data race that is fixed in 6.4 but the fix is too large for back merging. This CL will be back-merged to 6.3 after getting Canary coverage. Concurrent marking will be re-enabled afterwards. Bug: chromium:774644 Change-Id: I4112da0e133a637cc4fb52dee2e4c165cdc74f1f Reviewed-on: https://chromium-review.googlesource.com/749811 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#49080} [modify] https://crrev.com/a9a50dc9a89b4e9a78e7b0d048466f8717157a86/BUILD.gn [modify] https://crrev.com/a9a50dc9a89b4e9a78e7b0d048466f8717157a86/gypfiles/features.gypi
,
Nov 2 2017
Re #15: Totally agree with verwaest@, we shouldn't implement new features on a branch (Note: M63 feature freeze was on 09/29, branched on 10/12 and currently in Beta) Please update the bug once CL listed at #17 is well baked/verified in Canary and safe to merge. Thank you.
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/49c62872dbd11d16373c3e894257348c33267bab commit 49c62872dbd11d16373c3e894257348c33267bab Author: Ulan Degenbaev <ulan@chromium.org> Date: Thu Nov 02 15:47:57 2017 Revert "[heap] Temporarily disable concurrent marking." This reverts commit a9a50dc9a89b4e9a78e7b0d048466f8717157a86. Reason for revert: buildbot crashes. Original change's description: > [heap] Temporarily disable concurrent marking. > > The 6.3 branch has a data race that is fixed in 6.4 but the fix is too > large for back merging. > > This CL will be back-merged to 6.3 after getting Canary coverage. > > Concurrent marking will be re-enabled afterwards. > > Bug: chromium:774644 > Change-Id: I4112da0e133a637cc4fb52dee2e4c165cdc74f1f > Reviewed-on: https://chromium-review.googlesource.com/749811 > Reviewed-by: Michael Lippautz <mlippautz@chromium.org> > Commit-Queue: Ulan Degenbaev <ulan@chromium.org> > Cr-Commit-Position: refs/heads/master@{#49080} TBR=ulan@chromium.org,hpayer@chromium.org,mlippautz@chromium.org Change-Id: Ia9d2128c01b811073c1c8f0392eb13b7d7745cd1 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:774644 Reviewed-on: https://chromium-review.googlesource.com/751501 Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#49083} [modify] https://crrev.com/49c62872dbd11d16373c3e894257348c33267bab/BUILD.gn [modify] https://crrev.com/49c62872dbd11d16373c3e894257348c33267bab/gypfiles/features.gypi
,
Nov 2 2017
CL listed at #17 got reverted at #19. What option we've left for M63?
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/586067e45dd542a117b67ad66d3b14475d8d571f commit 586067e45dd542a117b67ad66d3b14475d8d571f Author: Ulan Degenbaev <ulan@chromium.org> Date: Thu Nov 02 19:10:00 2017 Reland "[heap] Temporarily disable concurrent marking." This is a reland of a9a50dc9a89b4e9a78e7b0d048466f8717157a86 Buildbot crashes are fixed by a274fc6. Original change's description: > [heap] Temporarily disable concurrent marking. > > The 6.3 branch has a data race that is fixed in 6.4 but the fix is too > large for back merging. > > This CL will be back-merged to 6.3 after getting Canary coverage. > > Concurrent marking will be re-enabled afterwards. > > Bug: chromium:774644 > Change-Id: I4112da0e133a637cc4fb52dee2e4c165cdc74f1f > Reviewed-on: https://chromium-review.googlesource.com/749811 > Reviewed-by: Michael Lippautz <mlippautz@chromium.org> > Commit-Queue: Ulan Degenbaev <ulan@chromium.org> > Cr-Commit-Position: refs/heads/master@{#49080} Bug: chromium:774644 Change-Id: Idf5d179eca25a1481c70c6ca3bccde4869deb544 Reviewed-on: https://chromium-review.googlesource.com/751271 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#49090} [modify] https://crrev.com/586067e45dd542a117b67ad66d3b14475d8d571f/BUILD.gn [modify] https://crrev.com/586067e45dd542a117b67ad66d3b14475d8d571f/gypfiles/features.gypi
,
Nov 6 2017
Requesting merge for #21. The latest canary looks good. I'll will merge back tomorrow after checking canary again.
,
Nov 6 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 6 2017
adamk@, could you please do M63 merge review?
,
Nov 6 2017
,
Nov 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/f35b7f41df2d5ed341fdfaa4d3ed5da0cf390312 commit f35b7f41df2d5ed341fdfaa4d3ed5da0cf390312 Author: Ulan Degenbaev <ulan@chromium.org> Date: Tue Nov 07 10:33:40 2017 Merged: Reland "[heap] Temporarily disable concurrent marking." Revision: 586067e45dd542a117b67ad66d3b14475d8d571f BUG= chromium:774644 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=mlippautz@chromium.org Change-Id: I4e4aaa292ddb1913ba9e34ce6c92ec42da1ff5d3 Reviewed-on: https://chromium-review.googlesource.com/756743 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/branch-heads/6.3@{#53} Cr-Branched-From: 094a7c93dcdcd921de3883ba4674b7e1a0feffbe-refs/heads/6.3.292@{#1} Cr-Branched-From: 18b8fbb528a8021e04a029e06eafee50b918bce0-refs/heads/master@{#48432} [modify] https://crrev.com/f35b7f41df2d5ed341fdfaa4d3ed5da0cf390312/BUILD.gn [modify] https://crrev.com/f35b7f41df2d5ed341fdfaa4d3ed5da0cf390312/gypfiles/features.gypi
,
Nov 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/6bb1d47e6e82f75ead36cdab209afc5dafa8329c commit 6bb1d47e6e82f75ead36cdab209afc5dafa8329c Author: Ulan Degenbaev <ulan@chromium.org> Date: Tue Nov 07 13:11:10 2017 [heap] Re-enable concurrent marking. Bug: chromium:774644 , chromium:694255 Change-Id: I957037b14bf6508e774d6fd1c97239b31f2296e8 Reviewed-on: https://chromium-review.googlesource.com/756893 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#49187} [modify] https://crrev.com/6bb1d47e6e82f75ead36cdab209afc5dafa8329c/BUILD.gn [modify] https://crrev.com/6bb1d47e6e82f75ead36cdab209afc5dafa8329c/gypfiles/features.gypi
,
Nov 7 2017
,
Nov 7 2017
Seems like this is already merged to M63 at #26. If nothing is pending for M63, pls remove "Merge-Approved-63" label. Thank you. Not sure cl listed at #27 needs to be merged to M63 or not.
,
Nov 7 2017
,
Nov 7 2017
No need to merge #27.
,
Nov 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/d8c355fcac70311b98409257dc01fc3744bf77af commit d8c355fcac70311b98409257dc01fc3744bf77af Author: Igor Sheludko <ishell@chromium.org> Date: Tue Nov 21 14:07:04 2017 [runtime] Stop using Map::unused_property_fields() byte. The unused properties fields number is calculatable via used in-object properties count and we can drop it now. Bug: chromium:774644 Change-Id: I7388af7772a8e793593fabc46527886cf2e36095 Reviewed-on: https://chromium-review.googlesource.com/781465 Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Commit-Queue: Igor Sheludko <ishell@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#49542} [modify] https://crrev.com/d8c355fcac70311b98409257dc01fc3744bf77af/src/code-stub-assembler.cc [modify] https://crrev.com/d8c355fcac70311b98409257dc01fc3744bf77af/src/heap/heap.cc [modify] https://crrev.com/d8c355fcac70311b98409257dc01fc3744bf77af/src/objects-inl.h [modify] https://crrev.com/d8c355fcac70311b98409257dc01fc3744bf77af/src/objects.cc [modify] https://crrev.com/d8c355fcac70311b98409257dc01fc3744bf77af/src/objects/map.h [modify] https://crrev.com/d8c355fcac70311b98409257dc01fc3744bf77af/test/cctest/test-inobject-slack-tracking.cc |
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by u...@chromium.org
, Oct 13 2017