New issue
Advanced search Search tips

Issue 703217 link

Starred by 2 users
Status: Archived
Owner:
derat@chromium.org
Closed: Jun 2017
Cc:
teravest@chromium.org
jamescook@chromium.org
mustash-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task

Blocking:
issue 692246



Sign in to add a comment

Create org.chromium.NetworkProxyService service

Project Member Reported by derat@chromium.org, Mar 20 2017 
Chrome's ResolveNetworkProxy D-Bus method, currently exported via org.chromium.LibCrosService, should be moved to a new service:

name:      org.chromium.NetworkProxyService
path:      /org/chromium/NetworkProxyService
interface: org.chromium.NetworkProxyServiceInterface

(I'm not opinionated about the names. They could perhaps be NetworkProxyResolverService or ProxyResolverService or something like that to indicate that the service only does resolution instead of actual proxying.)

As part of this, we should also make the method reply asynchronously instead of emitting a signal ( issue 446115 ).

This service is called by multiple clients:

crash-reporter
update_engine
tlsdated
network_ProxyResolver autotest
security_DbusMap (which looks like it compares exported methods against a hardcoded list, argh why do we keep doing this to ourselves? :-/)

Comment 1 by jamescook@chromium.org, Mar 20 2017 

SGTM. I'm not opinionated about the names either.

Comment 2 by derat@chromium.org, Mar 22 2017

Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/173bb6fcd2e5543829acbe70ae356ceba4922c6f

commit 173bb6fcd2e5543829acbe70ae356ceba4922c6f
Author: derat <derat@chromium.org>
Date: Wed Mar 22 22:37:06 2017

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}

[modify] https://crrev.com/173bb6fcd2e5543829acbe70ae356ceba4922c6f/chromeos/dbus/services/proxy_resolution_service_provider.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1a7452ea991283a3b83942348e0d2c35a34fea9

commit e1a7452ea991283a3b83942348e0d2c35a34fea9
Author: derat <derat@chromium.org>
Date: Thu Mar 23 03:53:28 2017

chromeos:: Remove pointless ProxyResolverInterface.

Remove an interface that has a single implementation. Maybe
someone planned to use it to stub out ProxyResolverImpl at
some point in the distant past but got distracted. The class
that owns it is tested now, in any case.

Also hoist ProxyResolverImpl's implementation into
ProxyResolutionServiceProvider and rename
ProxyResolverDelegate to
ProxyResolutionServiceProvider::Delegate.

BUG= 446115 , 703217 

Review-Url: https://codereview.chromium.org/2775503002
Cr-Commit-Position: refs/heads/master@{#458998}

[modify] https://crrev.com/e1a7452ea991283a3b83942348e0d2c35a34fea9/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/e1a7452ea991283a3b83942348e0d2c35a34fea9/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[rename] https://crrev.com/e1a7452ea991283a3b83942348e0d2c35a34fea9/chrome/browser/chromeos/dbus/chrome_proxy_resolution_service_provider_delegate.cc
[add] https://crrev.com/e1a7452ea991283a3b83942348e0d2c35a34fea9/chrome/browser/chromeos/dbus/chrome_proxy_resolution_service_provider_delegate.h
[delete] https://crrev.com/31843a5be08800e6bdf2a1f7c07eef342ad1abc6/chrome/browser/chromeos/dbus/chrome_proxy_resolver_delegate.h
[modify] https://crrev.com/e1a7452ea991283a3b83942348e0d2c35a34fea9/chromeos/dbus/services/proxy_resolution_service_provider.cc
[modify] https://crrev.com/e1a7452ea991283a3b83942348e0d2c35a34fea9/chromeos/dbus/services/proxy_resolution_service_provider.h
[modify] https://crrev.com/e1a7452ea991283a3b83942348e0d2c35a34fea9/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/6a5e98e4349c435dc06fc07908be0a91039e9583

commit 6a5e98e4349c435dc06fc07908be0a91039e9583
Author: Daniel Erat <derat@chromium.org>
Date: Thu Mar 23 18:55:51 2017

system_api: Add org.chromium.NetworkProxyService constants.

Add constants for a new org.chromium.NetworkProxyService
D-Bus service that will hold the ResolveNetworkProxy
(renamed to ResolveProxy) method that's currently exported
via org.chromium.LibCrosService.

BUG= chromium:703217 , chromium:446115 
TEST=built it

Change-Id: I9fc9462968aa55d48f6ebc9755140cb65bc0e1ef
Reviewed-on: https://chromium-review.googlesource.com/457926
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/6a5e98e4349c435dc06fc07908be0a91039e9583/dbus/service_constants.h
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 30 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34a136576bbddba2bc7cbd1bc6fc82a6c979b151

commit 34a136576bbddba2bc7cbd1bc6fc82a6c979b151
Author: derat <derat@chromium.org>
Date: Thu Mar 30 03:29:20 2017

chromeos: Add D-Bus config for org.chromium.NetworkProxyService.

Add a D-Bus config file letting chronos own the
org.chromium.NetworkProxyService service name and letting
all processes make calls to it.

BUG= 703217 

Review-Url: https://codereview.chromium.org/2783823004
Cr-Commit-Position: refs/heads/master@{#460643}

[modify] https://crrev.com/34a136576bbddba2bc7cbd1bc6fc82a6c979b151/chromeos/BUILD.gn
[modify] https://crrev.com/34a136576bbddba2bc7cbd1bc6fc82a6c979b151/chromeos/dbus/services/README.md
[add] https://crrev.com/34a136576bbddba2bc7cbd1bc6fc82a6c979b151/chromeos/dbus/services/org.chromium.NetworkProxyService.conf

Comment 10 by jorgelo@chromium.org, Apr 27 2017 

FWIW I've removed the security_DbusMap test, but in any case it was not being run in any suites.
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8d0d8a95e1dd4dc10565e7c3acee4daefd417d75

commit 8d0d8a95e1dd4dc10565e7c3acee4daefd417d75
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 27 05:13:13 2017

crash: Update list_proxies to call new D-Bus service.

Update crash-reporter's list_proxy program to call Chrome's
new org.chromium.NetworkProxyService D-Bus service rather
than org.chromium.LibCrosService. Also drop XML bindings and
use async method calls instead of the old, more-complicated
signal-requesting approach.

BUG= chromium:446115 , chromium:703217 
TEST=ran it after configuring Chrome to use direct
     connections or a PAC file and verified that correct
     connection info was printed

Change-Id: I17202e7fb917f73956a7c6c514b1adc584ffd258
Reviewed-on: https://chromium-review.googlesource.com/487732
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/8d0d8a95e1dd4dc10565e7c3acee4daefd417d75/crash-reporter/list_proxies.cc
[modify] https://crrev.com/8d0d8a95e1dd4dc10565e7c3acee4daefd417d75/crash-reporter/crash-reporter.gyp
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/tlsdate/+/8ccda0f645b65dc82f8676ce321406938a35d0b4

commit 8ccda0f645b65dc82f8676ce321406938a35d0b4
Author: Daniel Erat <derat@chromium.org>
Date: Fri Apr 28 09:58:13 2017

tlsdate: Use org.chromium.NetworkProxyService.

Make tlsdated make async method calls to the
org.chromium.NetworkProxyService D-Bus service to resolve
proxy information. This replaces old, weird
signal-requesting calls to org.chromium.LibCrosService.

Also remove some unused DBusError variables.

BUG= chromium:446115 , chromium:703217 
TEST=added logging and verified via /var/log/tlsdate.log
     that tlsdated is using proxy information configured in
     chrome

Change-Id: I665e2c78ac00e78ff4f6692c0b9b060ca5d92c4e
Reviewed-on: https://chromium-review.googlesource.com/487611
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

[modify] https://crrev.com/8ccda0f645b65dc82f8676ce321406938a35d0b4/src/platform-cros.c
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/7b81a313d627bc9458f1a072e4259333cfba7337

commit 7b81a313d627bc9458f1a072e4259333cfba7337
Author: Daniel Erat <derat@chromium.org>
Date: Fri Apr 28 09:58:14 2017

autotest: Update network_ProxyResolver D-Bus call.

Make the network_ProxyResolver test make an async method
call to the org.chromium.NetworkProxyService D-Bus service
instead of requesting a signal via
org.chromium.LibCrosService.

BUG= chromium:446115 , chromium:703217 
TEST=ran it

Change-Id: Ib1fe560ee9543bf56b96ae2f29ec671c07c999c7
Reviewed-on: https://chromium-review.googlesource.com/489462
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

[modify] https://crrev.com/7b81a313d627bc9458f1a072e4259333cfba7337/client/site_tests/network_ProxyResolver/network_ProxyResolver.py
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/4ca5602b59e1cf06ce97049615d3b460a4c80353

commit 4ca5602b59e1cf06ce97049615d3b460a4c80353
Author: Daniel Erat <derat@chromium.org>
Date: Sat Apr 29 00:12:40 2017

crash: Remove XML D-Bus spec for LibCrosService.

This file is unused and should've been deleted as part of
8d0d8a95e1dd4dc1.

BUG= chromium:446115 , chromium:703217 
TEST=built it
TBR=vapier@chromium.org,teravest@chromium.org

Change-Id: I26edd1a4d6d5fe3f9095ee3379d5d852859a4d5d
Reviewed-on: https://chromium-review.googlesource.com/490406
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>

[delete] https://crrev.com/42233fde5e833652c3d635ee9964ba0fd917cd51/crash-reporter/dbus_bindings/org.chromium.LibCrosService.xml
Project Member

Comment 15 by bugdroid1@chromium.org, May 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/update_engine/+/941cf235c5e56eddc6e4f2de2f38bee032a4dead

commit 941cf235c5e56eddc6e4f2de2f38bee032a4dead
Author: Daniel Erat <derat@chromium.org>
Date: Sat May 13 05:48:59 2017

update_engine: Use org.chromium.NetworkProxyService.

Make update_engine call Chrome's new
org.chromium.NetworkProxyService D-Bus service to resolve
network proxies instead of using
org.chromium.LibCrosService. The new service supports
asynchronous replies instead of responding via D-Bus
signals.

BUG= chromium:446115 , chromium:703217 
TEST=unit tests pass; also added debug logging and verified
     that chrome's proxy settings are used

Change-Id: Iebd268ea3e551c0760416d955828b9d7ebf851fb
Reviewed-on: https://chromium-review.googlesource.com/497491
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/common/http_fetcher.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/real_system_state.h
[delete] https://crrev.com/c00ec56f1f209d6841d5784e799f9cc0cbe0f26d/libcros_proxy.h
[add] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/dbus_bindings/org.chromium.NetworkProxyService.dbus-xml
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/common/libcurl_http_fetcher.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_attempter_unittest.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_manager/real_system_provider.h
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/chrome_browser_proxy_resolver.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_manager/real_system_provider_unittest.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/dbus_bindings/org.chromium.LibCrosService.dbus-xml
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_manager/real_system_provider.cc
[delete] https://crrev.com/c00ec56f1f209d6841d5784e799f9cc0cbe0f26d/libcros_proxy.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_manager/state_factory.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/real_system_state.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_manager/state_factory.h
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/chrome_browser_proxy_resolver_unittest.cc
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/common/http_fetcher.h
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/chrome_browser_proxy_resolver.h
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_engine.gyp
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_attempter.h
[modify] https://crrev.com/941cf235c5e56eddc6e4f2de2f38bee032a4dead/update_attempter.cc
Project Member

Comment 16 by bugdroid1@chromium.org, Jun 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67fdc5416dcf8317fe5b43f6afd999100e540c9d

commit 67fdc5416dcf8317fe5b43f6afd999100e540c9d
Author: derat <derat@chromium.org>
Date: Wed Jun 14 21:35:06 2017

chromeos: Remove LibCrosService's ResolveNetworkProxy method

Remove the ResolveNetworkProxy D-Bus method exported by
org.chromium.LibCrosService, along with support for
requesting that the response be sent via a signal (as
opposed to an async method call).

This method has been renamed to ResolveProxy and lives in a
new org.chromium.NetworkProxyService service. All Chrome OS
callers (tlsdated, crash-reporter, update_engine) have been
updated to use the new service.

BUG= 446115 , 703217 

Review-Url: https://codereview.chromium.org/2880083002
Cr-Commit-Position: refs/heads/master@{#479509}

[modify] https://crrev.com/67fdc5416dcf8317fe5b43f6afd999100e540c9d/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/67fdc5416dcf8317fe5b43f6afd999100e540c9d/chromeos/dbus/services/proxy_resolution_service_provider.cc
[modify] https://crrev.com/67fdc5416dcf8317fe5b43f6afd999100e540c9d/chromeos/dbus/services/proxy_resolution_service_provider.h
[modify] https://crrev.com/67fdc5416dcf8317fe5b43f6afd999100e540c9d/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc

Comment 17 by derat@chromium.org, Jun 22 2017

Components: OS>Systems
Status: Fixed (was: Started)

Comment 18 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
►

Sign in to add a comment