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

Issue 2886913003: Loosen thread-safety checks and update documentation on RunLoop. (Closed)

Created:
3 years, 7 months ago by gab
Modified:
3 years, 7 months ago
Reviewers:
danakj
CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Loosen thread-safety checks and update documentation on RunLoop. RunLoop::Delegate now being the thread-affine part. RunLoop itself is now merely thread-unsafe (->sequence checks). And make it safe to access a RunLoop's state from another sequence while it's being Run() as that access is technically "sequenced" (any access will rebind the SequenceChecker to that sequence for the remainder of the run and should still catch undesired concurrent accesses). The lack of detach during Run() might also be the cause of issue 715235 (will try to remove TODOs after this lands). BUG=722537, 715235 Review-Url: https://codereview.chromium.org/2886913003 Cr-Commit-Position: refs/heads/master@{#472835} Committed: https://chromium.googlesource.com/chromium/src/+/980a5271bd212db2ad7a050c3d49628aed42deb1

Patch Set 1 #

Patch Set 2 : rebase dependency #

Patch Set 3 : update dependency #

Patch Set 4 : proper dependency #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -16 lines) Patch
M base/run_loop.h View 1 5 chunks +11 lines, -7 lines 0 comments Download
M base/run_loop.cc View 1 5 chunks +20 lines, -9 lines 2 comments Download

Depends on Patchset:

Messages

Total messages: 37 (31 generated)
gab
Dana PTaL, thanks!
3 years, 7 months ago (2017-05-17 03:08:47 UTC) #9
gab
On 2017/05/17 03:08:47, gab wrote: > Dana PTaL, thanks! friendly ping :)
3 years, 7 months ago (2017-05-18 13:48:51 UTC) #30
danakj
LGTM https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc File base/run_loop.cc (right): https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc#newcode96 base/run_loop.cc:96: // Rebind this RunLoop to the current thread ...
3 years, 7 months ago (2017-05-18 15:25:27 UTC) #31
gab
https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc File base/run_loop.cc (right): https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc#newcode96 base/run_loop.cc:96: // Rebind this RunLoop to the current thread after ...
3 years, 7 months ago (2017-05-18 16:14:03 UTC) #32
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/2886913003/100001
3 years, 7 months ago (2017-05-18 16:14:48 UTC) #34
commit-bot: I haz the power
3 years, 7 months ago (2017-05-18 16:20:36 UTC) #37
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/980a5271bd212db2ad7a050c3d49...

Powered by Google App Engine
This is Rietveld 408576698