Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(344)

Issue 2891843003: Fix bug causing DCHECKs to be hit in DocumentMarkerController (Closed)

Created:
3 years, 7 months ago by rlanday
Modified:
3 years, 7 months ago
Reviewers:
yosin_UTC9, Xiaocheng
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix bug causing DCHECKs to be hit in DocumentMarkerController These DCHECKs check that we're resetting the possibly_has_marker_types_ flag when markers_ becomes empty. Resetting this flag properly enables a performance optimization. These DCHECKs are causing sporadic crashes in debug builds; we believe this is because the markers_ map is holding weak references to its keys (Node objects), so it's possible for the map to become empty through garbage collection, which doesn't reset possibly_has_marker_types_. This CL modifies DocumentMarkerController::PossiblyHasMarkers() to catch this case. Alternatively, we could handle this case at garbage collection time, but we believe this approach has better performance characteristics. BUG=685755 Review-Url: https://codereview.chromium.org/2891843003 Cr-Commit-Position: refs/heads/master@{#473400} Committed: https://chromium.googlesource.com/chromium/src/+/37c906bd514195222ab091fa98a461cc40b25577

Patch Set 1 #

Total comments: 1

Patch Set 2 : Modify PossiblyHasMarkers() instead of removing DCHECKs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -0 lines) Patch
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp View 1 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 30 (13 generated)
rlanday
3 years, 7 months ago (2017-05-18 02:51:04 UTC) #3
rlanday
kouhei: are we correctly understanding the behavior of HeapHashMap when used with WeakMember? It removes ...
3 years, 7 months ago (2017-05-18 03:25:17 UTC) #6
yosin_UTC9
https://codereview.chromium.org/2891843003/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (left): https://codereview.chromium.org/2891843003/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#oldcode167 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:167: DCHECK(!markers_.IsEmpty()); Rather than removing DCHECK, how about changing PossiblyHasMarkers(..) ...
3 years, 7 months ago (2017-05-18 04:29:08 UTC) #7
rlanday
Hmm...don't we still need to remove the DCHECKs though because they're checking for !markers_.IsEmpty(), and ...
3 years, 7 months ago (2017-05-18 05:03:03 UTC) #8
yosin_UTC9
On 2017/05/18 at 05:03:03, rlanday wrote: > Hmm...don't we still need to remove the DCHECKs ...
3 years, 7 months ago (2017-05-18 05:33:20 UTC) #11
rlanday
Ok, sounds good. On 2017/05/18 at 05:03:03, rlanday wrote: > Hmm...don't we still need to ...
3 years, 7 months ago (2017-05-18 06:36:27 UTC) #12
yosin_UTC9
On 2017/05/18 at 06:36:27, rlanday wrote: > Ok, sounds good. > > > On 2017/05/18 ...
3 years, 7 months ago (2017-05-18 06:44:15 UTC) #13
kouhei (in TOK)
Discussed offline with yosin@. Feel free to ping me again if you have further problems.
3 years, 7 months ago (2017-05-18 06:50:12 UTC) #14
yosin_UTC9
Discussed offline with Oilpan team. For HashMap<WeakMember<T>, V>. GC remove entries if key is invalid. ...
3 years, 7 months ago (2017-05-18 07:03:58 UTC) #16
yosin_UTC9
On 2017/05/18 at 07:03:58, yosin_UTC9 wrote: > Discussed offline with Oilpan team. > > To ...
3 years, 7 months ago (2017-05-18 07:14:26 UTC) #17
rlanday
Ok, I've updated the CL to leave the DCHECKs in place and modify PossiblyHasMarkers().
3 years, 7 months ago (2017-05-18 18:09:05 UTC) #19
Xiaocheng
lgtm
3 years, 7 months ago (2017-05-18 18:18:08 UTC) #20
yosin_UTC9
lgtm
3 years, 7 months ago (2017-05-19 05:51:13 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2891843003/20001
3 years, 7 months ago (2017-05-19 05:51:54 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/449648)
3 years, 7 months ago (2017-05-19 09:46:14 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2891843003/20001
3 years, 7 months ago (2017-05-19 23:11:22 UTC) #27
commit-bot: I haz the power
3 years, 7 months ago (2017-05-20 01:17:37 UTC) #30
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/37c906bd514195222ab091fa98a4...

Powered by Google App Engine
This is Rietveld 408576698