|
|
Created:
4 years, 5 months ago by kapishnikov Modified:
4 years, 5 months ago 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. |
DescriptionSynchronize 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 #Messages
Total messages: 19 (4 generated)
kapishnikov@chromium.org changed reviewers: + eroman@chromium.org, justincohen@chromium.org, mef@chromium.org
PTAL. Thanks!
Is it possible to test this? Locking logic isn't that trivial. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:133: if (!lock_acquired_) { Having |lock_acquired_| flag and checking it outside of lock seems error-prone. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:200: CR_DEFINE_STATIC_LOCAL(base::Lock, lock, ()); Would it make sense to make lock a static member of SynchronizedRunLoopObserver class?
Thanks for the review. It will be tricky to test the synchronization logic since we would need to mock CFNetworkExecuteProxyAutoConfigurationURL. We will need to inject it into ProxyResolverMac. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:133: if (!lock_acquired_) { On 2016/06/28 16:59:51, mef wrote: > Having |lock_acquired_| flag and checking it outside of lock seems error-prone. A SynchronizedRunLoopObservers instance is always associated with a single run loop (thread). Therefore, |lock_acquired_| is always changed by a single (same) thread. |lock_acquired_| indicates whether the current thread possesses the lock or not. It's value is changed only after the lock was actually acquired or released. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:200: CR_DEFINE_STATIC_LOCAL(base::Lock, lock, ()); On 2016/06/28 16:59:52, mef wrote: > Would it make sense to make lock a static member of SynchronizedRunLoopObserver > class? The lock is used in this method to synchronize 'add' and 'remove' operations of run loop source. These operations are independent from the observer. It looks backward for the method to ask for the lock from the observer. On the other hand, SynchronizedRunLoopObserver is designed to accept any external lock.
https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... 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_ma... net/proxy/proxy_resolver_mac.cc:115: void SynchronizedRunLoopObserver::AddObserver(const CFStringRef mode) { Rename into AddToCurrentRunLoop(). https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:119: void SynchronizedRunLoopObserver::RemoveObserver(const CFStringRef mode) { Rename into RemoveFromCurrentRunLoop(). https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:200: CR_DEFINE_STATIC_LOCAL(base::Lock, lock, ()); Use base/lazy_instance.h instead, rename into 'cfnetwork_pac_runloop_lock'. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:247: std::unique_ptr<SynchronizedRunLoopObserver> observer( drop unique_ptr. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:260: lock.Acquire(); Consider AutoLock
Addressed the review comments & added ThreadChecker https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:85: SynchronizedRunLoopObserver(base::Lock& lock); On 2016/06/28 19:41:06, mef wrote: > add destructor with DCHECK(!lock_acquired_); Done. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:115: void SynchronizedRunLoopObserver::AddObserver(const CFStringRef mode) { On 2016/06/28 19:41:06, mef wrote: > Rename into AddToCurrentRunLoop(). Done. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:119: void SynchronizedRunLoopObserver::RemoveObserver(const CFStringRef mode) { On 2016/06/28 19:41:06, mef wrote: > Rename into RemoveFromCurrentRunLoop(). Done. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:200: CR_DEFINE_STATIC_LOCAL(base::Lock, lock, ()); On 2016/06/28 19:41:07, mef wrote: > Use base/lazy_instance.h instead, rename into 'cfnetwork_pac_runloop_lock'. Done & Done https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:247: std::unique_ptr<SynchronizedRunLoopObserver> observer( On 2016/06/28 19:41:07, mef wrote: > drop unique_ptr. Done. https://codereview.chromium.org/2094373002/diff/1/net/proxy/proxy_resolver_ma... net/proxy/proxy_resolver_mac.cc:260: lock.Acquire(); On 2016/06/28 19:41:06, mef wrote: > Consider AutoLock Done.
Eric, Justin, could you please take a look at the change?
lgtm mod nits. https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolve... File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolve... net/proxy/proxy_resolver_mac.cc:38: static base::LazyInstance<base::Lock>::Leaky cfnetwork_pac_runloop_lock_ = nit: trailing underscore is used only for member variables, global should start with g_. https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolve... net/proxy/proxy_resolver_mac.cc:118: base::ThreadChecker thread_checker_; Add DISALLOW_COPY_AND_ASSIGN(SynchronizedRunLoopObserver);
https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolve... File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolve... 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: > nit: trailing underscore is used only for member variables, global should start > with g_. Done. https://codereview.chromium.org/2094373002/diff/20001/net/proxy/proxy_resolve... net/proxy/proxy_resolver_mac.cc:118: base::ThreadChecker thread_checker_; On 2016/06/29 20:13:39, mef wrote: > Add DISALLOW_COPY_AND_ASSIGN(SynchronizedRunLoopObserver); Done.
still lgtm
LGTM as a quick fix, however I am not very familiar with the mac/ios side of this. Can you follow up with: (1) Tests. You can have a unit-test that creates a MultiThreadedProxyResolver + ProxyResolverMac with a relatively high thread count. I think the only other dependency is to have a reachable pac_url. For this can use the TestServer or even a publicly reachable URL. Not guaranteed to reproduce the bug of course, but at least gives some coverage. (2) I think long term ProxyResolverMac should be made async per https://bugs.chromium.org/p/chromium/issues/detail?id=166387#c95. Can you add a TODO to that effect somewhere in the file, or discuss with me on the bug thread why you think having the thread management be done external to ProxyResolverMac is preferable. https://codereview.chromium.org/2094373002/diff/40001/net/proxy/proxy_resolve... File net/proxy/proxy_resolver_mac.cc (right): https://codereview.chromium.org/2094373002/diff/40001/net/proxy/proxy_resolve... net/proxy/proxy_resolver_mac.cc:85: // A run loop observer that guarantees that no any two run loop sources "no any two" --> "no two"
Eric, thanks for the review. On 2016/06/30 19:02:02, eroman wrote: > LGTM as a quick fix, however I am not very familiar with the mac/ios side of > this. > > Can you follow up with: > > (1) Tests. You can have a unit-test that creates a MultiThreadedProxyResolver > + ProxyResolverMac with a relatively high thread count. I think the only other > dependency is to have a reachable pac_url. For this can use the TestServer or > even a publicly reachable URL. Not guaranteed to reproduce the bug of course, > but at least gives some coverage. I can address it in the follow up CL or even better if we make ProxyResolverMac async and get rid of MultiThreadedProxyResolver. > > (2) I think long term ProxyResolverMac should be made async per > https://bugs.chromium.org/p/chromium/issues/detail?id=166387#c95. Can you add a > TODO to that effect somewhere in the file, or discuss with me on the bug thread > why you think having the thread management be done external to ProxyResolverMac > is preferable. I very much agree with that. I added the TODO. > > https://codereview.chromium.org/2094373002/diff/40001/net/proxy/proxy_resolve... > File net/proxy/proxy_resolver_mac.cc (right): > > https://codereview.chromium.org/2094373002/diff/40001/net/proxy/proxy_resolve... > net/proxy/proxy_resolver_mac.cc:85: // A run loop observer that guarantees that > no any two run loop sources > "no any two" --> "no two" Done.
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/2094373002/#ps60001 (title: "Addressed Eric's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/19a49ad54e88393e509771ad0659dd6004849140 Cr-Commit-Position: refs/heads/master@{#403284} |