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

Issue 1439153002: Revert of Change the update_client task runner behavior to continue on shutdown. (Closed)

Created:
5 years, 1 month ago by Sorin Jianu
Modified:
5 years, 1 month ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Change the update_client task runner behavior to continue on shutdown. (patchset #2 id:20001 of https://codereview.chromium.org/1415933011/ ) Reason for revert: This change is a suspect for causing crbug.com/475872 Original issue's description: > Change the update_client task runner behavior to continue on shutdown. > > Historically, we've had issues with running the BITS COM client on > threads and interfering with the browser shutdown. > > For reason not entirely understood, some out of process COM calls, > and most common the call to enumerate BITS jobs appear to hang and > consequently trigger the browser hang shutdown detector. > > At first, we had run this code on the FILE thread, then we had moved > it on blocking pool threads. However, the net effect is that the > hang moved as well. > > This change avoids blocking the shutdown by allowing the code to > run after the browser shutdown until the OS terminates the thread as > part of the process exit. > > While it is somehow difficult to reason about the correctness of > the update_client code, this change is reasonably safe to make > due to aspects of refcounting and containment of the update_client > such as not accessing browser global state and refcounting the > objects that are thread aware. > > BUG=552028 > > Committed: https://crrev.com/5081941b99d37952e1c60c9cb24257369d9c6416 > Cr-Commit-Position: refs/heads/master@{#359138} TBR=waffles@chromium.org,asargent@chromium.org,rockot@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=552028 Committed: https://crrev.com/f4106b804bd4a023c044de4a6b2c4f5e564b0954 Cr-Commit-Position: refs/heads/master@{#359441}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -87 lines) Patch
M chrome/browser/component_updater/chrome_component_updater_configurator.cc View 1 chunk +1 line, -5 lines 0 comments Download
M components/component_updater/component_updater_service.cc View 1 chunk +0 lines, -1 line 0 comments Download
M components/component_updater/component_updater_service_unittest.cc View 5 chunks +0 lines, -6 lines 0 comments Download
M components/update_client/task.h View 1 chunk +0 lines, -5 lines 0 comments Download
M components/update_client/task_update.h View 1 chunk +2 lines, -5 lines 0 comments Download
M components/update_client/task_update.cc View 2 chunks +4 lines, -12 lines 0 comments Download
M components/update_client/update_client.h View 2 chunks +0 lines, -8 lines 0 comments Download
M components/update_client/update_client.cc View 4 chunks +7 lines, -38 lines 0 comments Download
M components/update_client/update_client_internal.h View 3 chunks +8 lines, -6 lines 0 comments Download
M extensions/browser/updater/update_service_unittest.cc View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 5 (0 generated)
Sorin Jianu
Created Revert of Change the update_client task runner behavior to continue on shutdown.
5 years, 1 month ago (2015-11-12 23:52:38 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1439153002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1439153002/1
5 years, 1 month ago (2015-11-12 23:53:42 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 1 month ago (2015-11-12 23:55:13 UTC) #3
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/f4106b804bd4a023c044de4a6b2c4f5e564b0954 Cr-Commit-Position: refs/heads/master@{#359441}
5 years, 1 month ago (2015-11-12 23:56:10 UTC) #4
Sorin Jianu
5 years, 1 month ago (2015-11-13 18:17:45 UTC) #5
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1440393002/ by sorin@chromium.org.

The reason for reverting is: The original change has been ruled out as the cause
for https://code.google.com/p/chromium/issues/detail?id=475872

Therefore, this is the revert of the revert for
https://codereview.chromium.org/1415933011/

When this patch lands, the state of the code will be that we would have the
intended BITS threading changes in, but BITS itself will still be disabled due
to https://codereview.chromium.org/1441223002/

Another change to re-enable BITS will land shortly..

Powered by Google App Engine
This is Rietveld 408576698