Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Description • 9 years ago

I checked the battery usage on my Nexus 5 where I have run Fennec Nightly as my default browser. I noticed that "mdnsd" was the #3 power user at 6%. I'm worried that the NsdManager code will increase our already poor power usage.

	Gary Chen [:xeonchen] Assignee
	Comment 1 • 9 years ago

Currently registration is default disabled, so the most possibly power consumption part of mdnsd is service discovery.

Discovery is initiated at beginning because:
1. |nsIPresentationDeviceManager| has only 2 APIs: |forceDiscovery| and |getAll|.
2. A client is supposed to get devices by |getAll| without |forceDiscovery|.

An approach is to start discovery after |getAll| is called first time.

SC, do you have any comment?

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 2 • 9 years ago

(In reply to Gary Chen [:xeonchen] from comment #1)
> Currently registration is default disabled, so the most possibly power
> consumption part of mdnsd is service discovery.
> 
> Discovery is initiated at beginning because:
> 1. |nsIPresentationDeviceManager| has only 2 APIs: |forceDiscovery| and
> |getAll|.
> 2. A client is supposed to get devices by |getAll| without |forceDiscovery|.
> 
> An approach is to start discovery after |getAll| is called first time.
> 
> SC, do you have any comment?

Sounds reasonable to me, in this case we'll need to make |getAll| async. In this case device manager can determine a reasonable length of time for doing discovery. Remember to also consider the B2GPresentationDevicePrompt.js because it also uses |nsIPresentationDeviceManager.getAvailableDevices| [1].

[1] https://dxr.mozilla.org/mozilla-central/source/b2g/components/B2GPresentationDevicePrompt.js?offset=2100#74

	Shian-Yow Wu [:swu]
	Comment 3 • 9 years ago

It should be getting much better after bug 1194532 landed.

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 4 • 9 years ago

(In reply to Shian-Yow Wu [:swu] from comment #3)
> It should be getting much better after bug 1194532 landed.

I could be wrong, but it looks like bug 1194532 upgrades b2g to a different version of mdnsd. That shouldn't affect Firefox on Android at all.

	Kevin Brosnan [Ex-Mozilla]
	Comment 5 • 9 years ago

[Tracking Requested - why for this release]: regression in battery usage

	Gary Chen [:xeonchen] Assignee
	Comment 6 • 9 years ago

Mark, do you think this issue is critical and need a patch that temporarily disable mDNS on Fennec?
I'm afraid that I still need some time to handle this issue.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 12 • 9 years ago

Comment on attachment 8658558 [details] [diff] [review]
0002-Bug-1194049-Part-2-add-discovery-timeout-support-r-s.patch

Review of attachment 8658558 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/settings.js
@@ +575,5 @@
>    },
>    'dom.mozApps.use_reviewer_certs': false,
>    'dom.mozApps.signed_apps_installable_from': 'https://marketplace.firefox.com',
>    'dom.presentation.discovery.enabled': false,
> +  'dom.presentation.discovery.timeout': 10000,

Why we need make a mozSetting? This is probably not something user knows how to config. For development using WebIDE to change the gecko pref should be enough.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +14,5 @@
>  #include "nsIPresentationDevice.h"
>  #include "nsServiceManagerUtils.h"
>  
>  #define PREF_PRESENTATION_DISCOVERY "dom.presentation.discovery.enabled"
> +#define PREF_PRESENTATION_DISCOVERY_TIMEOUT "dom.presentation.discovery.timeout"

s/timeout/timeout_ms makes it more easy to understand the time unit.

@@ +245,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mDiscoveryTimer);
> +
> +  mDiscoveryTimer->Cancel();

This line might have warning about unused return value. Include "mozilla/unused.h" and use |unused << mDiscoveryTimer->Cancel();| can suppress the warning.

@@ +322,5 @@
>  NS_IMETHODIMP
>  MulticastDNSDeviceProvider::OnDiscoveryStarted(const nsACString& aServiceType)
>  {
>    LOG_I("OnDiscoveryStarted");
>    MOZ_ASSERT(NS_IsMainThread());

Add |MOZ_ASSERT(mDiscoveryTimer);| here as well.

@@ +325,5 @@
>    LOG_I("OnDiscoveryStarted");
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  nsresult rv;
> +  if (NS_WARN_IF(NS_FAILED(rv = mDiscoveryTimer->Cancel()))) {

Timer should be canceled by |StopDiscovery| so no need to do it again here.

	Nicholas Nethercote [inactive]
	Comment 13 • 9 years ago

xeonchen, do you have any measurements that show the effect of these patches? Or are you able to otherwise estimate the improvements? Thank you.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 14 • 9 years ago

Comment on attachment 8658563 [details] [diff] [review]
0003-Bug-1194049-Part-3-clear-discovered-devices-when-re-.patch

Review of attachment 8658563 [details] [diff] [review]:
-----------------------------------------------------------------

After thinking about how complex the service found/lost can happen during the discovery period, we need to implement a simple mark-and-swipe algorithm on the cached device list. With this algorithm we can have a correct sequence of event and can prevent same service object to be recreate after discovery.

cached_device_list = [...] // each entry have two attributes, service object and state (unknown, active)

[start discovery]
1. mark all service state in list to unknown.
2. start mdns dicsocvery.

[on service found]
1. if service is found in cached list, then set its state to active
2. otherwise, create a new one with state set to active. fire a device found event.

[on service lost]
1. if service is found in cached list, delete the entry in cached list. fire a device lost event.

[on discovery stopped]
1. for each service with state 'unknown', delete the entry and fire a device lost event.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +263,5 @@
> +nsresult
> +MulticastDNSDeviceProvider::OnServiceLost(const nsACString& aServiceName)
> +{
> +  nsAutoCString serviceName;
> +  serviceName = aServiceName;

Why need to use auto string instead of the original nsACString?

@@ +269,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIPresentationDevice> device;
> +  if (NS_FAILED(mPresentationServer->GetTCPDevice(serviceName,
> +                                                  getter_AddRefs(device)))) {

Should merge with |ClearDevice|.

@@ +301,5 @@
> +  }
> +
> +  nsCOMPtr<nsIPresentationDeviceListener> listener;
> +  GetListener(getter_AddRefs(listener));
> +  if (listener) {

if (NS_SUCCEEDED(GetListener(getter_AddRefs(listener))) && listener) {

@@ +305,5 @@
> +  if (listener) {
> +    listener->RemoveDevice(device);
> +  }
> +
> +  mDeviceNames.RemoveElement(aServiceName); // ignore return value

use |unused| to ignore return value explicitly.

@@ +396,5 @@
> +
> +  // save current device names
> +  mDeviceNames.SwapElements(mPrevDeviceNames);
> +  mDeviceNames.Clear();
> +  LOG_I("mPrevDeviceNames: %d", mPrevDeviceNames.Length());

This log doesn't look helpful for future debugging.

@@ +736,4 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    mServiceName = aServiceName;
> +  LOG_I("serviceName = %s\n", mServiceName.get());

why change this line?

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +7,5 @@
>  #define mozilla_dom_presentation_provider_MulticastDNSDeviceProvider_h
>  
>  #include "mozilla/nsRefPtr.h"
>  #include "nsCOMPtr.h"
> +#include "nsCOMArray.h"

remove the unused header inclusion.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 15 • 9 years ago

Comment on attachment 8658564 [details] [diff] [review]
0004-Bug-1194049-Part-4-re-discover-when-previous-discove.patch

Review of attachment 8658564 [details] [diff] [review]:
-----------------------------------------------------------------

You only need a |mIsDiscoverying| state for the entire logic.

	Gary Chen [:xeonchen] Assignee
	Comment 16 • 9 years ago

(In reply to Nicholas Nethercote [:njn] from comment #13)
> xeonchen, do you have any measurements that show the effect of these
> patches? Or are you able to otherwise estimate the improvements? Thank you.

Because this patch will make |mdnsd| launched in a very short period (10 sec) instead of running forever.
It means in normal case the power consumption will be like it was before.

	Gary Chen [:xeonchen] Assignee
	Comment 19 • 9 years ago

Rewritten version according to comment 14 and comment 15

	Gary Chen [:xeonchen] Assignee
	Comment 20 • 9 years ago

In |nsIPresentationDeviceListener| it supports |updateDevice(nsIPresentationDevice)|,
but the attributes of |nsIPresentationDevice| are read-only.

In order to update attributes, a new function |updateTCPDevice| is added
to |nsITCPPresentationServer| to support device updating.

	Gary Chen [:xeonchen] Assignee
	Comment 21 • 9 years ago

Hi njn, do you have a recommended measurement tool for power consumption?

	Nicholas Nethercote [inactive]
	Comment 22 • 9 years ago

(In reply to Gary Chen [:xeonchen] from comment #21)
> Hi njn, do you have a recommended measurement tool for power consumption?

Not for Android, unfortunately.

For comment 0 I'm guessing that mfinkle just consulted the battery usage in the settings.

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 23 • 9 years ago

(In reply to Nicholas Nethercote [:njn] from comment #22)
> (In reply to Gary Chen [:xeonchen] from comment #21)
> > Hi njn, do you have a recommended measurement tool for power consumption?
> 
> Not for Android, unfortunately.
> 
> For comment 0 I'm guessing that mfinkle just consulted the battery usage in
> the settings.

That's correct. I just looked today and "mdnsd" is at 3% of my battery. In comparison, Firefox is at 1%.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 24 • 9 years ago

Comment on attachment 8662228 [details] [diff] [review]
Part-4-clear-discovered-devices-when-re-.patch

Review of attachment 8662228 [details] [diff] [review]:
-----------------------------------------------------------------

overall looks good, just some rearrangement to make the code more symmetric.

::: dom/presentation/PresentationDeviceManager.cpp
@@ +153,5 @@
>    NS_ENSURE_ARG_POINTER(aRetVal);
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // Bug 1194049: some providers may discontinue discovery after timeout.
> +  // Call |ForceDiscovery()| here to make sure device lists are updated.

It'll make more sense to document this behavior in IDL file. User needs to be aware that |getAvailableDevices| returns the cached device list and may need to monitor following device add/update/remove events.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +294,5 @@
> +nsresult
> +MulticastDNSDeviceProvider::UpdateDevice(const nsACString& aServiceName,
> +                                      const nsACString& aServiceType,
> +                                      const nsACString& aHost,
> +                                      uint16_t aPort)

nit: alignment

@@ +356,5 @@
> +
> +  for (uint32_t i = 0; i < mDevices.Length(); ++i) {
> +    if (mDevices[i].name == aServiceName) {
> +      mDevices.RemoveElementAt(i);
> +      break;

put the |mDevices| manipulation code into |RemoveDevice| and make |OnServiceLost| call |RemoveDevice| directly.

@@ +364,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +MulticastDNSDeviceProvider::ClearDevices(DeviceState aState)

I'll prefer |ClearUnknownDevices| instead of function overloading.

@@ +375,5 @@
> +    if (mDevices[i].state == aState) {
> +      NS_WARN_IF(NS_FAILED(RemoveDevice(mDevices[i].name)));
> +      mDevices.RemoveElementAt(i);
> +    }
> +  }

Use reverse iterator https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#1052.

@@ +383,5 @@
> +MulticastDNSDeviceProvider::ClearDevices()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  for (auto& device : mDevices) {

Use reverse iterator here, too.

@@ +685,5 @@
>      return rv;
>    }
>  
> +  for (uint32_t i = 0; i < mDevices.Length(); ++i) {
> +    if (mDevices[i].name == serviceName) {

Checking |state| first to avoid unnecessary string comparison.

@@ +693,5 @@
> +                                                 port)))) {
> +        return rv;
> +      }
> +
> +      mDevices[i].state = DeviceState::eActive;

put the |mDevices| manipulation code into |UpdateDevice|.

@@ +704,2 @@
>    }
> +  mDevices.AppendElement(Device{ serviceName, DeviceState::eActive });

put the |mDevices| manipulation code into |AddDevice|.

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +54,5 @@
> +
> +  typedef struct {
> +    nsCString name;
> +    DeviceState state;
> +  } Device;

I prefer c++ struct declaration with constructor defined.

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +625,5 @@
>    add_test(registerServiceDynamically);
>    add_test(addDevice);
>    add_test(noAddDevice);
>    add_test(addDeviceDynamically);
> +  add_test(diffDiscovery);

need a test for device update.

	Gary Chen [:xeonchen] Assignee
	Comment 25 • 9 years ago

Comment on attachment 8662228 [details] [diff] [review]
Part-4-clear-discovered-devices-when-re-.patch

Review of attachment 8662228 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +375,5 @@
> +    if (mDevices[i].state == aState) {
> +      NS_WARN_IF(NS_FAILED(RemoveDevice(mDevices[i].name)));
> +      mDevices.RemoveElementAt(i);
> +    }
> +  }

It's unsafe to delete elements while traversing with a iterator.
After discussion with S.C., we decided to keep original method.

@@ +383,5 @@
> +MulticastDNSDeviceProvider::ClearDevices()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  for (auto& device : mDevices) {

keep unchanged, too.

	Gary Chen [:xeonchen] Assignee
	Comment 26 • 9 years ago

main changes:
* using |host| as storing key instead of |serviceName|.
* move device manipulation code into |XxxDevice|
* add test for update device

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 27 • 9 years ago

Comment on attachment 8664622 [details] [diff] [review]
[v2] Part-4-clear-discovered-devices-when-re-.patch

Review of attachment 8664622 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for fiddling with the array operation many times, hope this is the last review round.

::: dom/presentation/PresentationDeviceManager.cpp
@@ +154,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // Bug 1194049: some providers may discontinue discovery after timeout.
> +  // Call |ForceDiscovery()| here to make sure device lists are updated.
> +  ForceDiscovery();

Dispatch a task to trigger |ForceDiscovery| on main thread. It'll prevent the cached device list being modified before function return. We don't want |getAvailableDevices| being blocked by |forceDiscovery|.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +287,5 @@
> +  if (NS_SUCCEEDED(GetListener(getter_AddRefs(listener))) && listener) {
> +    unused << listener->AddDevice(device);
> +  }
> +
> +  mDevices.AppendElement(Device{ PromiseFlatCString(aHost), DeviceState::eActive });

1. why you need a PromiseFlatCString here? Is it just for the log output?
2. use Device(...) instead of Device{...}.

@@ +301,5 @@
> +                                         const uint16_t aPort)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mPresentationServer);
> +  MOZ_ASSERT(0 <= aIndex && aIndex < mDevices.Length());

We want to protect release build as well, use:

if (NS_WARN_IF(aIndex >= mDevices.Length())) {
  return NS_ERROR_INVALID_ARG;
}

@@ +316,5 @@
> +                                           getter_AddRefs(device))))) {
> +    return rv;
> +  }
> +
> +  // don't have to update listener here because no public attribute was changed.

Do we need to notify device manager if service name is changed?

@@ +328,5 @@
> +MulticastDNSDeviceProvider::RemoveDevice(const uint32_t aIndex)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mPresentationServer);
> +  MOZ_ASSERT(0 <= aIndex && aIndex < mDevices.Length());

ditto.

@@ +355,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +MulticastDNSDeviceProvider::FindDevice(const nsACString& aId,

Just realized that we can leverage |nsTArray::IndexOf| instead of writing our own find function.

class DeviceIdComparator {
public:
  bool Equals(const Device& aA, const Device& aB) const {
    return aA.Id == aB.Id;
  }
}

and use |size_t index = mDevices.IndexOf(Device(aId, DeviceStat::eUnknown), DeviceIdComparator())| for finding the index of device we want and use |index == mDevices::NoIndex| to check if device exists.

@@ +513,5 @@
>  {
>    LOG_I("OnDiscoveryStopped");
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // notify for removed devices

this comment is redundant. |ClearUnknownDevices| is self-explained.

@@ +579,4 @@
>    }
>  
> +  uint32_t index;
> +  if (NS_FAILED(FindDevice(host, DeviceState::eActive, index))) {

You should remove the device in unknown state here as well, in this case, you don't need the second argument of |FindDevice| and the code will be simpler.

@@ +712,5 @@
>  
> +  uint32_t index;
> +  if (NS_SUCCEEDED(FindDevice(host, DeviceState::eUnknown, index)) ||
> +      NS_SUCCEEDED(FindDevice(host, DeviceState::eActive, index))) {
> +    if (NS_WARN_IF(NS_FAILED(rv = UpdateDevice(index,

return UpdateDevice(...);

@@ +724,3 @@
>    }
>  
> +  if (NS_WARN_IF(NS_FAILED(rv = AddDevice(serviceName,

else {
  return AddDevice(...);
}

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +457,5 @@
> +function updateDevice() {
> +  Services.prefs.setBoolPref(PREF_DISCOVERY, true);
> +
> +  let mockDevice1 = createDevice("A.local", 12345, "N1", "_mozilla_papi._tcp");
> +  let mockDevice2 = createDevice("A.local", 23456, "N2", "_mozilla_papi._tcp");

|mockDevice2| doesn't seem to be used in the test procedure. You can consider remove it.

@@ +499,5 @@
> +  };
> +
> +  let contractHook = new ContractHook(SD_CONTRACT_ID, mockObj);
> +  let provider = Cc[PROVIDER_CONTRACT_ID].createInstance(Ci.nsIPresentationDeviceProvider);
> +  let listener = new TestPresentationDeviceListener();

shouldn't use the TestPresentationDeviceListener here because we want to explicitly check the |updateDevice| is invoked in this case.

	Gary Chen [:xeonchen] Assignee
	Comment 28 • 9 years ago

Comment on attachment 8664622 [details] [diff] [review]
[v2] Part-4-clear-discovered-devices-when-re-.patch

Review of attachment 8664622 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +316,5 @@
> +                                           getter_AddRefs(device))))) {
> +    return rv;
> +  }
> +
> +  // don't have to update listener here because no public attribute was changed.

Yes, I forgot to change this while changing behavior.

@@ +579,4 @@
>    }
>  
> +  uint32_t index;
> +  if (NS_FAILED(FindDevice(host, DeviceState::eActive, index))) {

But if the device state is unknown, it means listeners will receive a REMOVE event without an ADD event.

	Gary Chen [:xeonchen] Assignee
	Comment 29 • 9 years ago

Patch-diff to save reviewer's time.

	Gary Chen [:xeonchen] Assignee
	Comment 30 • 9 years ago

See Attachment 8665310 [details] [diff] for more detail.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 31 • 9 years ago

(In reply to Gary Chen [:xeonchen] from comment #28)
> @@ +579,4 @@
> >    }
> >  
> > +  uint32_t index;
> > +  if (NS_FAILED(FindDevice(host, DeviceState::eActive, index))) {
> 
> But if the device state is unknown, it means listeners will receive a REMOVE
> event without an ADD event.

Those unknown devices are in active list but have confirm the existence during the ongoing round of discovery, which means we've report "add" event for this device before. If we receive the service lost on an unknown device, it means the device proactively report its absence and we should report "remove" event immediately.

	Gary Chen [:xeonchen] Assignee
	Comment 32 • 9 years ago

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #31)
> Those unknown devices are in active list but have confirm the existence
> during the ongoing round of discovery, which means we've report "add" event
> for this device before. If we receive the service lost on an unknown device,
> it means the device proactively report its absence and we should report
> "remove" event immediately.

Yes, you are right.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 33 • 9 years ago

Comment on attachment 8665311 [details] [diff] [review]
[v3] Part-4-clear-discovered-devices-when-re-.patch

Review of attachment 8665311 [details] [diff] [review]:
-----------------------------------------------------------------

Great work! Only one little bit of modification before landing. Thanks a lot @xeonchen!

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +363,5 @@
> +  mDevices.RemoveElementAt(aIndex);
> +  return NS_OK;
> +}
> +
> +nsresult

I'll prefer to use |bool| instead of error code.

	Gary Chen [:xeonchen] Assignee
	Comment 34 • 9 years ago

follow suggestion, carry r+

	Gary Chen [:xeonchen] Assignee
	Comment 35 • 9 years ago

add missing semicolon, carry r+.

	Gary Chen [:xeonchen] Assignee
	Comment 36 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cd926e1e2c1

	Pulsebot
	Comment 37 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/84390dd84a9b
https://hg.mozilla.org/integration/fx-team/rev/d31b14da65f1
https://hg.mozilla.org/integration/fx-team/rev/2ce21fab2cb9
https://hg.mozilla.org/integration/fx-team/rev/1c8e8329247c

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 38 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/84390dd84a9b
https://hg.mozilla.org/mozilla-central/rev/d31b14da65f1
https://hg.mozilla.org/mozilla-central/rev/2ce21fab2cb9
https://hg.mozilla.org/mozilla-central/rev/1c8e8329247c

	Sylvestre Ledru [:Sylvestre]
	Comment 39 • 9 years ago

Gary, I think this might be too risky for 42 as we are after middle cycle.
However, we probably want this in 43, could you fill the uplift request? Thanks

	Gary Chen [:xeonchen] Assignee
	Comment 40 • 9 years ago

Comment on attachment 8658553 [details] [diff] [review]
Part-1-add-thread-assertion-r-schien.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: power consumption will be increased
[Describe test coverage new/current, TreeHerder]: TreeHerder
[Risks and why]: this should only affect mDNS discovery if anything goes wrong.
[String/UUID change made/needed]:

	Gary Chen [:xeonchen] Assignee
	Comment 41 • 9 years ago

Comment on attachment 8668290 [details] [diff] [review]
[v3] Part-2-add-discovery-timeout-support-r-s.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: power consumption will be increased
[Describe test coverage new/current, TreeHerder]: TreeHerder
[Risks and why]: this should only affect mDNS discovery if anything goes wrong.
[String/UUID change made/needed]:

	Gary Chen [:xeonchen] Assignee
	Comment 42 • 9 years ago

Comment on attachment 8662232 [details] [diff] [review]
Part-3-support-device-update-r-fabrice.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: power consumption will be increased
[Describe test coverage new/current, TreeHerder]: TreeHerder
[Risks and why]: this should only affect mDNS discovery if anything goes wrong.
[String/UUID change made/needed]:

	Gary Chen [:xeonchen] Assignee
	Comment 43 • 9 years ago

Comment on attachment 8667784 [details] [diff] [review]
[v4] Part-4-clear-discovered-devices-when-re-.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: power consumption will be increased
[Describe test coverage new/current, TreeHerder]: TreeHerder
[Risks and why]: this should only affect mDNS discovery if anything goes wrong.
[String/UUID change made/needed]:

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 44 • 9 years ago

Comment on attachment 8658553 [details] [diff] [review]
Part-1-add-thread-assertion-r-schien.patch

Worth a try to uplift to aurora to improve battery life. Please uplift parts 1-4.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 45 • 9 years ago

Comment on attachment 8662232 [details] [diff] [review]
Part-3-support-device-update-r-fabrice.patch

OK to uplift to aurora.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 46 • 9 years ago

Comment on attachment 8667784 [details] [diff] [review]
[v4] Part-4-clear-discovered-devices-when-re-.patch

OK to uplift to aurora.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 47 • 9 years ago

Comment on attachment 8668290 [details] [diff] [review]
[v3] Part-2-add-discovery-timeout-support-r-s.patch

OK to uplift to aurora.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 48 • 9 years ago

Gary, how can we test and verify this once it lands? Thanks.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 49 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc00f3a6a496
https://hg.mozilla.org/releases/mozilla-aurora/rev/a59aa67a7ba6
https://hg.mozilla.org/releases/mozilla-aurora/rev/dfd7ec48d4fe
https://hg.mozilla.org/releases/mozilla-aurora/rev/9a761fd458be

	Gary Chen [:xeonchen] Assignee
	Comment 50 • 9 years ago

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #48)
> Gary, how can we test and verify this once it lands? Thanks.

Per comment 22 and comment 23, all we can do is use it and check battery usage, and that's also what I did.

	Carsten Book [:Tomcat]
	Comment 51 • 9 years ago

Sorry had to backout and recheckin this since this turned innocent for a memory leaks, sorry for the hassle

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 52 • 9 years ago

Gary, this is apparently really hurting people on 42. Can we consider adding the fixes there, or just disabling this code on 42?

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 53 • 9 years ago

From bug 1215012, it looks like there could be issues on Fx43 (where this is fixed). Battery usage is still shown as high.

	Gary Chen [:xeonchen] Assignee
	Comment 54 • 9 years ago

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #52)
> Gary, this is apparently really hurting people on 42. Can we consider adding
> the fixes there, or just disabling this code on 42?

In comment 39 Sylvestre mentioned that's a little bit risky to merge this into 42.
I'd suggest to disable this on 42 because it looks really easy to reproduce.

SC, do you have any concern?

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
	Comment 55 • 9 years ago

I took a closer look at MDNS initialization path on Fennec. The NsdManager is created at GeckoApplication::OnCreate() stage. However, on Firefox OS the behavior is different: the mdnsd daemon will not be enabled until any registration/discovery request is made.
That's why changing dom.presentation.discovery.enabled works for Firefox OS but not on Fennec.

I'll suggest that we can fix this issue first with lazy initializing NsDManager and default disable dom.presentation.discovery.enabled.

@mfinkle, does this sound like a plan?

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 56 • 9 years ago

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #55)
> I took a closer look at MDNS initialization path on Fennec. The NsdManager
> is created at GeckoApplication::OnCreate() stage. However, on Firefox OS the
> behavior is different: the mdnsd daemon will not be enabled until any
> registration/discovery request is made.
> That's why changing dom.presentation.discovery.enabled works for Firefox OS
> but not on Fennec.
> 
> I'll suggest that we can fix this issue first with lazy initializing
> NsDManager and default disable dom.presentation.discovery.enabled.
> 
> @mfinkle, does this sound like a plan?

Yes. That sounds like a good approach for now, when we are not really using the system. We can look into even better NsdManager management in the future.

	Nad A
	Comment 57 • 9 years ago

Just update here as well, I have disabled dom.presentation.discovery.enabled in about:config and the process / deamon still runs for me.