Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786103002:
chromeos: Avoid DCHECK in ProxyResolutionServiceProvider. (Closed)
|
Created:
3 years, 8 months ago by Daniel Erat Modified:
3 years, 8 months ago Reviewers:
James Cook CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org, oshima+watch_chromium.org, hashimoto+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Avoid DCHECK in ProxyResolutionServiceProvider.
ProxyResolutionServiceProvider was incorrectly dereferencing
a WeakPtr from a thread other than the one on which it was
created. Make several of its private members be static and
pass more data into them to avoid this.
Also make tests create a network thread to catch issues like
this.
BUG=706665
Review-Url: https://codereview.chromium.org/2786103002
Cr-Commit-Position: refs/heads/master@{#461599}
Committed: https://chromium.googlesource.com/chromium/src/+/8cfe70b3bca63aec509c39bf48c384e7eba3372b
Patch Set 1 #Patch Set 2 : make tests multithreaded #Patch Set 3 : merge with change to create net::ProxyService in tests #
Total comments: 10
Patch Set 4 : add comments and fix race #Patch Set 5 : rename RunAllTasks to RunPendingTasks to avoid tricking future me #
Messages
Total messages: 26 (19 generated)
The CQ bit was checked by derat@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.
derat@chromium.org changed reviewers: + jamescook@chromium.org
sigh, i'd forgotten that WeakPtr can't be dereferenced across threads. :-/ let me know if you can think of a cleaner way to do this.
The CQ bit was checked by derat@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.
Description was changed from ========== chromeos: Avoid DCHECK in ProxyResolutionServiceProvider. ProxyResolutionServiceProvider was incorrectly dereferencing a WeakPtr from a thread other than the one on which it was created. Make several of its private members be static and pass more data into them to avoid this. BUG=706665 ========== to ========== chromeos: Avoid DCHECK in ProxyResolutionServiceProvider. ProxyResolutionServiceProvider was incorrectly dereferencing a WeakPtr from a thread other than the one on which it was created. Make several of its private members be static and pass more data into them to avoid this. Also make tests create a network thread to catch issues like this. BUG=706665 ==========
mind taking another look at this now? https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc (right): https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:130: RunAllTasks(network_task_runner); yuck. https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:137: RunAllTasks(context_getter_->GetNetworkTaskRunner()); yuck again.
The CQ bit was checked by derat@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.
LGTM https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:206: base::Passed(std::move(request)), notify_thread, notify_callback); optional: Add a comment near here (or in header?) saying we can't use GetWeakPtr() here and that's the reason these methods are static? https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider.h (right): https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.h:114: NotifyCallback notify_callback); I really appreciate your consistent naming of things like notify_*. https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc (right): https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:130: RunAllTasks(network_task_runner); On 2017/04/01 00:19:19, Daniel Erat wrote: > yuck. lol https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:169: std::unique_ptr<net::ProxyService> proxy_service_; optional: comment that it lives on network thread? https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:364: } // namespace chromeos Awesome that you were able to make the tests cover the threading.
https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:206: base::Passed(std::move(request)), notify_thread, notify_callback); On 2017/04/03 15:30:23, James Cook wrote: > optional: Add a comment near here (or in header?) saying we can't use > GetWeakPtr() here and that's the reason these methods are static? Done. https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc (right): https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:169: std::unique_ptr<net::ProxyService> proxy_service_; On 2017/04/03 15:30:23, James Cook wrote: > optional: comment that it lives on network thread? Done. https://codereview.chromium.org/2786103002/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc:267: base::RunLoop().RunUntilIdle(); sigh, it turns out there's a tricky race here. i think that this is what happens: - the first RunUntilIdle call presumably doesn't do anything, as we're running ResolveProxy synchronously (whoops). ResolveProxyOnNetworkThread is posted to the network thread, as expected. - the RunAllTasks call results in ResolveProxyOnNetworkThread running, but looking at ThreadTestHelper's implementation, it just posts a task and waits for it to be executed (instead of running the thread until it has no scheduled tasks). as a result, in the async-lookup case, RunAllTasks can return before ResolveProxyOnNetworkThread runs on the network thread. i initially looked into making RunAllTasks actually wait for a task that runs RunLoop::RunUntilIdle on the target thread, but that doesn't work either. i suspect that net::ProxyService's threading story is more complicated than i thought. luckily, it's not to hard to make us just run the main message loop here until we receive the d-bus signal, which seems to do the trick and which is what i should've done in the first place instead of trying to run multiple message loops in the correct order. and even better, that code will go away once this class only sends d-bus responses and no longer emits signals.
The CQ bit was checked by derat@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...
still lgtm
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491265251452460,
"parent_rev": "be66fed8cd273727437c2bfc1cf5952eba00959e", "commit_rev":
"8cfe70b3bca63aec509c39bf48c384e7eba3372b"}
Message was sent while issue was closed.
Description was changed from ========== chromeos: Avoid DCHECK in ProxyResolutionServiceProvider. ProxyResolutionServiceProvider was incorrectly dereferencing a WeakPtr from a thread other than the one on which it was created. Make several of its private members be static and pass more data into them to avoid this. Also make tests create a network thread to catch issues like this. BUG=706665 ========== to ========== chromeos: Avoid DCHECK in ProxyResolutionServiceProvider. ProxyResolutionServiceProvider was incorrectly dereferencing a WeakPtr from a thread other than the one on which it was created. Make several of its private members be static and pass more data into them to avoid this. Also make tests create a network thread to catch issues like this. BUG=706665 Review-Url: https://codereview.chromium.org/2786103002 Cr-Commit-Position: refs/heads/master@{#461599} Committed: https://chromium.googlesource.com/chromium/src/+/8cfe70b3bca63aec509c39bf48c3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8cfe70b3bca63aec509c39bf48c3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
