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

Issue 1964013002: Handle overlapping CrossThreadPersistent<> releases. (Closed)

Created:
4 years, 7 months ago by sof
Modified:
4 years, 7 months ago
Reviewers:
oilpan-reviews, haraken
CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), blink-reviews, kinuko+watch, kouhei+heap_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Handle overlapping CrossThreadPersistent<> releases. When a CrossThreadPersistent<> is cleared, the associated PersistentNode is freed. In the case when multiple threads attempt to do such clearing at the same time, the freeing protocol ensured atomicity but failed to check if the PersistentNode had been freed already. This follows up on the freeing of PersistentNodes that r392263 added for CrossThreadPersistent<>s. R=haraken BUG= Committed: https://crrev.com/be260f5b91027b81be5ba04939ebef2dbd5c92ee Cr-Commit-Position: refs/heads/master@{#392689}

Patch Set 1 #

Patch Set 2 : drop unneeded PLATFORM_EXPORTs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -17 lines) Patch
M third_party/WebKit/Source/platform/heap/Handle.h View 2 chunks +13 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/PersistentNode.h View 1 1 chunk +13 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/heap/PersistentNode.cpp View 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 16 (8 generated)
sof
please take a look.
4 years, 7 months ago (2016-05-10 14:59:58 UTC) #3
haraken
LGTM, thanks for cleaning up persistent handles a lot!
4 years, 7 months ago (2016-05-10 15:21:05 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964013002/20001
4 years, 7 months ago (2016-05-10 15:50:23 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/210969)
4 years, 7 months ago (2016-05-10 15:58:32 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964013002/20001
4 years, 7 months ago (2016-05-10 18:38:50 UTC) #11
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 7 months ago (2016-05-10 19:48:45 UTC) #13
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/be260f5b91027b81be5ba04939ebef2dbd5c92ee Cr-Commit-Position: refs/heads/master@{#392689}
4 years, 7 months ago (2016-05-10 19:50:13 UTC) #15
sof
4 years, 7 months ago (2016-05-11 06:52:01 UTC) #16
Message was sent while issue was closed.
On 2016/05/10 15:21:05, haraken wrote:
> LGTM, thanks for cleaning up persistent handles a lot!

It looks they're in a good state, for now.

Powered by Google App Engine
This is Rietveld 408576698