Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763963003:
chromeos: Simplify D-Bus proxy resolution service code. (Closed)
|
Created:
3 years, 9 months ago by Daniel Erat Modified:
3 years, 9 months ago CC:
chromium-reviews, oshima+watch_chromium.org, hashimoto+watch_chromium.org, satorux1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Simplify D-Bus proxy resolution service code.
Try to simplify ProxyResolutionServiceProvider's
implementation in preparation for future changes.
Specifically, reduce the use of raw pointers and bind fewer
parameters into callbacks.
BUG=446115, 703217
TEST=list_proxies works both with a manual proxy config and
a trivial .pac file that requires async resolution
Review-Url: https://codereview.chromium.org/2763963003
Cr-Commit-Position: refs/heads/master@{#458902}
Committed: https://chromium.googlesource.com/chromium/src/+/173bb6fcd2e5543829acbe70ae356ceba4922c6f
Patch Set 1 #
Total comments: 6
Patch Set 2 : put Request in unique_ptr #Patch Set 3 : make Request private #
Total comments: 7
Patch Set 4 : apply feedback: un-inline ProxyResolverImpl and rename method to ResolveProxyOnNetworkThread #
Total comments: 1
Patch Set 5 : fix crash #Messages
Total messages: 31 (22 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...
derat@chromium.org changed reviewers: + jamescook@chromium.org, teravest@chromium.org
sorry, feature creep definitely started to set in here. i think there's at least one more cleanup change needed after this one. https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... File chromeos/dbus/services/proxy_resolution_service_provider.cc (left): https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider.cc:116: request->error_ = "No URLRequestContextGetter"; i suspect that the old code is susceptible to use-after-free races. ResolveProxy posts this static method to the network thread, but it looks to me like ProxyResolverImpl could be deleted before it runs, resulting in all outstanding requests being deleted. https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider.cc:34: // Request object in that case. i initially considered binding base::WeakPtr<Request> instead, but that requires ProxyResolverImpl to continue holding onto a std::unique_ptr<Request> until resolution is complete. i think that a better implementation of net::ProxyService::ResolveProxy() would either always post the callback that's passed to it, or maybe return the callback so the caller can decide what to do with it and with any objects that are owned by it. https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider.cc:50: notify_task_(notify_task), note that members like this one should be unnecessary if the later TODO about moving the implementation into this class is resolved. https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider.cc:169: dbus::Signal signal(request->signal_interface_, request->signal_name_); moving the implementation into Request will also avoid these weird member accesses.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
satorux@chromium.org changed reviewers: + satorux@chromium.org
https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider.cc:33: // synchronously, it won't be, and we don't want to lose ownership of the > if the proxy can be resolved synchronously, it won't be I'm confused. Even if it's completed synchronously, it seems that the callback is run: 157 if (result != net::ERR_IO_PENDING) { 158 VLOG(1) << "Network proxy resolution completed synchronously."; 159 completion_callback.Run(result); 160 } I'm guessing I'm misunderstanding something.
you might want to hold off on reviewing this until i finish wading through unreadable std::move() template errors. :-/ https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2763963003/diff/1/chromeos/dbus/services/prox... chromeos/dbus/services/proxy_resolution_service_provider.cc:33: // synchronously, it won't be, and we don't want to lose ownership of the On 2017/03/22 04:04:02, satorux1 wrote: > > if the proxy can be resolved synchronously, it won't be > > I'm confused. Even if it's completed synchronously, it seems that the callback > is run: > > 157 if (result != net::ERR_IO_PENDING) { > 158 VLOG(1) << "Network proxy resolution completed synchronously."; > 159 completion_callback.Run(result); > 160 } > > I'm guessing I'm misunderstanding something. sorry if the comment is unclear. i meant that net::ProxyService::ResolveProxy() doesn't run the CompletionCallback that's passed to it if the proxy is resolved synchronously. it's likely that i'm misunderstanding how RepeatingCallback<> handles move-only parameters that are bound into it, though. upon rereading the docs, it sounds like it uses internal reference counting, so presumably it's safe for ResolveProxyInternal to std::move the Request into the callback and then run the callback itself if the proxy is resolved synchronously. i'll give that a try.
i think that this is ready for review now. thanks! https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider.cc (left): https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:29: class Request { this is moved to the private section. i also made it a (non-copyable) struct, removed the OnCompletion method (which is now ProxyResolverImpl::OnResolutionComplete), and changed the members around a bit. https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:115: if (!getter.get()) { i don't understand the reason for this check. it's the same object that's used to get the thread that this method is posted to, so it was obviously valid at the time of the refcount being incremented. https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:137: GURL(request->source_url_), std::string(), &request->proxy_info_, i mentioned it in an earlier patch set, but i think that the references to |request| in the old version of this method are potential use-after-frees. this is a static method, so there's no weak_ptr to guarantee that ProxyResolverImpl is still around, and it deletes all of its Requests on destruction.
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...
Description was changed from
==========
chromeos: Simplify D-Bus proxy resolution service code.
Try to simplify ProxyResolutionServiceProvider's
implementation in preparation for future changes.
Specifically, reduce the use of raw pointers and bind fewer
parameters into callbacks. Unfortunately, the extent to
which memory ownership can be clarified appears to be
limited by net::ProxyService::ResolveProxy()'s design.
BUG=446115,703217
TEST=list_proxies works both with a manual proxy config and
a trivial .pac file that requires async resolution
==========
to
==========
chromeos: Simplify D-Bus proxy resolution service code.
Try to simplify ProxyResolutionServiceProvider's
implementation in preparation for future changes.
Specifically, reduce the use of raw pointers and bind fewer
parameters into callbacks.
BUG=446115,703217
TEST=list_proxies works both with a manual proxy config and
a trivial .pac file that requires async resolution
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. This does seem cleaner. https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:38: void ResolveProxy( nit: There's a lot of inline code in these class methods, which makes the member variables far away. I think it would be more readable if the method implementations were de-inlined. https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:102: void ResolveProxyInternal(std::unique_ptr<Request> request) { nit: Maybe ResolveProxyOnNetworkThread()?
i also noticed that the test coverage for this code is quite poor (e.g. the async stuff doesn't get exercised), so i have a followup change that improves it. https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:38: void ResolveProxy( On 2017/03/22 19:53:20, James Cook wrote: > nit: There's a lot of inline code in these class methods, which makes the member > variables far away. I think it would be more readable if the method > implementations were de-inlined. sure, i've moved the implementations out. https://codereview.chromium.org/2763963003/diff/40001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:102: void ResolveProxyInternal(std::unique_ptr<Request> request) { On 2017/03/22 19:53:20, James Cook wrote: > nit: Maybe ResolveProxyOnNetworkThread()? Done.
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.
https://codereview.chromium.org/2763963003/diff/60001/chromeos/dbus/services/... File chromeos/dbus/services/proxy_resolution_service_provider.cc (right): https://codereview.chromium.org/2763963003/diff/60001/chromeos/dbus/services/... chromeos/dbus/services/proxy_resolution_service_provider.cc:124: request->context_getter->GetNetworkTaskRunner()->PostTask( whoops, introduced a bug here (dereferencing |request| and also std::move-ing it within the argument list). it segfaults on the device but not in my unit tests; i'm having a hard time finding whether the spec makes any promises about which is evaluated first.
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 derat@chromium.org
The CQ bit was checked by derat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2763963003/#ps80001 (title: "fix crash")
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": 1490219618277970,
"parent_rev": "87648263de7c79dc8966235e1567067173d7b5d3", "commit_rev":
"173bb6fcd2e5543829acbe70ae356ceba4922c6f"}
Message was sent while issue was closed.
Description was changed from
==========
chromeos: Simplify D-Bus proxy resolution service code.
Try to simplify ProxyResolutionServiceProvider's
implementation in preparation for future changes.
Specifically, reduce the use of raw pointers and bind fewer
parameters into callbacks.
BUG=446115,703217
TEST=list_proxies works both with a manual proxy config and
a trivial .pac file that requires async resolution
==========
to
==========
chromeos: Simplify D-Bus proxy resolution service code.
Try to simplify ProxyResolutionServiceProvider's
implementation in preparation for future changes.
Specifically, reduce the use of raw pointers and bind fewer
parameters into callbacks.
BUG=446115,703217
TEST=list_proxies works both with a manual proxy config and
a trivial .pac file that requires async resolution
Review-Url: https://codereview.chromium.org/2763963003
Cr-Commit-Position: refs/heads/master@{#458902}
Committed:
https://chromium.googlesource.com/chromium/src/+/173bb6fcd2e5543829acbe70ae35...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/173bb6fcd2e5543829acbe70ae35... |
