[chrome.mdns] Don't publish an event on listener add/remove |
||||||||
Issue descriptionVersion: ChromeOS 53 chrome.mdns forces a deviceList event each time there is callback from the NetworkChangeNotifier. Because of Bug 644501 , this results in continuous Media Router wakeups on ChromeOS. This bug tracks a fix to not force-fire an event in this case. Notes: ServiceDiscoveryClientMdns::OnNetworkChanged => ServiceWatcherProxy::OnNewMdnsReady => ServiceWatcher::UpdatedCallback(ServiceWatcher::UPDATE_INVALIDATED) => ServiceDiscoveryClientMdns::OnServiceUpdated() => ServiceDiscoveryDeviceLister::Delegate::OnDeviceCacheFlushed() => (implemented by) DnsSdDeviceLister::OnDeviceCachedFlushed() => DnsSdDelegate::ServicesFlushed(<service_name>) (implemented by) DnsSdRegistry::ServicesFlushed() => DispatchApiEvent() DiscoverNewDevices() (may also cause an event to be dispatched) There is code to not fire an event if there are no services. But it seems to not help this case. https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/mdns/dns_sd_registry.cc?rcl=1473866283&l=223
,
Sep 14 2016
Marking as RB-S for M-54 as well.
,
Sep 14 2016
What devices are affected by this on M53?
,
Sep 15 2016
Issue 644501 is not the root cause of this but this may have the same root cause as issue 646995 . Can you provide a JS snippet to repro this?
,
Sep 16 2016
We are still investigating this. The root cause is still likely to be the behavior in Bug 644501 , but in a very indirect way: the event page wakeup causes a new event listener to be added to onServiceList, which triggers the API to publish an event to all event handlers. APIs that publish events on listener manipulation are not good because of behavior like this. The API should be refactored to have a getter separate from the event handler for clients to intitialize their state. Regardless, we should fix the publish behavior as it's not event page friendly.
,
Sep 16 2016
Removing R-B-S as there's no short term fix to this API required.
,
Sep 19 2016
I don't understand what any of comment #5 has to do with issue 644501 . chrome.networkingPrivate.onNetworksChanged is behaving as desired and intended for its primary use which is active pages like Settings that want to be notified any time a change occurs. Other pages do not need to subscribe to the event. The API also has getters available independent of the event handler. It is documented here: https://developer.chrome.com/extensions/networkingPrivate.
,
Sep 20 2016
Re: c#7 I'm not arguing about the behavior of the networkingPrivate API any more. As you say, it's intended for continuous monitoring/scanning of network properties and we shouldn't use it for our use case. It happened to expose a different problem in this API. Remove the blocking label if you like.
,
Oct 5 2016
,
Nov 2 2016
,
May 10 2017
Next item is to scope the work involved w/o breaking existing clients.
,
Apr 5 2018
Presentation API no longer depends on the chrome.mdns API (or chrome.networkingPrivate for that matter) so removing us from CC. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by mfo...@chromium.org
, Sep 14 2016