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

Issue 2808113003: [Sync] Post back to UI thread for USS GetAllNodes. (Closed)

Created:
3 years, 8 months ago by skym
Modified:
3 years, 8 months ago
Reviewers:
maxbogue
CC:
chromium-reviews, sync-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Sync] Post back to UI thread for USS GetAllNodes. When running GetAllNodes, USS types post to the model thread to access their processor/bridge. This means after they've gathered the nodes, they need to return to the UI thread to give the results back to the GetAllNodesRequestHelper. Unfortunately I removed a wrapping BindToCurrentThread call in https://codereview.chromium.org/2672493002/diff/80001/components/sync/driver/model_type_controller.cc that did the jump back to the UI thread. This meant that non-UI USS types (namely AUTOFILL) were updating GetAllNodesRequestHelper from a different thread, through very non-thread safe code. To fix simply added the BindToCurrentThread back. Also added a added a thread checker to GetAllNodesRequestHelper in hopes of stopping similar bugs from occurring again. BUG=686172 Review-Url: https://codereview.chromium.org/2808113003 Cr-Commit-Position: refs/heads/master@{#463320} Committed: https://chromium.googlesource.com/chromium/src/+/903ebf3235f4057351f7734c0e148e7938009dbd

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -3 lines) Patch
M components/browser_sync/profile_sync_service.cc View 2 chunks +5 lines, -1 line 0 comments Download
M components/sync/driver/model_type_controller.cc View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 17 (12 generated)
skym
PTAL
3 years, 8 months ago (2017-04-10 16:59:18 UTC) #6
maxbogue
lgtm
3 years, 8 months ago (2017-04-10 17:02:51 UTC) #7
maxbogue
lgtm
3 years, 8 months ago (2017-04-10 17:02:52 UTC) #8
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/2808113003/1
3 years, 8 months ago (2017-04-10 17:50:11 UTC) #14
commit-bot: I haz the power
3 years, 8 months ago (2017-04-10 18:00:30 UTC) #17
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/903ebf3235f4057351f7734c0e14...

Powered by Google App Engine
This is Rietveld 408576698