Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2950573002:
Fix chromeos::ProxyConfigServiceImpl not setting ProxyConfig on start. (Closed)
DescriptionFix chromeos::ProxyConfigServiceImpl not setting ProxyConfig on start.
The issue is due to an API mismatch.
PrefProxyConfigTrackerImpl::update_pending() means that is has a cached
config that it has yet to pass to the ConfigService, but
chromeos::ProxyConfigServiceImpl (Which, despite the name, does not
implement ProxyConfigService, but is instead a
PrefProxyConfigTrackerImpl subclass), thinks it means that
PrefProxyConfigTrackerImpl will tell it to update the config at a
future point in time.
The CL that exposed this mismatch may have been
https://codereview.chromium.org/2860033003 (Not compeltely clear), but
it looks like this is a pre-existing issue.
BUG=734565
Review-Url: https://codereview.chromium.org/2950573002
Cr-Commit-Position: refs/heads/master@{#480623}
Committed: https://chromium.googlesource.com/chromium/src/+/ed8193383116668bc6571cb1eed64f0514530724
Patch Set 1 #Patch Set 2 : Oops #
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by mmenke@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 ========== Fix chromeos::ProxyConfigServiceImpl not setting ProxyConfig on start. The issue is due to an API mismatch. PrefProxyConfigTrackerImpl::update_pending() means that is has a cached config that it has yet to pass to the ConfigService, but chromeos::ProxyConfigServiceImpl (Which, despite the name, does not implement ProxyConfigService, but is instead a PrefProxyConfigTrackerImpl subclass), thinks it means that PrefProxyConfigTrackerImpl will tell it to update the config at a future point in time. The CL that exposed this mismatch was https://codereview.chromium.org/2860033003, but it looks like this is a pre-existing issue. BUG=734565 ========== to ========== Fix chromeos::ProxyConfigServiceImpl not setting ProxyConfig on start. The issue is due to an API mismatch. PrefProxyConfigTrackerImpl::update_pending() means that is has a cached config that it has yet to pass to the ConfigService, but chromeos::ProxyConfigServiceImpl (Which, despite the name, does not implement ProxyConfigService, but is instead a PrefProxyConfigTrackerImpl subclass), thinks it means that PrefProxyConfigTrackerImpl will tell it to update the config at a future point in time. The CL that exposed this mismatch may have been https://codereview.chromium.org/2860033003 (Not compeltely clear), but it looks like this is a pre-existing issue. BUG=734565 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@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.
mmenke@chromium.org changed reviewers: + armansito@chromium.org
The lack of any tests (unit or integration) of ChromeOS's proxy config code is rather troubling, but I'm not sufficiently familiar with ChromeOS's proxy logic to create a test fixture for this code, unfortunately. I think the issue is that on start, ProxyConfigServiceImpl::DetermineEffectiveConfigFromDefaultNetwork() is called and gets the initial proxy configuration, but doesn't send it, and then it doesn't get called again unless there's a network change, but I'm not positive of that.
Sorry, armansito, didn't realize the stephenjb also owned the components/ part of this CL. Redirecting.
mmenke@chromium.org changed reviewers: + stevenjb@chromium.org - armansito@chromium.org
[+stevenjb] Oops, redirecting this CL. And a bit more detail on the breakage: https://codereview.chromium.org/2860033003 removed the OnProxyConfigChanged call in PrefProxyConfigTrackerImpl::CreateTrackingProxyConfigService, which was what actually lead to the bug. The call wasn't necessary for PrefProxyConfigTrackerImpl itself, since it had an updated configuration cached in |pref_config_|, but chromeos::ProxyConfigServiceImpl broke that, by suppressing updates to that field and not updating it to reflect what it thinks the configuration is. So this should indeed fix the issue. Still wish we had a test fixture for that class, though, so we could be a bit more confident of that.
On 2017/06/19 22:42:25, mmenke wrote: > [+stevenjb] Oops, redirecting this CL. > > And a bit more detail on the breakage: > https://codereview.chromium.org/2860033003 removed the OnProxyConfigChanged call > in PrefProxyConfigTrackerImpl::CreateTrackingProxyConfigService, which was what > actually lead to the bug. The call wasn't necessary for > PrefProxyConfigTrackerImpl itself, since it had an updated configuration cached > in |pref_config_|, but chromeos::ProxyConfigServiceImpl broke that, by > suppressing updates to that field and not updating it to reflect what it thinks > the configuration is. > > So this should indeed fix the issue. Still wish we had a test fixture for that > class, though, so we could be a bit more confident of that. "I'm not sufficiently familiar with ChromeOS's proxy logic..." Sadly, nobody is at this point :) It would be nice to add at least some sort of basic test if possible. (That said, I've also avoided doing so for the same reason).
On 2017/06/19 22:48:31, stevenjb wrote: > On 2017/06/19 22:42:25, mmenke wrote: > > [+stevenjb] Oops, redirecting this CL. > > > > And a bit more detail on the breakage: > > https://codereview.chromium.org/2860033003 removed the OnProxyConfigChanged > call > > in PrefProxyConfigTrackerImpl::CreateTrackingProxyConfigService, which was > what > > actually lead to the bug. The call wasn't necessary for > > PrefProxyConfigTrackerImpl itself, since it had an updated configuration > cached > > in |pref_config_|, but chromeos::ProxyConfigServiceImpl broke that, by > > suppressing updates to that field and not updating it to reflect what it > thinks > > the configuration is. > > > > So this should indeed fix the issue. Still wish we had a test fixture for > that > > class, though, so we could be a bit more confident of that. > > "I'm not sufficiently familiar with ChromeOS's proxy logic..." > > Sadly, nobody is at this point :) > > It would be nice to add at least some sort of basic test if possible. (That > said, I've also avoided doing so for the same reason). I'm not sure about mocking out the NetworkHandler global, or dealing with ONC configuration stuff in a test. And I don't want to spend a week or two getting up to speed and writing a test or two for this class, particularly if there's no one familiar enough with them to catch my mistakes when doing so. I'm already doing some volunteer maintenance on iOS, and don't want to get sidetracked into yet more maintenance on ChromeOS code I don't own.
I inherited this component because the other owners are no longer around. We really need to find someone to wrap their head around it, clean this all up, and add some decent tests. Maybe someday I'll get to it, but like everyone else my backlog is pretty huge. LGTM because this *seems* like a more robust approach, but I do not claim to be able to confidently review this.
On 2017/06/19 23:03:30, stevenjb wrote: > I inherited this component because the other owners are no longer around. We > really need to find someone to wrap their head around it, clean this all up, and > add some decent tests. Maybe someday I'll get to it, but like everyone else my > backlog is pretty huge. > > LGTM because this *seems* like a more robust approach, but I do not claim to be > able to confidently review this. Someone will have to learn at least some of this for the network servicification stuff - there are a bunch of classes that hook up to NetworkHandler, and it hooks up directly to prefs and some other stuff as well. Might be a good time to add tests, at least for modified classes. I'm not volunteering for that now, but there's a non-zero chance that it will end up being me, anyways.
The CQ bit was checked by mmenke@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": 20001, "attempt_start_ts": 1497913689790810,
"parent_rev": "b89575ccba3b5a18f3a5da4debe0c1ee18dc392d", "commit_rev":
"ed8193383116668bc6571cb1eed64f0514530724"}
Message was sent while issue was closed.
Description was changed from ========== Fix chromeos::ProxyConfigServiceImpl not setting ProxyConfig on start. The issue is due to an API mismatch. PrefProxyConfigTrackerImpl::update_pending() means that is has a cached config that it has yet to pass to the ConfigService, but chromeos::ProxyConfigServiceImpl (Which, despite the name, does not implement ProxyConfigService, but is instead a PrefProxyConfigTrackerImpl subclass), thinks it means that PrefProxyConfigTrackerImpl will tell it to update the config at a future point in time. The CL that exposed this mismatch may have been https://codereview.chromium.org/2860033003 (Not compeltely clear), but it looks like this is a pre-existing issue. BUG=734565 ========== to ========== Fix chromeos::ProxyConfigServiceImpl not setting ProxyConfig on start. The issue is due to an API mismatch. PrefProxyConfigTrackerImpl::update_pending() means that is has a cached config that it has yet to pass to the ConfigService, but chromeos::ProxyConfigServiceImpl (Which, despite the name, does not implement ProxyConfigService, but is instead a PrefProxyConfigTrackerImpl subclass), thinks it means that PrefProxyConfigTrackerImpl will tell it to update the config at a future point in time. The CL that exposed this mismatch may have been https://codereview.chromium.org/2860033003 (Not compeltely clear), but it looks like this is a pre-existing issue. BUG=734565 Review-Url: https://codereview.chromium.org/2950573002 Cr-Commit-Position: refs/heads/master@{#480623} Committed: https://chromium.googlesource.com/chromium/src/+/ed8193383116668bc6571cb1eed6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ed8193383116668bc6571cb1eed6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
