New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 875502 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

TetherService allows Magic Tether even when feature is supported by not enabled

Project Member Reported by khorimoto@chromium.org, Aug 17

Issue description

In TetherService (see [1]), we grab the HostStatus from MultiDeviceSetupClient and check the Instant Tethering preference; if the host is verified and the preference is enabled, we move forward.

However, this does not check whether the phone has set the feature to the "enabled" state. Thus, we should change this check to use MultiDeviceSetupClient::GetFeatureStates(). If the state for Instant Tethering is kEnabledByUser, we should continue; otherwise, we should not. There is no need to check the preference or the host status, as the feature state encapsulates both of those internally.

[1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/tether/tether_service.h
 
I actually already switched to using kEnabled in my cl (https://chromium-review.googlesource.com/c/chromium/src/+/1180473). I think that's the right thing to do to cover both this bug and the other one.
That's actually not quite good enough. If the phone has the feature enabled and the Instant Tethering preference is enabled *but* the Better Together suite's preference is disabled, we shouldn't be initializing the component.
Owner: khorimoto@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 31

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

commit 7bddbb1336df98bcb11941ea2725e4466e7a60ab
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Aug 31 01:15:42 2018

[CrOS MultiDevice] Only start up Instant Tethering when enabled.

This CL updates TetherService and TetherHostFetcher to use the
MultiDeviceSetupClient's GetFeatureState() function to determine whether
the feature should be active or not. This fixes a few edge cases:
  (1) If the Instant Tethering user preference is enabled, but the
      Better Together suite is disabled, we should not start up the
      feature.
  (2) If the preference is enabled, but the host device does not support
      Instant Tethering, we should not start up the feature.

This CL eliminates the MULTIDEVICE_HOST_UNVERIFIED metrics enum since
an unverified host results in NO_AVAILABLE_HOSTS; it replaces that enum
value with BETTER_TOGETHER_SUITE_DISABLED, which can occur in practice.
Note that because the old metric value has not yet been pushed to users,
it is okay to replace it with a new value instead of deprecating it.

Bug:  875502 , 824568
Change-Id: Ia85f0ba31871c94ed9eb0cb2ef3c4699bb0000dd
Reviewed-on: https://chromium-review.googlesource.com/1196172
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587894}
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_component.h
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_component_impl.cc
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_host_fetcher_impl.cc
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_host_fetcher_impl.h
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_session_completion_logger.h
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_session_completion_logger_unittest.cc
[modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/tools/metrics/histograms/enums.xml

Status: Fixed (was: Started)
Re comment 1:

"There is no need to check the preference or the host status, as the feature state encapsulates both of those internally."

This sounds potentially fragile, i.e., this may break if that contract changes in the future.  Am I missing something?
Re comment 2:

"If the phone has the feature enabled and the Instant Tethering preference is enabled *but* the Better Together suite's preference is disabled"

How does this case happen?  I think I'm getting confused by all of the different toggles in the multidevice world (and that causes me some concern).
Re: comment #6: The thing you're missing is that we still need to support both Instant Tethering and EasyUnlock with the MultiDevice flags turned *off*. Thus, we still need a way for the features to talk to the prefs directly in that case, but when the flag is on, these features should talk to MultiDeviceSetup service.

Re: comment #7: This is how this situation happens:
(1) Start with all toggles on, including both the Better Together suite and Instant Tethering.
(2) Turn the Better Together suite toggle off, but do not touch the Instant Tethering toggle (it should still be on).
(3) Instant Tethering should not be active, but its toggle is still on.

Please read through go/cros-multidevice-prefs if you have more questions.
Labels: Merge-Request-70
Status: Started (was: Fixed)
Looks like this CL didn't make it into M-70; merge requested please!
Project Member

Comment 10 by sheriffbot@chromium.org, Sep 1

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 11 by sheriffbot@chromium.org, Sep 5

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: hansberry@chromium.org
Labels: -Hotlist-Merge-Approved -Merge-Approved-70
Status: Fixed (was: Started)
Merged: https://chromium-review.googlesource.com/c/chromium/src/+/1208139
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 5

Labels: merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8665b5bb5f08911931fc5972311c509bc803323

commit d8665b5bb5f08911931fc5972311c509bc803323
Author: Ryan Hansberry <hansberry@chromium.org>
Date: Wed Sep 05 21:51:21 2018

[CrOS MultiDevice] Only start up Instant Tethering when enabled.

This CL updates TetherService and TetherHostFetcher to use the
MultiDeviceSetupClient's GetFeatureState() function to determine whether
the feature should be active or not. This fixes a few edge cases:
  (1) If the Instant Tethering user preference is enabled, but the
      Better Together suite is disabled, we should not start up the
      feature.
  (2) If the preference is enabled, but the host device does not support
      Instant Tethering, we should not start up the feature.

This CL eliminates the MULTIDEVICE_HOST_UNVERIFIED metrics enum since
an unverified host results in NO_AVAILABLE_HOSTS; it replaces that enum
value with BETTER_TOGETHER_SUITE_DISABLED, which can occur in practice.
Note that because the old metric value has not yet been pushed to users,
it is okay to replace it with a new value instead of deprecating it.

TBR=khorimoto@google.com

(cherry picked from commit 7bddbb1336df98bcb11941ea2725e4466e7a60ab)

Bug:  875502 , 824568
Change-Id: Ia85f0ba31871c94ed9eb0cb2ef3c4699bb0000dd
Reviewed-on: https://chromium-review.googlesource.com/1196172
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#587894}
Reviewed-on: https://chromium-review.googlesource.com/1208139
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#68}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_component.h
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_component_impl.cc
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_host_fetcher_impl.cc
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_host_fetcher_impl.h
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_session_completion_logger.h
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_session_completion_logger_unittest.cc
[modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/tools/metrics/histograms/enums.xml

Sign in to add a comment