Skip to content

Commit

Permalink
[Nearby] Stop forwarding Stop() to the ServiceController
Browse files Browse the repository at this point in the history
When NearbyShare is toggled off, StopAllEndpoints() is called which has
the unfortunate side effect of calling Stop() on the shared
OfflineServiceController. This ends up causing two problems:

1) It breaks other clients like PhoneHub sharing the service controller.
2) The OfflineServiceController is not designed to be restarted so this
is not recoverable. If the feature is re-enabled in the same session the
user ends up having a broken nearby share experience (see crbug/1180463
and crbug/1182425).

The NC change that introduced this is here: cl/356658736 and the uprev
is here: https://crrev.com/c/2697908.

To fix this issue we simply don't forward the Stop() call to the
shared OfflineServiceController from the proxy. This is necessary to
allow both PhoneHub and Nearby Share to be able to toggle their feature
states without affecting each other. We rely on the destructor to do
the necessary final cleanup.

Long term, the API design of NC needs to be updated to properly support
multiple clients with a share OfflineServiceController see:
https://crbug.com/1176249.


(cherry picked from commit 6c0823d)

Fixed: 1180463, 1182425
Change-Id: I8c1f6e4e63a4d5b01a7e6da7e1c57b4a77814d1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2716476
Commit-Queue: James Vecore <vecore@google.com>
Reviewed-by: Josh Nohle <nohle@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#858258}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2725354
Commit-Queue: Josh Nohle <nohle@chromium.org>
Auto-Submit: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/branch-heads/4430@{#13}
Cr-Branched-From: e5ce7dc-refs/heads/master@{#857950}
  • Loading branch information
James Vecore authored and Chromium LUCI CQ committed Feb 27, 2021
1 parent 19f9876 commit 9333d21
Showing 1 changed file with 2 additions and 5 deletions.
7 changes: 2 additions & 5 deletions chrome/services/sharing/nearby/nearby_connections.cc
Expand Up @@ -130,11 +130,8 @@ class ServiceControllerProxy : public ServiceController {
}

void Stop() override {
if (!inner_service_controller_)
return;
// TODO(crbug/1176249): This stops the service controller for all Cores that
// share this inner_service_controller_.
inner_service_controller_->Stop();
// TODO(https://crbug.com/1182428): we purposefully do nothing here so we
// don't shutdown the share OfflineServiceController.
}

private:
Expand Down

0 comments on commit 9333d21

Please sign in to comment.