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

Issue 667573007: Revert "Sync: Avoid deadlock in SyncBackendRegistrar / ModelSafeWorker on sync backend shutdown." (Closed)

Created:
6 years, 2 months ago by Devlin
Modified:
6 years, 2 months ago
Reviewers:
stanisc, Nicolas Zea
CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, pvalenzuela+watch_chromium.org, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org, maniscalco+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Revert "Sync: Avoid deadlock in SyncBackendRegistrar / ModelSafeWorker on sync backend shutdown." This reverts commit 8e84bfcc1c33fc0b63392d48ee8288d73f4a7675. Original CL: https://codereview.chromium.org/637413003 Reason for Revert: New test SyncBackendRegistrarShutdownTest.BlockingShutdown fails repeatedly on android and causes other tests to hang. Also fails on other bots. http://chromium-build-logs.appspot.com/gtest_query?gtest_query=SyncBackendRegistrarShutdownTest.BlockingShutdown Failures are timeouts, and don't have any useful logs or stack traces (sorry). Reason for hand-revert: Conflict with crrev.com/0cd9ff92b2dbc83fe8c3664b48041c7ced683c9c NOTRY=true TBR=stanisc@chromium.org TBR=zea@chromium.org -- Original CL Description -- Sync: Avoid deadlock in SyncBackendRegistrar / ModelSafeWorker on sync backend shutdown. This deadlock involves SyncBackendRegistrar running on the sync thread and at least two workers - one of them (worker A) running on UI thread and another (worker B) running on some other background thread. In this particular case it was a password worker running on Password model thread. The deadlock occurs when the SyncBackendRegistrar shutdown code running in the sync thread blocks on a waitable event set by worker B, at the same time the worker B's thread is blocked for whatever reason waiting to make a synchronous call into the UI thread, and at the same time the worker A is blocked on a lock owned by the sync thread while it attempts to unregister itself with SyncBackendRegistrar. I think the main issue here is that the sync thread might block on a worker. This is possible in only one situation - when that worker hasn't have a chance to run any tasks because its own thread was blocked doing something else. The fix removes this dependency. The only reason this blocking occurs is because ModelSafeWorker has a mechanism for subscribing / unsubscribing to the MessageLoop descruction which has to be done in the right thread context. In order to unsubscribe the worker needs to remember the message loop it runs on which is done in SetWorkingLoopToCurrent callback, which is called from a task posted on the worker's thread. There is also a waitable event which is signalled at that time. The even is used to block ModelSafeWorker::UnregisterForLoopDestruction from delegating the call on an uninitialized message loop which is possible only when SetWorkingLoopToCurrent has never been called. There is no need to block in UnregisterForLoopDestruction. If the registration for even loop destruction hasn't been been done yet (because the thread is blocked) then there really no reason to wait for that and then immediately unsubscribe. We can skip both subscription to the even loop notification altogether and let the registrar know that it is OK to remove the worker. That's essentially what the fix does. I added a test that verifies that SyncBackendRegistrar::Shutdown doesn't block on a blocked model type thread anymore. BUG=423078 Committed: https://crrev.com/8e84bfcc1c33fc0b63392d48ee8288d73f4a7675 Cr-Commit-Position: refs/heads/master@{#300350} Committed: https://crrev.com/141df6d109aad65adba7a58f6522af415a586b7c Cr-Commit-Position: refs/heads/master@{#300498}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -126 lines) Patch
M chrome/browser/sync/glue/browser_thread_model_worker.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sync/glue/history_model_worker.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sync/glue/password_model_worker.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sync/glue/sync_backend_registrar_unittest.cc View 1 chunk +0 lines, -85 lines 0 comments Download
M chrome/browser/sync/glue/ui_model_worker.cc View 1 chunk +1 line, -0 lines 0 comments Download
M sync/internal_api/public/engine/model_safe_worker.h View 1 chunk +1 line, -7 lines 0 comments Download
M sync/internal_api/public/engine/model_safe_worker.cc View 2 chunks +18 lines, -34 lines 0 comments Download
M sync/internal_api/public/engine/passive_model_worker.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 9 (4 generated)
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667573007/1
6 years, 2 months ago (2014-10-21 16:31:00 UTC) #3
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
6 years, 2 months ago (2014-10-21 16:31:01 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667573007/1
6 years, 2 months ago (2014-10-21 16:32:52 UTC) #7
commit-bot: I haz the power
Committed patchset #1 (id:1)
6 years, 2 months ago (2014-10-21 16:35:04 UTC) #8
commit-bot: I haz the power
6 years, 2 months ago (2014-10-21 16:36:52 UTC) #9
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/141df6d109aad65adba7a58f6522af415a586b7c
Cr-Commit-Position: refs/heads/master@{#300498}

Powered by Google App Engine
This is Rietveld 408576698