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

Issue 2094373002: Synchronize events of CFNetworkExecuteProxyAutoConfigurationURL (Closed)

Created:
4 years, 5 months ago by kapishnikov
Modified:
4 years, 5 months ago
Reviewers:
eroman, mef, justincohen
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Synchronize events of CFNetworkExecuteProxyAutoConfigurationURL CFNetworkExecuteProxyAutoConfigurationURL run loop sources are not thread safe. Therefore, the following source events should be synchronized: 1. Adding the source to the run loop. 2. Handling the result of proxy resolution. 3. Removing the source from the run loop. This change does not prevent the parallel resolution of proxies for multiple URLs but only prevents concurrent execution of the above mentioned event, which are supposed to be lightweight operations. BUG=166387 Committed: https://crrev.com/19a49ad54e88393e509771ad0659dd6004849140 Cr-Commit-Position: refs/heads/master@{#403284}

Patch Set 1 #

Total comments: 16

Patch Set 2 : Addressed Misha's comments #

Total comments: 4

Patch Set 3 : Addressed Misha's comments #

Total comments: 1

Patch Set 4 : Addressed Eric's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+144 lines, -3 lines) Patch
M net/proxy/proxy_resolver_mac.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M net/proxy/proxy_resolver_mac.cc View 1 2 3 4 chunks +142 lines, -3 lines 0 comments Download

Messages

Total messages: 19 (4 generated)
kapishnikov
PTAL. Thanks!
4 years, 5 months ago (2016-06-27 16:32:21 UTC) #2
mef
Is it possible to test this? Locking logic isn't that trivial. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_mac.cc File net/proxy/proxy_resolver_mac.cc (right): ...
4 years, 5 months ago (2016-06-28 16:59:52 UTC) #3
kapishnikov
Thanks for the review. It will be tricky to test the synchronization logic since we ...
4 years, 5 months ago (2016-06-28 18:51:20 UTC) #4
mef
https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_mac.cc File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_mac.cc#newcode85 net/proxy/proxy_resolver_mac.cc:85: SynchronizedRunLoopObserver(base::Lock& lock); add destructor with DCHECK(!lock_acquired_); https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_mac.cc#newcode115 net/proxy/proxy_resolver_mac.cc:115: void ...
4 years, 5 months ago (2016-06-28 19:41:07 UTC) #5
kapishnikov
Addressed the review comments & added ThreadChecker https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_mac.cc File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_mac.cc#newcode85 net/proxy/proxy_resolver_mac.cc:85: SynchronizedRunLoopObserver(base::Lock& lock); ...
4 years, 5 months ago (2016-06-28 21:31:05 UTC) #6
kapishnikov
Eric, Justin, could you please take a look at the change?
4 years, 5 months ago (2016-06-28 22:27:08 UTC) #7
mef
lgtm mod nits. https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolver_mac.cc File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolver_mac.cc#newcode38 net/proxy/proxy_resolver_mac.cc:38: static base::LazyInstance<base::Lock>::Leaky cfnetwork_pac_runloop_lock_ = nit: trailing ...
4 years, 5 months ago (2016-06-29 20:13:39 UTC) #8
kapishnikov
https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolver_mac.cc File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolver_mac.cc#newcode38 net/proxy/proxy_resolver_mac.cc:38: static base::LazyInstance<base::Lock>::Leaky cfnetwork_pac_runloop_lock_ = On 2016/06/29 20:13:39, mef wrote: ...
4 years, 5 months ago (2016-06-29 22:41:11 UTC) #9
mef
still lgtm
4 years, 5 months ago (2016-06-30 14:55:50 UTC) #10
eroman
LGTM as a quick fix, however I am not very familiar with the mac/ios side ...
4 years, 5 months ago (2016-06-30 19:02:02 UTC) #11
kapishnikov
Eric, thanks for the review. On 2016/06/30 19:02:02, eroman wrote: > LGTM as a quick ...
4 years, 5 months ago (2016-06-30 20:10:16 UTC) #12
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/2094373002/60001
4 years, 5 months ago (2016-06-30 20:11:24 UTC) #15
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 5 months ago (2016-06-30 21:10:36 UTC) #16
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-06-30 21:10:45 UTC) #17
commit-bot: I haz the power
4 years, 5 months ago (2016-06-30 21:12:26 UTC) #19
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/19a49ad54e88393e509771ad0659dd6004849140
Cr-Commit-Position: refs/heads/master@{#403284}

Powered by Google App Engine
This is Rietveld 408576698