Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2328453003:
Makes use of EngineGeolocationFeature weak_ptr threadsafe. (Closed)
|
Created:
4 years, 3 months ago by CJ Modified:
3 years, 11 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMakes use of EngineGeolocationFeature weak_ptr threadsafe.
There was an issue where a weak_ptr to EngineGeolocationFeature
was being invalidated while another thread was accessing it. This
CL corrects this issue.
BUG=644483
Committed: https://crrev.com/4b44beefc9298186e5e1e27d0c2f90244bcbfef6
Cr-Commit-Position: refs/heads/master@{#424029}
Patch Set 1 #Patch Set 2 : Get rid of ui/gl/BUILD.gn change #
Total comments: 10
Patch Set 3 : Addresses kmarshall's #3 comments. #
Total comments: 16
Patch Set 4 : Addresses Wez's #7 comments. #Patch Set 5 : Addresses Wez's #9 comments. #
Total comments: 4
Patch Set 6 : Adds comments, addresses Wez's #13 comments. #
Total comments: 1
Patch Set 7 : Addresses Wez's #16, kmarshall's #17 comments. #Patch Set 8 : Merge branch 'master' into threadmess #
Total comments: 6
Patch Set 9 : Addresses Wez's #23 comments. #
Messages
Total messages: 30 (7 generated)
lethalantidote@chromium.org changed reviewers: + khushalsagar@chromium.org, kmarshall@chromium.org, wez@chromium.org
https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:69: delegate_task_runner_->PostTask( You can actually bind a callback which manages its own threadhopping! base::Bind(&base::TaskRunner::PostTask base::ThreadTaskRunnerHandle::Get(), FROM_HERE, callback); Now you don't need to pass in a task runner to the delegate. Shiny. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:16: // Location provider for Blimp using the device's provider over the network. Add good comments about threading requirements here - about BlimpLocationProvider and Delegate. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:19: // A delegate that implements a subset of LocationProvider's functions. Also document that all methods will be executed on |delegate_task_runner_|. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:30: feature_task_runner_ = base::ThreadTaskRunnerHandle::Get(); Do we need to store this as a field? Can we just get the handle in OverrideSystemLocationProvider? https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:161: // Set |callback_task_runner_| to run on the current thread. This doesn't make sense?
https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:69: delegate_task_runner_->PostTask( On 2016/09/09 17:07:04, Kevin M wrote: > You can actually bind a callback which manages its own threadhopping! > > base::Bind(&base::TaskRunner::PostTask base::ThreadTaskRunnerHandle::Get(), > FROM_HERE, callback); > > Now you don't need to pass in a task runner to the delegate. Shiny. Done. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:16: // Location provider for Blimp using the device's provider over the network. On 2016/09/09 17:07:04, Kevin M wrote: > Add good comments about threading requirements here - about > BlimpLocationProvider and Delegate. Done. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:19: // A delegate that implements a subset of LocationProvider's functions. On 2016/09/09 17:07:04, Kevin M wrote: > Also document that all methods will be executed on |delegate_task_runner_|. Done. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:30: feature_task_runner_ = base::ThreadTaskRunnerHandle::Get(); On 2016/09/09 17:07:05, Kevin M wrote: > Do we need to store this as a field? Can we just get the handle in > OverrideSystemLocationProvider? I don't believe so, since OverrideSystemLocationProvider is called on the Geolocation Thread, not the Blimp thread. We want the Blimp thread in this case. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:161: // Set |callback_task_runner_| to run on the current thread. On 2016/09/09 17:07:05, Kevin M wrote: > This doesn't make sense? Done.
https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:69: delegate_task_runner_->PostTask( On 2016/09/09 17:07:04, Kevin M wrote: > You can actually bind a callback which manages its own threadhopping! > > base::Bind(&base::TaskRunner::PostTask base::ThreadTaskRunnerHandle::Get(), > FROM_HERE, callback); > > Now you don't need to pass in a task runner to the delegate. Shiny. Done. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:16: // Location provider for Blimp using the device's provider over the network. On 2016/09/09 17:07:04, Kevin M wrote: > Add good comments about threading requirements here - about > BlimpLocationProvider and Delegate. Done. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:19: // A delegate that implements a subset of LocationProvider's functions. On 2016/09/09 17:07:04, Kevin M wrote: > Also document that all methods will be executed on |delegate_task_runner_|. Done. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:30: feature_task_runner_ = base::ThreadTaskRunnerHandle::Get(); On 2016/09/09 17:07:05, Kevin M wrote: > Do we need to store this as a field? Can we just get the handle in > OverrideSystemLocationProvider? I don't believe so, since OverrideSystemLocationProvider is called on the Geolocation Thread, not the Blimp thread. We want the Blimp thread in this case. https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/engine_geolocation_feature.cc:161: // Set |callback_task_runner_| to run on the current thread. On 2016/09/09 17:07:05, Kevin M wrote: > This doesn't make sense? Done.
On 2016/09/09 19:52:42, CJ wrote: > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): > > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > blimp/engine/feature/geolocation/blimp_location_provider.cc:69: > delegate_task_runner_->PostTask( > On 2016/09/09 17:07:04, Kevin M wrote: > > You can actually bind a callback which manages its own threadhopping! > > > > base::Bind(&base::TaskRunner::PostTask base::ThreadTaskRunnerHandle::Get(), > > FROM_HERE, callback); > > > > Now you don't need to pass in a task runner to the delegate. Shiny. > > Done. > > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > File blimp/engine/feature/geolocation/blimp_location_provider.h (right): > > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > blimp/engine/feature/geolocation/blimp_location_provider.h:16: // Location > provider for Blimp using the device's provider over the network. > On 2016/09/09 17:07:04, Kevin M wrote: > > Add good comments about threading requirements here - about > > BlimpLocationProvider and Delegate. > > Done. > > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > blimp/engine/feature/geolocation/blimp_location_provider.h:19: // A delegate > that implements a subset of LocationProvider's functions. > On 2016/09/09 17:07:04, Kevin M wrote: > > Also document that all methods will be executed on |delegate_task_runner_|. > > Done. > > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right): > > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > blimp/engine/feature/geolocation/engine_geolocation_feature.cc:30: > feature_task_runner_ = base::ThreadTaskRunnerHandle::Get(); > On 2016/09/09 17:07:05, Kevin M wrote: > > Do we need to store this as a field? Can we just get the handle in > > OverrideSystemLocationProvider? > > I don't believe so, since OverrideSystemLocationProvider is called on the > Geolocation Thread, not the Blimp thread. We want the Blimp thread in this case. > > https://codereview.chromium.org/2328453003/diff/20001/blimp/engine/feature/ge... > blimp/engine/feature/geolocation/engine_geolocation_feature.cc:161: // Set > |callback_task_runner_| to run on the current thread. > On 2016/09/09 17:07:05, Kevin M wrote: > > This doesn't make sense? > > Done. Ping.
https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:18: void InvokeGeopositionCallback( Ah, I think we're going to need bind this to the lifetime of the BlimpLocationProvider, after all. If we don't then we can have: 1. [Thread A] Delegate calls InvokeGeopositionCallback. 2. [Thread B] Object to which |callback| is bound is deleted. 3. [Thread B] PostTask(callback) executes. At #3 we explode! This is a question of the expected semantics of SetUpdateCallback() - does the caller have a reasonable expectation that their callback won't be invoked after they have deleted the BlimpLocationProvider? Or what if they call SetUpdateCallback(null) - is it expected then that the old callback may still be invoked (which would be the case with your current patch)? https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:22: // this calls task_runner->PostTas ? https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:47: GeolocationSetInterestLevelMessage::HIGH_ACCURACY)); nit: You can merge this into a single PostTask with (high_accuracy ? HIGH_ACCURACY : LOW_ACCURACY) as the parameter :) https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:18: // posted using |delegate_task_runner_|. nit: This is part of the Delegate contract, so you don't need to stipulate it here - just mention it in the Delegate comment, or a comment on the constructor. https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:39: scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner); nit: Strictly a SequencedTaskRunner is sufficient here, since that's the requirement WeakPtr itself has https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (left): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:116: TEST_F(BlimpLocationProviderTest, OnPermissionGrantedHandlesNullDelegate) { We still want these tests, don't we, even if we are using PostTask internally?
https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:18: void InvokeGeopositionCallback( On 2016/09/13 20:19:07, Wez wrote: > Ah, I think we're going to need bind this to the lifetime of the > BlimpLocationProvider, after all. If we don't then we can have: > > 1. [Thread A] Delegate calls InvokeGeopositionCallback. > 2. [Thread B] Object to which |callback| is bound is deleted. > 3. [Thread B] PostTask(callback) executes. > > At #3 we explode! This is a question of the expected semantics of > SetUpdateCallback() - does the caller have a reasonable expectation that their > callback won't be invoked after they have deleted the BlimpLocationProvider? Or > what if they call SetUpdateCallback(null) - is it expected then that the old > callback may still be invoked (which would be the case with your current patch)? |callback| is bound to |this|, the BlimpLocationProvider. I'm not following you. https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:22: // this calls task_runner->PostTas On 2016/09/13 20:19:07, Wez wrote: > ? Done. https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:47: GeolocationSetInterestLevelMessage::HIGH_ACCURACY)); On 2016/09/13 20:19:07, Wez wrote: > nit: You can merge this into a single PostTask with (high_accuracy ? > HIGH_ACCURACY : LOW_ACCURACY) as the parameter :) Done. https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:18: // posted using |delegate_task_runner_|. On 2016/09/13 20:19:07, Wez wrote: > nit: This is part of the Delegate contract, so you don't need to stipulate it > here - just mention it in the Delegate comment, or a comment on the constructor. Done. https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:39: scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner); On 2016/09/13 20:19:07, Wez wrote: > nit: Strictly a SequencedTaskRunner is sufficient here, since that's the > requirement WeakPtr itself has Are you saying remove the scoped_refptr part? https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (left): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:116: TEST_F(BlimpLocationProviderTest, OnPermissionGrantedHandlesNullDelegate) { On 2016/09/13 20:19:07, Wez wrote: > We still want these tests, don't we, even if we are using PostTask internally? Done.
https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:18: void InvokeGeopositionCallback( On 2016/09/13 23:34:02, CJ wrote: > On 2016/09/13 20:19:07, Wez wrote: > > Ah, I think we're going to need bind this to the lifetime of the > > BlimpLocationProvider, after all. If we don't then we can have: > > > > 1. [Thread A] Delegate calls InvokeGeopositionCallback. > > 2. [Thread B] Object to which |callback| is bound is deleted. > > 3. [Thread B] PostTask(callback) executes. > > > > At #3 we explode! This is a question of the expected semantics of > > SetUpdateCallback() - does the caller have a reasonable expectation that their > > callback won't be invoked after they have deleted the BlimpLocationProvider? > Or > > what if they call SetUpdateCallback(null) - is it expected then that the old > > callback may still be invoked (which would be the case with your current > patch)? > > |callback| is bound to |this|, the BlimpLocationProvider. I'm not following you. Right, but Bind(callback, this) is just used to supply the LocationProvider to the LocationUpdateCallback, to satisfy that API contract. There is a separate issue of whether the calling code expects that if it does: location_provider->SetUpdateCallback(callback); delete location_provider; then is it still possible that |callback| is invoked *after* the caller deleted the LocationProvider? With this current approach, using a static function to bounce across threads, that is possible. So do you instead need to bind the PostTask of the callback to the lifetime of the LocationProvider in some way? https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:39: scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner); On 2016/09/13 23:34:02, CJ wrote: > On 2016/09/13 20:19:07, Wez wrote: > > nit: Strictly a SequencedTaskRunner is sufficient here, since that's the > > requirement WeakPtr itself has > > Are you saying remove the scoped_refptr part? So, I'm saying you should accept a scoped_refptr<SequencedTaskRunner> rather than <SingleThreadTaskRunner>, since strictly you only need the sequenced property, and don't care if it's a dedicated thread.
https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:18: void InvokeGeopositionCallback( On 2016/09/16 00:40:16, Wez wrote: > On 2016/09/13 23:34:02, CJ wrote: > > On 2016/09/13 20:19:07, Wez wrote: > > > Ah, I think we're going to need bind this to the lifetime of the > > > BlimpLocationProvider, after all. If we don't then we can have: > > > > > > 1. [Thread A] Delegate calls InvokeGeopositionCallback. > > > 2. [Thread B] Object to which |callback| is bound is deleted. > > > 3. [Thread B] PostTask(callback) executes. > > > > > > At #3 we explode! This is a question of the expected semantics of > > > SetUpdateCallback() - does the caller have a reasonable expectation that > their > > > callback won't be invoked after they have deleted the BlimpLocationProvider? > > > Or > > > what if they call SetUpdateCallback(null) - is it expected then that the old > > > callback may still be invoked (which would be the case with your current > > patch)? > > > > |callback| is bound to |this|, the BlimpLocationProvider. I'm not following > you. > > Right, but Bind(callback, this) is just used to supply the LocationProvider to > the LocationUpdateCallback, to satisfy that API contract. > > There is a separate issue of whether the calling code expects that if it does: > > location_provider->SetUpdateCallback(callback); > delete location_provider; > > then is it still possible that |callback| is invoked *after* the caller deleted > the LocationProvider? With this current approach, using a static function to > bounce across threads, that is possible. > > So do you instead need to bind the PostTask of the callback to the lifetime of > the LocationProvider in some way? Done. https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/40001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.h:39: scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner); On 2016/09/16 00:40:16, Wez wrote: > On 2016/09/13 23:34:02, CJ wrote: > > On 2016/09/13 20:19:07, Wez wrote: > > > nit: Strictly a SequencedTaskRunner is sufficient here, since that's the > > > requirement WeakPtr itself has > > > > Are you saying remove the scoped_refptr part? > > So, I'm saying you should accept a scoped_refptr<SequencedTaskRunner> rather > than <SingleThreadTaskRunner>, since strictly you only need the sequenced > property, and don't care if it's a dedicated thread. Done.
https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:23: if (provider) Isn't InvokeGeopositionCallback called from the delegate_task_runner_, though, hence why the PostTask() is here? It's at the point when the task reaches the geolocation thread, where the caller may have deleted the BlimpLocationProvider in the meantime, that we need to check this.
https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:23: if (provider) On 2016/09/17 01:47:42, Wez wrote: > Isn't InvokeGeopositionCallback called from the delegate_task_runner_, though, > hence why the PostTask() is here? > > It's at the point when the task reaches the geolocation thread, where the caller > may have deleted the BlimpLocationProvider in the meantime, that we need to > check this. InvokeGeopositionCallback is called on the geolocation thread. I'm not sure I follow you.
https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:23: if (provider) On 2016/09/23 18:03:58, CJ wrote: > On 2016/09/17 01:47:42, Wez wrote: > > Isn't InvokeGeopositionCallback called from the delegate_task_runner_, though, > > hence why the PostTask() is here? > > > > It's at the point when the task reaches the geolocation thread, where the > caller > > may have deleted the BlimpLocationProvider in the meantime, that we need to > > check this. > > InvokeGeopositionCallback is called on the geolocation thread. I'm not sure I > follow you. Are you sure? InvokeGepositionCallback is used to create the callback passed to the BlimpLocationProvider::Delegate, which runs on the delegate task runner, so presumably will call the calback there. That callback then needs to jump back to the geolocation thread, via PostTask, to run the LocationProviderUpdateCallback - but it needs to check that the BlimpLocationProvider still exists before it runs it.
On 2016/09/23 22:28:36, Wez wrote: > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): > > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > blimp/engine/feature/geolocation/blimp_location_provider.cc:23: if (provider) > On 2016/09/23 18:03:58, CJ wrote: > > On 2016/09/17 01:47:42, Wez wrote: > > > Isn't InvokeGeopositionCallback called from the delegate_task_runner_, > though, > > > hence why the PostTask() is here? > > > > > > It's at the point when the task reaches the geolocation thread, where the > > caller > > > may have deleted the BlimpLocationProvider in the meantime, that we need to > > > check this. > > > > InvokeGeopositionCallback is called on the geolocation thread. I'm not sure I > > follow you. > > Are you sure? InvokeGepositionCallback is used to create the callback passed to > the BlimpLocationProvider::Delegate, which runs on the delegate task runner, so > presumably will call the calback there. That callback then needs to jump back to > the geolocation thread, via PostTask, to run the LocationProviderUpdateCallback > - but it needs to check that the BlimpLocationProvider still exists before it > runs it. I am pretty sure. Checked with Kevin to make sure I'm not crazy, and he thinks it's ok but we both could be wrong. How I see it happening is the following: SetUpdateCallback gets called on the delegate (geolocation) thread and stores the wrapped callback in EngineGeolocationFeature. NotifyCallback gets called on the geolocation thread, which runs (not posts) the wrapped callback, which causes InvokeGeopositionCallback to be ran on the geolocation thread. InvokeGeoposition checks to see if we still have a BlimpLocationProvider, and if so, runs the callback on the main blimp thread using the task runner that was passed to it when it was binded back at BlimpLocationProvider's SetUpdateCallback. I've added comments explaining this in the code. Please let me know if something is still off or more comments are needed.
On 2016/09/28 00:57:29, CJ wrote: > On 2016/09/23 22:28:36, Wez wrote: > > > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > > File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): > > > > > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > > blimp/engine/feature/geolocation/blimp_location_provider.cc:23: if (provider) > > On 2016/09/23 18:03:58, CJ wrote: > > > On 2016/09/17 01:47:42, Wez wrote: > > > > Isn't InvokeGeopositionCallback called from the delegate_task_runner_, > > though, > > > > hence why the PostTask() is here? > > > > > > > > It's at the point when the task reaches the geolocation thread, where the > > > caller > > > > may have deleted the BlimpLocationProvider in the meantime, that we need > to > > > > check this. > > > > > > InvokeGeopositionCallback is called on the geolocation thread. I'm not sure > I > > > follow you. > > > > Are you sure? InvokeGepositionCallback is used to create the callback passed > to > > the BlimpLocationProvider::Delegate, which runs on the delegate task runner, > so > > presumably will call the calback there. That callback then needs to jump back > to > > the geolocation thread, via PostTask, to run the > LocationProviderUpdateCallback > > - but it needs to check that the BlimpLocationProvider still exists before it > > runs it. > > I am pretty sure. Checked with Kevin to make sure I'm not crazy, and he thinks > it's ok but we both could be wrong. Thanks for writing out the sequence. There are two issues, I think: 1. Confusion over which thread each step runs on. 2. You're checking a pre-condition on a task before posting it, rather than before running it. > How I see it happening is the following: > SetUpdateCallback gets called on the delegate (geolocation) thread and stores > the wrapped callback in EngineGeolocationFeature. I don't think this is correct. SetUpdateCallback() is a LocationProvider API, called by the Content Layer, from the Geolocation thread. The Delegate thread is not the Geolocation thread. (In practice it's the same thread as the Content Layer's UI thread, but that's not important here) > NotifyCallback gets called on the geolocation thread, which runs (not posts) the > wrapped callback, which causes InvokeGeopositionCallback to be ran on the > geolocation thread. NotifyCallback is an implementation detail of the EngineGeolocationFeature, which is the Delegate. So it is called on the Delegate thread (in practice the UI thread) and Run()s InvokeGeolocationFeature on that thread. > InvokeGeoposition checks to see if we still have a BlimpLocationProvider, and if > so, runs the callback on the main blimp thread using the task runner that was > passed to it when it was binded back at BlimpLocationProvider's > SetUpdateCallback. This means you are checking (i.e. dereferencing) the WeakPtr<BlimpLocationProvider> on the Delegate thread, before posting the callback to be run on the Geolocation thread. Since the LocationProvider will be destroyed on the Geolocation thread, this creates a race-condition, which WeakPtr will DCHECK for. However, even if all this were happening on the same thread, the current code is still broken. Consider the sequence: 1. NotifyCallback -> InvokeGeolocationCallback -> PostTask(LocationUpdateCallback). 2. delete BlimpLocationProvider. 3. LocationUpdateCallback.Run() At #3 we are running the LocationUpdateCallback, because it was PostTask'd, even though BlimpLocationProvider is gone. In general you can't check a precondition and then post a task, because there is no guarantee that the precondition still holds when the task is actually run - some other task may have been run in-between (in this example #2, deleting the provider). You need to check the precondition immediately before Run()ing the callback/task.
On 2016/09/28 11:56:03, Wez wrote: > On 2016/09/28 00:57:29, CJ wrote: > > On 2016/09/23 22:28:36, Wez wrote: > > > > > > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > > > File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): > > > > > > > > > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > > > blimp/engine/feature/geolocation/blimp_location_provider.cc:23: if > (provider) > > > On 2016/09/23 18:03:58, CJ wrote: > > > > On 2016/09/17 01:47:42, Wez wrote: > > > > > Isn't InvokeGeopositionCallback called from the delegate_task_runner_, > > > though, > > > > > hence why the PostTask() is here? > > > > > > > > > > It's at the point when the task reaches the geolocation thread, where > the > > > > caller > > > > > may have deleted the BlimpLocationProvider in the meantime, that we need > > to > > > > > check this. > > > > > > > > InvokeGeopositionCallback is called on the geolocation thread. I'm not > sure > > I > > > > follow you. > > > > > > Are you sure? InvokeGepositionCallback is used to create the callback passed > > to > > > the BlimpLocationProvider::Delegate, which runs on the delegate task runner, > > so > > > presumably will call the calback there. That callback then needs to jump > back > > to > > > the geolocation thread, via PostTask, to run the > > LocationProviderUpdateCallback > > > - but it needs to check that the BlimpLocationProvider still exists before > it > > > runs it. > > > > I am pretty sure. Checked with Kevin to make sure I'm not crazy, and he thinks > > it's ok but we both could be wrong. > > Thanks for writing out the sequence. There are two issues, I think: > 1. Confusion over which thread each step runs on. > 2. You're checking a pre-condition on a task before posting it, rather than > before running it. > > > How I see it happening is the following: > > > SetUpdateCallback gets called on the delegate (geolocation) thread and stores > > the wrapped callback in EngineGeolocationFeature. > > I don't think this is correct. SetUpdateCallback() is a LocationProvider API, > called by the Content Layer, from the Geolocation thread. The Delegate thread > is not the Geolocation thread. (In practice it's the same thread as the Content > Layer's UI thread, but that's not important here) > > > NotifyCallback gets called on the geolocation thread, which runs (not posts) > the > > wrapped callback, which causes InvokeGeopositionCallback to be ran on the > > geolocation thread. > > NotifyCallback is an implementation detail of the EngineGeolocationFeature, > which is the Delegate. So it is called on the Delegate thread (in practice the > UI thread) and Run()s InvokeGeolocationFeature on that thread. > > > InvokeGeoposition checks to see if we still have a BlimpLocationProvider, and > if > > so, runs the callback on the main blimp thread using the task runner that was > > passed to it when it was binded back at BlimpLocationProvider's > > SetUpdateCallback. > > This means you are checking (i.e. dereferencing) the > WeakPtr<BlimpLocationProvider> on the Delegate thread, before posting the > callback to be run on the Geolocation thread. Since the LocationProvider will be > destroyed on the Geolocation thread, this creates a race-condition, which > WeakPtr will DCHECK for. > > However, even if all this were happening on the same thread, the current code is > still broken. Consider the sequence: > > 1. NotifyCallback -> InvokeGeolocationCallback -> > PostTask(LocationUpdateCallback). > 2. delete BlimpLocationProvider. > 3. LocationUpdateCallback.Run() > > At #3 we are running the LocationUpdateCallback, because it was PostTask'd, even > though BlimpLocationProvider is gone. In general you can't check a precondition > and then post a task, because there is no guarantee that the precondition still > holds when the task is actually run - some other task may have been run > in-between (in this example #2, deleting the provider). You need to check the > precondition immediately before Run()ing the callback/task. You should be able to add a test which triggers NotifyCallback but then deletes the BlimpLocationProvider *before* calling RunLoop().RunUntilIdle(), and see that the LocationUpdateCallback will get called even though the provider is gone (and that test will fail with or without threading :)
https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... blimp/engine/feature/geolocation/blimp_location_provider.cc:25: base::Bind(callback, provider.get(), geoposition)); Weak pointers must be dereferenced and deleted on the same thread. In this case, we are dereferencing it on the delegate thread, but we are deleting the factory elsewhere on the main thread. That'll definitely blow up. We need to thread here hop BEFORE we poke at |provider|. https://codereview.chromium.org/2328453003/diff/100001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): https://codereview.chromium.org/2328453003/diff/100001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.cc:19: // of the geolocation update back to the main blimp thread. main blimp thread => blimp_task_runner.
On 2016/09/28 11:57:50, Wez wrote: > On 2016/09/28 11:56:03, Wez wrote: > > On 2016/09/28 00:57:29, CJ wrote: > > > On 2016/09/23 22:28:36, Wez wrote: > > > > > > > > > > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > > > > File blimp/engine/feature/geolocation/blimp_location_provider.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2328453003/diff/80001/blimp/engine/feature/ge... > > > > blimp/engine/feature/geolocation/blimp_location_provider.cc:23: if > > (provider) > > > > On 2016/09/23 18:03:58, CJ wrote: > > > > > On 2016/09/17 01:47:42, Wez wrote: > > > > > > Isn't InvokeGeopositionCallback called from the delegate_task_runner_, > > > > though, > > > > > > hence why the PostTask() is here? > > > > > > > > > > > > It's at the point when the task reaches the geolocation thread, where > > the > > > > > caller > > > > > > may have deleted the BlimpLocationProvider in the meantime, that we > need > > > to > > > > > > check this. > > > > > > > > > > InvokeGeopositionCallback is called on the geolocation thread. I'm not > > sure > > > I > > > > > follow you. > > > > > > > > Are you sure? InvokeGepositionCallback is used to create the callback > passed > > > to > > > > the BlimpLocationProvider::Delegate, which runs on the delegate task > runner, > > > so > > > > presumably will call the calback there. That callback then needs to jump > > back > > > to > > > > the geolocation thread, via PostTask, to run the > > > LocationProviderUpdateCallback > > > > - but it needs to check that the BlimpLocationProvider still exists before > > it > > > > runs it. > > > > > > I am pretty sure. Checked with Kevin to make sure I'm not crazy, and he > thinks > > > it's ok but we both could be wrong. > > > > Thanks for writing out the sequence. There are two issues, I think: > > 1. Confusion over which thread each step runs on. > > 2. You're checking a pre-condition on a task before posting it, rather than > > before running it. > > > > > How I see it happening is the following: > > > > > SetUpdateCallback gets called on the delegate (geolocation) thread and > stores > > > the wrapped callback in EngineGeolocationFeature. > > > > I don't think this is correct. SetUpdateCallback() is a LocationProvider API, > > called by the Content Layer, from the Geolocation thread. The Delegate thread > > is not the Geolocation thread. (In practice it's the same thread as the > Content > > Layer's UI thread, but that's not important here) > > > > > NotifyCallback gets called on the geolocation thread, which runs (not posts) > > the > > > wrapped callback, which causes InvokeGeopositionCallback to be ran on the > > > geolocation thread. > > > > NotifyCallback is an implementation detail of the EngineGeolocationFeature, > > which is the Delegate. So it is called on the Delegate thread (in practice the > > UI thread) and Run()s InvokeGeolocationFeature on that thread. > > > > > InvokeGeoposition checks to see if we still have a BlimpLocationProvider, > and > > if > > > so, runs the callback on the main blimp thread using the task runner that > was > > > passed to it when it was binded back at BlimpLocationProvider's > > > SetUpdateCallback. > > > > This means you are checking (i.e. dereferencing) the > > WeakPtr<BlimpLocationProvider> on the Delegate thread, before posting the > > callback to be run on the Geolocation thread. Since the LocationProvider will > be > > destroyed on the Geolocation thread, this creates a race-condition, which > > WeakPtr will DCHECK for. > > > > However, even if all this were happening on the same thread, the current code > is > > still broken. Consider the sequence: > > > > 1. NotifyCallback -> InvokeGeolocationCallback -> > > PostTask(LocationUpdateCallback). > > 2. delete BlimpLocationProvider. > > 3. LocationUpdateCallback.Run() > > > > At #3 we are running the LocationUpdateCallback, because it was PostTask'd, > even > > though BlimpLocationProvider is gone. In general you can't check a > precondition > > and then post a task, because there is no guarantee that the precondition > still > > holds when the task is actually run - some other task may have been run > > in-between (in this example #2, deleting the provider). You need to check the > > precondition immediately before Run()ing the callback/task. > > You should be able to add a test which triggers NotifyCallback but then deletes > the BlimpLocationProvider *before* calling RunLoop().RunUntilIdle(), and see > that the LocationUpdateCallback will get called even though the provider is gone > (and that test will fail with or without threading :) Done.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:47: void OnLocationUpdate(const device::Geoposition& geoposition); This should be private, since it's an internal implementation detail of BlimpLocationProvider. Since you need to Bind() to it from the InvokeGeolocationCallback() function you may need to move that function to be a static member, so it has visibility into the private bit of the class. https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:52: bool callback_called_ = false; nit: Suggest on_location_update_called_ for clarity. https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:171: EXPECT_FALSE(callback_called_); nit: Do you ever EXPECT_TRUE(callback_called_) in any other tests? If not then how do you know it's not *always* false, in which case we're broken, but this test will still pass? :P
https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider.h (right): https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider.h:47: void OnLocationUpdate(const device::Geoposition& geoposition); On 2016/10/07 21:42:46, Wez wrote: > This should be private, since it's an internal implementation detail of > BlimpLocationProvider. > > Since you need to Bind() to it from the InvokeGeolocationCallback() function you > may need to move that function to be a static member, so it has visibility into > the private bit of the class. Done. https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... File blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc (right): https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:52: bool callback_called_ = false; On 2016/10/07 21:42:46, Wez wrote: > nit: Suggest on_location_update_called_ for clarity. Done. https://codereview.chromium.org/2328453003/diff/140001/blimp/engine/feature/g... blimp/engine/feature/geolocation/blimp_location_provider_unittest.cc:171: EXPECT_FALSE(callback_called_); On 2016/10/07 21:42:46, Wez wrote: > nit: Do you ever EXPECT_TRUE(callback_called_) in any other tests? If not then > how do you know it's not *always* false, in which case we're broken, but this > test will still pass? :P Done.
The CQ bit was checked by wez@chromium.org
lgtm
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 #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Makes use of EngineGeolocationFeature weak_ptr threadsafe. There was an issue where a weak_ptr to EngineGeolocationFeature was being invalidated while another thread was accessing it. This CL corrects this issue. BUG=644483 ========== to ========== Makes use of EngineGeolocationFeature weak_ptr threadsafe. There was an issue where a weak_ptr to EngineGeolocationFeature was being invalidated while another thread was accessing it. This CL corrects this issue. BUG=644483 Committed: https://crrev.com/4b44beefc9298186e5e1e27d0c2f90244bcbfef6 Cr-Commit-Position: refs/heads/master@{#424029} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4b44beefc9298186e5e1e27d0c2f90244bcbfef6 Cr-Commit-Position: refs/heads/master@{#424029} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
