1199564 - start/stop mDNS service on demand

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Gary Chen [:xeonchen]]

In https://bugzilla.mozilla.org/show_bug.cgi?id=1196884#c6, it mentioned that mdnsd is still launched.
The reason is |nsDNSServiceDiscovery| always keep mdnsd alive, so we should launch mdnsd only when necessary.

To fix this, mdnsd should be started/stopped on demand.

Also make sure resources are not leaked if the service is stopped while mDNS resolution requests are still waiting for response.

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: start stop mDNS on demand

Comment 2

[Gary Chen [:xeonchen]]

Comment on attachment 8656983 [details] [diff] [review]
start stop mDNS on demand

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

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +38,5 @@
>    property_set("ctl.stop", "mdnsd");
>  }
>  
> +static uint32_t sUseCount = 0;
> +

suggest to keep it private in the class, and make sure its thread-safety, e.g. use mozilla::Atomic

@@ +44,5 @@
> +{
> +protected:
> +  RequestBase()
> +  {
> +    if (sUseCount++) {

inverse this condition?

@@ +62,2 @@
>  class DiscoveryRequest final : public nsICancelable
> +                             , public RequestBase

Is it required to be public?

@@ +96,5 @@
>    return NS_OK;
>  }
>  
>  class RegisterRequest final : public nsICancelable
> +                            , public RequestBase

ditto

Comment 3

[Kershaw Chang [:kershaw]]

(In reply to Gary Chen [:xeonchen] from comment #2)
> Comment on attachment 8656983 [details] [diff] [review]
> start stop mDNS on demand
> 
> Review of attachment 8656983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
> @@ +38,5 @@
> >    property_set("ctl.stop", "mdnsd");
> >  }
> >  
> > +static uint32_t sUseCount = 0;
> > +
> 
> suggest to keep it private in the class, and make sure its thread-safety,
> e.g. use mozilla::Atomic
> 
Thanks. It's a good suggestion.

> @@ +44,5 @@
> > +{
> > +protected:
> > +  RequestBase()
> > +  {
> > +    if (sUseCount++) {
> 
> inverse this condition?
> 
Yes, you are right.

> @@ +62,2 @@
> >  class DiscoveryRequest final : public nsICancelable
> > +                             , public RequestBase
> 
> Is it required to be public?
> 
No, it can also be private.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: start stop mDNS on demand

Hi Patrick,

Could you please take a look at this patch?

Thanks.

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8657006 [details] [diff] [review]
start stop mDNS on demand

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

what's the threading model here that means you need the atomic?

Comment 6

[Kershaw Chang [:kershaw]]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8657006 [details] [diff] [review]
> start stop mDNS on demand
> 
> Review of attachment 8657006 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> what's the threading model here that means you need the atomic?

Thanks for reviewing.

Actually, it's should be main thread only. I think we don't need atomic here. An unsigned integer and MOZ_ASSERT(NS_IsMainThread()) should be enough.
I'll update the patch later.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: start stop mDNS on demand - v2

Summary of changes:
1. Change the type to static uint32_t
2. Add MOZ_ASSERT(NS_IsMainThread()) in constructor and destructor

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: start stop mDNS on demand, r=mcmanus

Carry reviewer's name.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cdff7699f6b

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: start stop mDNS on demand - v3

Hi Patrick,

Sorry that I have to ask you to review again. I forgot to move creating request earlier than calling operator function. Without this, mdns service will never have a chance to start.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Interdiff of v3 and v2

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: start stop mDNS on demand, r=mcmanus

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/8d707bceab14

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8d707bceab14