1115480 - [Presentation WebAPI] built-in device discovery and control protocol

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

We need a built-in device protocol implementation for Firefox and Firefox OS. Per discussion during Portland workweek, we'll use mDNS/DNS-SD for device discovery and a plain TCP socket as control channel.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

We are going to use the mDNS JS module in bug 1011340.

Comment 2

[Gary Chen [:xeonchen]]

Attachment: part 1, import-mDNS-JS-module-in-bug-1011340.patch

[WIP] this is imported from Bug 1011340, and fix some error.

Comment 3

[Gary Chen [:xeonchen]]

Attachment: part 2, add-mDNS-provider.patch

[WIP] add a mDNS XPCOM component.

Comment 4

[Gary Chen [:xeonchen]]

Attachment: [test-only] modify-to-test-in-browser.patch

[WIP] use this patch to test mDNS provider in browser

Comment 5

[Gary Chen [:xeonchen]]

There's lots of details about mDNS & device discovery, and the MulticastDNS.jsm only implements basic functions of discovery.

To support more features, we decide to use mDNSResponder, the mDNS/DNS-SD implementation by Apple, instead of extending MulticastDNS.jsm since the specs (RFC-1035, RFC-6762, RFC-6763) contain a lot of works.

Our first step is to bring up mDNSResponder on b2g devices, make sure it works, then implement the module on other platforms (OSX, Linux, Windows, Android) later.

Comment 6

[Gary Chen [:xeonchen]]

Attachment: implement-nsIPresentationDeviceProvider-with-mDNS.patch

update previous work on mDNS response.

Comment 7

[Gary Chen [:xeonchen]]

Attachment: add MulticastDNSProvider

[WIP] use mDNSReponder to get discovery responses.
However, the client-server domain socket model seems not working on B2G (got kDNSServiceErr_ServiceNotRunning) by running /system/bin/mdnsd.
Using the embedded model in [1] is the next step.

[1] http://www.opensource.apple.com/source/mDNSResponder/mDNSResponder-561.1.1/mDNSPosix/ReadMe.txt

Comment 8

[Gary Chen [:xeonchen]]

(In reply to Liang-Heng Chen [:xeonchen] from comment #7)
> Created attachment 8554397 [details] [diff] [review]
> add MulticastDNSProvider
> 
> [WIP] use mDNSReponder to get discovery responses.
> However, the client-server domain socket model seems not working on B2G (got
> kDNSServiceErr_ServiceNotRunning) by running /system/bin/mdnsd.
> Using the embedded model in [1] is the next step.
> 
> [1]
> http://www.opensource.apple.com/source/mDNSResponder/mDNSResponder-561.1.1/
> mDNSPosix/ReadMe.txt

Finally I got mdnsd running by removing "disabled" and "oneshot" in the mdnsd segment of $B2G/system/core/rootdir/init.rc, therefore embedded model will be dropped.

Comment 9

[Gary Chen [:xeonchen]]

Attachment: 0001-add-MulticastDNSProvider.patch

[WIP] support Browse device via mDNS protocol using mDNSResponder.

Comment 10

[Gary Chen [:xeonchen]]

Attachment: 0001-enable-mDNS-daemon.patch

This patch launches /system/bin/mdnsd at boot time, which is originally launched on demand via NsdManager[1] in Android.
We don't have an interface to communicate with NsdManager because it's Java-based.

I'm not sure if it's a good idea to modify init.rc (external repository in [2]), would you give some advices?

[1] http://developer.android.com/reference/android/net/nsd/NsdManager.html
[2] git://codeaurora.org/platform/system/core

Comment 11

[[:fabrice] Fabrice Desré]

Sure we can modify init.rc 
The one you patched already has gonk changes.

Comment 12

[Gary Chen [:xeonchen]]

Attachment: 0001-add-mDNSResponder-XPCOM-module.patch

PCOM module of mDNSResponder

Comment 13

[Gary Chen [:xeonchen]]

Attachment: mDNS-provider

Comment 14

[Gary Chen [:xeonchen]]

Attachment: mDNSResponder XPCOM module

Comment 15

[Gary Chen [:xeonchen]]

Attachment: mDNSResponder XPCOM module

[WIP] feature complete, going to write test cases.

Comment 16

[Gary Chen [:xeonchen]]

(In reply to Liang-Heng Chen [:xeonchen] from comment #15)
> Created attachment 8562575 [details] [diff] [review]
> mDNSResponder XPCOM module
> 
> [WIP] feature complete, going to write test cases.

I found that it is difficult to test this module since it's tight coupling with DNSService*() APIs defined in dns_sd.h provided by mDNSResponder.

Fabrice, do we have any conventional way to test such modules, or do you have any suggestion to refine the design?

Comment 17

[Gary Chen [:xeonchen]]

Attachment: add mDNSResponder XPCOM module

This patch creates an XPCOM module of mDNS implemetation using Android built-in mDNSResponder.

TXT records are not supported yet, but it's not a necessary part for our Presentation API implementation. Maybe we should create another bug to implement this feature in the future.

This patch also obsoletes previous modification of init.rc since it can be launched via |property_set| API.

Comment 18

[Gary Chen [:xeonchen]]

Comment on attachment 8563297 [details] [diff] [review]
add mDNSResponder XPCOM module

This patch doesn't pass on some Desktop and Emulator platform, I'll fix it.

Comment 19

[Gary Chen [:xeonchen]]

Attachment: add mDNSResponder XPCOM module

This patch creates an XPCOM module of mDNS implemetation using Android built-in mDNSResponder.

TXT records are not supported yet since it's not a necessary part for our Presentation API implementation. Maybe we should create another bug to implement this feature in the future.

This patch also obsoletes previous modification of init.rc since it can be launched via |property_set| API.

Comment 20

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

This patch creates an XPCOM module of mDNS implemetation using Android built-in mDNSResponder.

TXT records are implemented in this patch.

This patch also obsoletes previous modification of init.rc since it can be launched via |property_set| API.

Comment 21

[[:fabrice] Fabrice Desré]

Comment on attachment 8566441 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

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

I have mixed feelings about implementing this feature with platform specific code like that. I feel using a js implementation like the one from Justin at https://github.com/justindarc/dns-sd.js would be easier to support.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> Comment on attachment 8566441 [details] [diff] [review]
> add mDNSResponder XPCOM module
> 
> Review of attachment 8566441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have mixed feelings about implementing this feature with platform specific
> code like that. I feel using a js implementation like the one from Justin at
> https://github.com/justindarc/dns-sd.js would be easier to support.

We'll not be able to bind the mDNS port on platform with built-in mDNS service, e.g. Mac and Android.

Comment 23

[Gary Chen [:xeonchen]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #22)
> (In reply to Fabrice Desré [:fabrice] from comment #21)
> > Comment on attachment 8566441 [details] [diff] [review]
> > add mDNSResponder XPCOM module
> > 
> > Review of attachment 8566441 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I have mixed feelings about implementing this feature with platform specific
> > code like that. I feel using a js implementation like the one from Justin at
> > https://github.com/justindarc/dns-sd.js would be easier to support.
> 
> We'll not be able to bind the mDNS port on platform with built-in mDNS
> service, e.g. Mac and Android.

For the case S.C. mentioned, using mDNSResponder might be the only choice.

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Liang-Heng Chen [:xeonchen] from comment #23)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #22)
> > (In reply to Fabrice Desré [:fabrice] from comment #21)
> > > Comment on attachment 8566441 [details] [diff] [review]
> > > add mDNSResponder XPCOM module
> > > 
> > > Review of attachment 8566441 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I have mixed feelings about implementing this feature with platform specific
> > > code like that. I feel using a js implementation like the one from Justin at
> > > https://github.com/justindarc/dns-sd.js would be easier to support.
> > 
> > We'll not be able to bind the mDNS port on platform with built-in mDNS
> > service, e.g. Mac and Android.
> 
> For the case S.C. mentioned, using mDNSResponder might be the only choice.

Yeah, that's unfortunate... So the implementation you have now is gonk only, right? Or can it also be used on fennec? If it's gonk only, I'd still rather use a js implementation for gonk and other platforms that don't have a default mDNS service.

Comment 25

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> (In reply to Liang-Heng Chen [:xeonchen] from comment #23)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #22)
> > > (In reply to Fabrice Desré [:fabrice] from comment #21)
> > > > Comment on attachment 8566441 [details] [diff] [review]
> > > > add mDNSResponder XPCOM module
> > > > 
> > > > Review of attachment 8566441 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > I have mixed feelings about implementing this feature with platform specific
> > > > code like that. I feel using a js implementation like the one from Justin at
> > > > https://github.com/justindarc/dns-sd.js would be easier to support.
> > > 
> > > We'll not be able to bind the mDNS port on platform with built-in mDNS
> > > service, e.g. Mac and Android.
> > 
> > For the case S.C. mentioned, using mDNSResponder might be the only choice.
> 
> Yeah, that's unfortunate... So the implementation you have now is gonk only,
> right? Or can it also be used on fennec? If it's gonk only, I'd still rather
> use a js implementation for gonk and other platforms that don't have a
> default mDNS service.

This is for gonk only. For fennec we'll need to use NsdManager in Android framework. mDNSResponder library also supports multiple platform and it has been widely used, which means all the protocol features and protocol compatibility issues has been resolved. We should be able to use it on Windows/Linux with slightly modification on library linking model. Is there any other strong reason to use JS library here?

@xeonchen, please also take a look at dns-sd.js to see if it has all the feature we need, e.g. TXT record.

Comment 26

[Gary Chen [:xeonchen]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #25)
> @xeonchen, please also take a look at dns-sd.js to see if it has all the
> feature we need, e.g. TXT record.

TXT record seems not working.
In https://github.com/justindarc/dns-sd.js/blob/master/src/dns-sd.js#L193, the TXT related code is commented out, and I didn't see any log about it.
But TXT is for extensibility and we don't actually use it currently.

For simple registration/discovery, it works.
However, the responses by mDNSResponder and dns-sd.js are a little bit different by observing using Wireshark.
I'm not sure whether the differences affect compatibility or not, but some of my discovery clients (mdns-scan, avahi-discover) cannot display the service registered by dns-sd.js well.

Comment 27

[[:fabrice] Fabrice Desré]

Ok, that makes me a bit sad but let's move on with the mDNSResponder library.

Comment 28

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

Comment 29

[Gary Chen [:xeonchen]]

Comment on attachment 8572452 [details] [diff] [review]
Part 2: implement mDNS device provider

I'll make some modifications on this patch

Comment 30

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

Comment 31

[[:fabrice] Fabrice Desré]

Shih-Chiang, do you mind doing the reviews?

Comment 32

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Yeah I can do that, you want to transfer the r? to me?

Comment 33

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

Fix code style, put function return type to single line

Comment 34

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

fix try server errors

Comment 35

[Gary Chen [:xeonchen]]

Hi Steve, I discussed with you the other day about the Hazard Analysis Build failure.
My build criteria is: (in moz.build of Attachment #8577052 [details] [diff])
> if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] >= '16':
>   DIRS += ['gonk']
and the failure platform is B2G Desktop.

So I don't think it should be built.
Do you have any idea?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56

Comment 36

[Steve Fink [:sfink] [:s:]]

(In reply to Liang-Heng Chen [:xeonchen] from comment #35)
> Hi Steve, I discussed with you the other day about the Hazard Analysis Build
> failure.
> My build criteria is: (in moz.build of Attachment #8577052 [details] [diff] [diff])
> > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] >= '16':
> >   DIRS += ['gonk']
> and the failure platform is B2G Desktop.
> 
> So I don't think it should be built.
> Do you have any idea?
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56

Ok, I'm going to need some help with this. Despite creating the b2g hazard build, I still have only a very basic understanding of the b2g build system.

In the hazard build, I see it trying to create libxul.so with -lmdnssd, and it can't find the mdnssd library.

In http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-efe6a4feee56/try-linux64_gecko/try-linux64_gecko-bm76-try1-build735.txt.gz (for example), then when it creates libxul.so it does *not* have -lmdnssd. Is that expected? You said "So I don't think it should be built." What configurations should and should not build with this?

In general, I like the hazard build to build as much code as possible. Technically, it doesn't even need to link, though that's probably a useful sanity check and it's hard to suppress the link anyway. But in particular, the hazard build *needs* to compile any code that could conceivably use the JSAPI.

The hazard build currently just does |build.sh gecko|, except it also passes a custom mozconfig that looks like

  . gonk-misc/default-gecko-config
  ac_add_options --with-compiler-wrapper=...
  ac_add_options --without-ccache

Er... I don't even understand how that selects a device/whatever to build for, though in the logs I do see

  device/generic/armv7-a-neon/vendorsetup.sh

Oh. I see in http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-efe6a4feee56/try-emulator-kk-debug/b2g_try_emulator-kk-debug_dep-bm75-try1-build681.txt.gz that it *does* pass in -lmdnssd. So I guess it's not supposed to link in on desktop? Which seems to say that the problem is that |./build.sh gecko| is linking it in when it shouldn't be?

Sorry for being so clueless here.

Comment 37

[Gary Chen [:xeonchen]]

(In reply to Steve Fink [:sfink, :s:] from comment #36)
> (In reply to Liang-Heng Chen [:xeonchen] from comment #35)
> > Hi Steve, I discussed with you the other day about the Hazard Analysis Build
> > failure.
> > My build criteria is: (in moz.build of Attachment #8577052 [details] [diff] [diff] [diff])
> > > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] >= '16':
> > >   DIRS += ['gonk']
> > and the failure platform is B2G Desktop.
> > 
> > So I don't think it should be built.
> > Do you have any idea?
> > 
> > [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56
> 
> Ok, I'm going to need some help with this. Despite creating the b2g hazard
> build, I still have only a very basic understanding of the b2g build system.
> 
> In the hazard build, I see it trying to create libxul.so with -lmdnssd, and
> it can't find the mdnssd library.
> 
> In
> http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-
> efe6a4feee56/try-linux64_gecko/try-linux64_gecko-bm76-try1-build735.txt.gz
> (for example), then when it creates libxul.so it does *not* have -lmdnssd.
> Is that expected? You said "So I don't think it should be built." What
> configurations should and should not build with this?
> 
> In general, I like the hazard build to build as much code as possible.
> Technically, it doesn't even need to link, though that's probably a useful
> sanity check and it's hard to suppress the link anyway. But in particular,
> the hazard build *needs* to compile any code that could conceivably use the
> JSAPI.
> 
> The hazard build currently just does |build.sh gecko|, except it also passes
> a custom mozconfig that looks like
> 
>   . gonk-misc/default-gecko-config
>   ac_add_options --with-compiler-wrapper=...
>   ac_add_options --without-ccache
> 
> Er... I don't even understand how that selects a device/whatever to build
> for, though in the logs I do see
> 
>   device/generic/armv7-a-neon/vendorsetup.sh
> 
> Oh. I see in
> http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-
> efe6a4feee56/try-emulator-kk-debug/b2g_try_emulator-kk-debug_dep-bm75-try1-
> build681.txt.gz that it *does* pass in -lmdnssd. So I guess it's not
> supposed to link in on desktop? Which seems to say that the problem is that
> |./build.sh gecko| is linking it in when it shouldn't be?
> 
> Sorry for being so clueless here.

Sorry I didn't clarify the question clearly.

The sub-directory named 'gonk' is expected to be built only for some platforms, and

  CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk'

defines the criteria.

And that's why it didn't have -lmdnssd in [1], because it's 'B2G Desktop Linux x64 opt'.

I don't know why it tries to build the sub-directory 'gonk' in the hazard build since it doesn't in the normal build.

If it have to build the 'gonk' directory for some reason, I think it is supposed to provide the mdnssd library.
Actually, if mdnssd library is provided, I'll be happy to build it on all platforms.

[1] http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-efe6a4feee56/try-linux64_gecko/try-linux64_gecko-bm76-try1-build735.txt.gz

Comment 38

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8577055 [details] [diff] [review]
Part 2: implement mDNS device provider

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

Sorry for the late response. Haven't looked at the detailed log but I think we can do that after the architecture/code clean up.

::: b2g/installer/package-manifest.in
@@ +266,5 @@
>  @RESPATH@/components/layout_xul_tree.xpt
>  @RESPATH@/components/layout_xul.xpt
>  @RESPATH@/components/locale.xpt
>  @RESPATH@/components/lwbrk.xpt
> +@RESPATH@/components/mdnsresponder.xpt

this change should belongs to patch part 1, right?

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +12,5 @@
> +
> +#ifdef LOG
> +#undef LOG
> +#endif
> +#define LOG printf_stderr

Use NSPR Logging mechanism.

@@ +16,5 @@
> +#define LOG printf_stderr
> +
> +#define SERVICE_TYPE "_mozilla_papi._tcp"
> +
> +using namespace mozilla::dom::presentation;

avoid |using namespace|.

@@ +18,5 @@
> +#define SERVICE_TYPE "_mozilla_papi._tcp"
> +
> +using namespace mozilla::dom::presentation;
> +
> +class nsIPresentationDevice;

nit: remove this because we already include nsIPresentationDevice.h

@@ +23,5 @@
> +
> +/**
> + * This wrapper is used to break circular-reference problem.
> + */
> +class mozilla::dom::presentation::DNSServiceWrappedListener

use NS_FORWARD_SAFE_XXX technique to reduce hand-crafting proxy object.

@@ +47,5 @@
> +private:
> +  nsIDNSServiceDiscoveryListener* mServiceDiscoveryListener;
> +  nsIDNSRegistrationListener* mRegistrationListener;
> +  nsIDNSServiceResolveListener* mServiceResolveListener;
> +  nsITCPPresentationServerListener* mPresentationServerListener;

MulticastDNSDeviceProvider and DNSSErviceWrappedListener should be 1-to-1 mapping, right? We should be able to use only one member for storing the relationship.

@@ +117,5 @@
> +  mInitialized = true;
> +  return NS_OK;
> +}
> +
> +nsresult

change the return value to "void" because we don't really use it.

@@ +136,5 @@
> +    nsCOMPtr<nsIDNSServiceDiscoveryListener> listener(do_QueryObject(mWrappedListener, &rv));
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +    if (NS_WARN_IF(NS_FAILED(rv = mDiscovery->StopDiscovery(listener)))) {

Can we use |mDiscovery->StopDiscovery(mWrappedListener)| directly? So does other do_QueryObject code in this file.

@@ +170,5 @@
> +MulticastDNSDeviceProvider::RegisterService(uint32_t port)
> +{
> +  nsresult rv;
> +
> +  mRegistration = do_CreateInstance("@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1", &rv);

Is there any possible to create only one instance of nsDNSServiceDiscovery for both registration and discovery? In addition, should we use do_GetService instead of do_CreateInstance because mdnsresponder should be a singleton?

@@ +457,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMPL_ISUPPORTS(DNSServiceWrappedListener,
> +  nsIDNSServiceDiscoveryListener,

nit: should align with DNSServiceWrappedListener

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: suggest to add vim/emacs format tag above, like in https://dxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1 (other new C++ files, too)

@@ +10,5 @@
> +
> +#include "nsIDNSServiceDiscovery.h"
> +#include "nsWeakPtr.h"
> +#include "nsIPresentationDeviceProvider.h"
> +#include "nsITCPPresentationServer.h"

nit: sort header in dictionary order.

@@ +12,5 @@
> +#include "nsWeakPtr.h"
> +#include "nsIPresentationDeviceProvider.h"
> +#include "nsITCPPresentationServer.h"
> +
> +#define MULTICAST_DNS_PROVIDER_CID_STR "814f947a-52f7-41c9-94a1-3684797284ac"

nit: remove unused define

@@ +17,5 @@
> +#define MULTICAST_DNS_PROVIDER_CID \
> +  {0x814f947a, 0x52f7, 0x41c9, \
> +    { 0x94, 0xa1, 0x36, 0x84, 0x79, 0x72, 0x84, 0xac }}
> +
> +#define MULTICAST_DNS_PROVIDER_CONTRACT_ID "@mozilla.org/dom/presentation/multicastdns-provider;1"

We don't expect CID and CONTRACT_ID will be used out side of MulticastDNSProviderModule.cpp, right? If so, we should move those define to MuldicastDNSProviderModule.cpp.

@@ +26,5 @@
> +
> +class DNSServiceWrappedListener;
> +class MulticastDNSService;
> +
> +class MulticastDNSDeviceProvider

Add MOZ_FINAL if we don't expect people to extend this class.

@@ +43,5 @@
> +  NS_DECL_NSITCPPRESENTATIONSERVERLISTENER
> +
> +  MulticastDNSDeviceProvider();
> +  virtual nsresult Init();
> +  virtual nsresult Uninit();

why virtual?

::: dom/presentation/provider/MulticastDNSProviderModule.cpp
@@ +4,5 @@
> +
> +#include "MulticastDNSDeviceProvider.h"
> +#include "mozilla/ModuleUtils.h"
> +
> +using namespace mozilla::dom::presentation;

avoid using namespace, explicitly list all the name you want like:

using mozilla::dom::presentation::MulticastDNSDeviceProvider;

@@ +31,5 @@
> +  kMulticastDNSDeviceProviderContracts,
> +  kMulticastDNSDeviceProviderCategories
> +};
> +
> +NSMODULE_DEFN(MulticastDNSDeviceProviderModule) = &kMulticastDNSDeviceProviderModule;

This file might be used by other device provider in the future. Rename to something more general. e.g. PresentationDeviceProviderModule. (File name should be changed as well)

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Use CC0 for test files.

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

@@ +223,5 @@
> +
> +  add_test(registerService);
> +  add_test(addDevice);
> +
> +  infoHook.cleanup();

use head/tail script for test setup/teardown. test framework will guarantee that teardown will be executed even if exception happened in test procedure.

Comment 39

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8577052 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

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

I'd prefer moving all these files to netwerk folder since we are going to introduce a general network feature for future use. In this case, the namespace should change to mozilla::net::mdns as well.

::: config/system-headers
@@ +1358,5 @@
>  unicode/utypes.h
>  #endif
>  libutil.h
>  unwind.h
> +dns_sd.h

move it according to the dictionary order and wrap with corresponding #ifdef block

::: toolkit/components/mdnsresponder/gonk/MDNSResponderMonitor.cpp
@@ +6,5 @@
> +#include <unistd.h>
> +#include <sys/epoll.h>
> +#include "nsThreadUtils.h"
> +
> +using namespace mozilla::toolkit::components::mdnsresponder;

IIRC, "using namespace" is bad for unified build because it will pollute namespace. Use namespace block directly.

namspace mozilla {
namespace toolkit {
namespace components {
namespace mdsnresponder {

::: toolkit/components/mdnsresponder/gonk/MDNSResponderMonitor.h
@@ +21,5 @@
> + * mDNSResponder callbacks are invoked by DNSServiceProcessResult(),
> + * but it blocks until data is readable.
> + * This class utilizes another thread to trigger callback events.
> + */
> +class MDNSResponderMonitor

mark as final.

@@ +33,5 @@
> +
> +private:
> +  MDNSResponderMonitor();
> +  virtual ~MDNSResponderMonitor();
> +  NS_IMETHODIMP Init();

use nsresult directly since it is not a virtual function from any XPIDL interface.

::: toolkit/components/mdnsresponder/gonk/MDNSResponderOperator.cpp
@@ +20,5 @@
> +
> +#ifdef ANDROID
> +#include <android/log.h>
> +#define LOG(aFmt, ...) __android_log_print(ANDROID_LOG_INFO, "MulticastDNS", aFmt, ## __VA_ARGS__);
> +#endif

Use NSPR logging mechanism would be better. https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/Logging

@@ +25,5 @@
> +
> +using namespace mozilla::toolkit::components::mdnsresponder;
> +
> +nsresult
> +CloneService(nsIDNSServiceInfo* src, nsIDNSServiceInfo** obj)

can we use a copy constructor?

::: toolkit/components/mdnsresponder/gonk/MDNSResponderReply.cpp
@@ +27,5 @@
> +
> +NS_IMETHODIMP
> +BrowseReplyRunnable::Run()
> +{
> +  BrowseOperator* obj = reinterpret_cast<BrowseOperator*>(mContext);

should be caller's responsibility to perform type casting. |mContext| should be |BrowseOperator*| instead of |void*|.

::: toolkit/components/mdnsresponder/gonk/MDNSResponderReply.h
@@ +22,5 @@
> +    uint32_t interfaceIndex,
> +    DNSServiceErrorType errorCode,
> +    const char* serviceName,
> +    const char* regType,
> +    const char* replyDomain,

I'll suggest to use Gecko string type if possible.

::: toolkit/components/mdnsresponder/gonk/MDNSService.cpp
@@ +5,5 @@
> +#define MDNS_SERVICE_STATUS "init.svc.mdnsd"
> +#define NAP_TIME 200 // 200 ms between polls
> +
> +static bool
> +check_property(const char* name, const char* desired_value)

nit: coding style, use camel case

@@ +34,5 @@
> +    return NS_ERROR_FAILURE;
> +}
> +
> +nsresult
> +mozilla::toolkit::components::mdnsresponder::StartService()

should use namespace and anonymous namespace.

::: toolkit/components/mdnsresponder/gonk/MDNSService.h
@@ +10,5 @@
> +namespace components {
> +namespace mdnsresponder {
> +
> +nsresult StartService();
> +nsresult StopService();

These functions are only used in nsDNSServiceDiscovery.cpp, can we move them into the anonymous namespace in that file?

::: toolkit/components/mdnsresponder/gonk/nsDNSServiceDiscovery.h
@@ +8,5 @@
> +#include <map>
> +#include "nsRefPtr.h"
> +#include "nsIDNSServiceDiscovery.h"
> +
> +#define DNSSERVICEDISCOVERY_CID_STR "8df43d23-d3f9-4dd5-b965-de2ca3f6a42c"

nit: remove unused define.

@@ +11,5 @@
> +
> +#define DNSSERVICEDISCOVERY_CID_STR "8df43d23-d3f9-4dd5-b965-de2ca3f6a42c"
> +#define DNSSERVICEDISCOVERY_CID \
> +  {0x8df43d23, 0xd3f9, 0x4dd5, \
> +    { 0xb9, 0x65, 0xde, 0x2c, 0xa3, 0xf6, 0xa4, 0x2c }}

move this into nsMulticastDNSResponderModule.cpp

@@ +13,5 @@
> +#define DNSSERVICEDISCOVERY_CID \
> +  {0x8df43d23, 0xd3f9, 0x4dd5, \
> +    { 0xb9, 0x65, 0xde, 0x2c, 0xa3, 0xf6, 0xa4, 0x2c }}
> +
> +#define DNSSERVICEDISCOVERY_CONTRACT_ID "@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1"

move this into nsIDNSServiceDiscovery.idl

@@ +18,5 @@
> +
> +namespace mozilla {
> +namespace toolkit {
> +namespace components {
> +namespace mdnsresponder {

too many namespace hierarchy. mozilla::mdnsresponder should be enough.

::: toolkit/components/mdnsresponder/gonk/nsDNSServiceInfo.cpp
@@ +7,5 @@
> +
> +NS_IMPL_ISUPPORTS(nsDNSServiceInfo, nsIDNSServiceInfo)
> +
> +nsDNSServiceInfo::nsDNSServiceInfo()
> +  : mHost(NS_LITERAL_CSTRING(""))

no need to assign empty string for nsCString

@@ +12,5 @@
> +  , mPort(0)
> +  , mServiceName(NS_LITERAL_CSTRING(""))
> +  , mServiceType(NS_LITERAL_CSTRING(""))
> +  , mDomainName(NS_LITERAL_CSTRING(""))
> +  , mAttributes(nullptr)

no need to assign nullptr for nsCOMPtr.

@@ +129,5 @@
> +  nsCOMPtr<nsIProperties> attributes(mAttributes);
> +  attributes.forget(aAttributes);
> +  return NS_OK;
> +}
> +NS_IMETHODIMP

nit: add empty line above

::: toolkit/components/mdnsresponder/gonk/nsDNSServiceInfo.h
@@ +7,5 @@
> +
> +#include "nsCOMPtr.h"
> +#include "nsIDNSServiceDiscovery.h"
> +
> +#define DNSSERVICEINFO_CID_STR "14a50f2b-7ff6-48a5-88e3-615fd111f5d3"

nit: remove this because it hasn't been used in any place.

@@ +10,5 @@
> +
> +#define DNSSERVICEINFO_CID_STR "14a50f2b-7ff6-48a5-88e3-615fd111f5d3"
> +#define DNSSERVICEINFO_CID \
> +  {0x14a50f2b, 0x7ff6, 0x48a5, \
> +    { 0x88, 0xe3, 0x61, 0x5f, 0xd1, 0x11, 0xf5, 0xd3 }}

move this to nsMulticastDNSResponderModule.cpp since it is only been used there.

@@ +12,5 @@
> +#define DNSSERVICEINFO_CID \
> +  {0x14a50f2b, 0x7ff6, 0x48a5, \
> +    { 0x88, 0xe3, 0x61, 0x5f, 0xd1, 0x11, 0xf5, 0xd3 }}
> +
> +#define DNSSERVICEINFO_CONTRACT_ID "@mozilla.org/toolkit/components/mdnsresponder/dns-info;1"

move this define to nsIDNSServiceDiscovery.idl, then you can use this macro in other place (with nsIDNSServiceDiscovery.h included)

::: toolkit/components/mdnsresponder/nsIDNSServiceDiscovery.idl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +#include "nsIProperties.idl"

replace with |interface nsIProperties;|.

@@ +43,5 @@
> +
> +  /**
> +   * The attributes of the service.
> +   */
> +  attribute nsIProperties attributes;

nsIPropertyBag2 has better interface than nsIProperties.

Comment 40

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@mcmanus, we're going to support MDNS as the device discovery protocol. Do you think it is appropriate to put the related code under netwerk folder?

Comment 41

[Gary Chen [:xeonchen]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #38)
> @@ +117,5 @@
> > +  mInitialized = true;
> > +  return NS_OK;
> > +}
> > +
> > +nsresult
> 
> change the return value to "void" because we don't really use it.

It is used by |SetListener|.

Comment 42

[Patrick McManus [:mcmanus]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #40)
> @mcmanus, we're going to support MDNS as the device discovery protocol. Do
> you think it is appropriate to put the related code under netwerk folder?

awesome! yes please.. under network/dns/mdns probly

Comment 43

[Patrick McManus [:mcmanus]]

or even flat in netwerk/dns - I'm happy to review or sr

Comment 44

[Gary Chen [:xeonchen]]

(In reply to Steve Fink [:sfink, :s:] from comment #36)
> The hazard build currently just does |build.sh gecko|, except it also passes
> a custom mozconfig that looks like
> 
>   . gonk-misc/default-gecko-config
>   ac_add_options --with-compiler-wrapper=...
>   ac_add_options --without-ccache
> 

The link error seems because it *only* does |build.sh gecko| instead of |build.sh|, therefore the libraries in the external/* are not built.
I'm going to find out how to avoid this situation.

Steve, do we have a document to build the hazard analysis process on my computer instead of try server?

Comment 45

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

refinements according to review comments

Comment 46

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8584389 [details] [diff] [review]
Part 2: implement mDNS device provider

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

Overall looks good but need some more modifications.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +6,5 @@
> +#include "MulticastDNSDeviceProvider.h"
> +#include "nsAutoPtr.h"
> +#include "nsComponentManagerUtils.h"
> +#include "nsIPresentationDevice.h"
> +#include "nsISupportsImpl.h"

Do we need to explicitly include nsISupportsImpl.h?

@@ +33,5 @@
> +
> +/**
> + * This wrapper is used to break circular-reference problem.
> + */
> +class DNSServiceWrappedListener

marks as final.

@@ +46,5 @@
> +  NS_FORWARD_SAFE_NSIDNSREGISTRATIONLISTENER(mListener)
> +  NS_FORWARD_SAFE_NSIDNSSERVICERESOLVELISTENER(mListener)
> +  NS_FORWARD_SAFE_NSITCPPRESENTATIONSERVERLISTENER(mListener)
> +
> +  DNSServiceWrappedListener() = default;

mark as explicit.

@@ +57,5 @@
> +
> +private:
> +  virtual ~DNSServiceWrappedListener() = default;
> +
> +private:

unnecessary private label.

@@ +83,5 @@
> +nsresult
> +MulticastDNSDeviceProvider::Init()
> +{
> +  if (mInitialized) {
> +    return NS_ERROR_ALREADY_INITIALIZED;

simply return NS_OK;

@@ +92,5 @@
> +  mMulticastDNS = do_GetService("@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1", &rv);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +  mWrappedListener = new DNSServiceWrappedListener();

nit: need an empty line above, so does other places.

@@ +106,5 @@
> +  }
> +  if (NS_WARN_IF(NS_FAILED(mPresentationServer->SetListener(mWrappedListener)))) {
> +    return rv;
> +  }
> +  if (NS_WARN_IF(NS_FAILED(rv = mPresentationServer->Init(NS_LITERAL_CSTRING(""), 0)))) {

use EmptyCString() instead.

@@ +125,5 @@
> +nsresult
> +MulticastDNSDeviceProvider::Uninit()
> +{
> +  if (!mInitialized) {
> +    return NS_ERROR_NOT_INITIALIZED;

simply return NS_OK?

@@ +137,5 @@
> +  }
> +
> +  if (mMulticastDNS) {
> +    if (NS_WARN_IF(NS_FAILED(rv = mMulticastDNS->StopDiscovery(mWrappedListener)))) {
> +      return rv;

don't need to early return during the uninit.

@@ +149,5 @@
> +  if (mWrappedListener) {
> +    mWrappedListener->SetListener(nullptr);
> +    mWrappedListener = nullptr;
> +  }
> +

forget |mInitialized = false| in this function?

@@ +178,5 @@
> +MulticastDNSDeviceProvider::GetListener(nsIPresentationDeviceListener** aListener)
> +{
> +  if (NS_WARN_IF(!aListener)) {
> +    return NS_ERROR_INVALID_POINTER;
> +  }

NS_ENSURE_ARG_POINTER(aListener);

@@ +203,5 @@
> +  if (!mInitialized && mDeviceListener) {
> +    if (NS_WARN_IF(NS_FAILED(rv = Init()))) {
> +      return rv;
> +    }
> +  }

The logic of this function seems a bit odd to me. Could we simply do uninit if aListener is null and do init otherwise?

@@ +213,5 @@
> +MulticastDNSDeviceProvider::ForceDiscovery()
> +{
> +  if (!mInitialized) {
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }

|MOZ_ASSERT(mInitialized);| should be enough.

@@ +216,5 @@
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +  if (!mMulticastDNS) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

MOZ_ASSERT here, too?

@@ +248,5 @@
> +MulticastDNSDeviceProvider::OnServiceFound(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +252,5 @@
> +  }
> +
> +  nsresult rv ;
> +
> +  if (mMulticastDNS) {

MOZ_ASSERT here?

@@ +266,5 @@
> +MulticastDNSDeviceProvider::OnServiceLost(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +289,5 @@
> +  }
> +
> +  nsCOMPtr<nsIPresentationDevice> device;
> +  if (NS_FAILED(mPresentationServer->GetTCPDevice(serviceName, getter_AddRefs(device)))) {
> +    return NS_OK; // ignore non-exist device;

s/non-exist/non-existing

@@ +312,5 @@
> +
> +NS_IMETHODIMP
> +MulticastDNSDeviceProvider::OnStartDiscoveryFailed(const nsACString& serviceType, int32_t errorCode)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

Do we ever need those unimplemented callbacks? need follow-up bugs?

@@ +327,5 @@
> +MulticastDNSDeviceProvider::OnServiceRegistered(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +368,5 @@
> +MulticastDNSDeviceProvider::OnServiceResolved(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +372,5 @@
> +  }
> +
> +  nsresult rv;
> +
> +  nsAutoCString host;

use nsCString directly since we are going to pass it to CreateTCPDevice, should be able to prevent one string copy.

@@ +400,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIPresentationDeviceListener> listener;
> +  if (NS_WARN_IF(NS_FAILED(rv = GetListener(getter_AddRefs(listener))))) {

GetListener should be free from returning error here, the error checking is redundant.

@@ +405,5 @@
> +    return rv;
> +  }
> +  if (listener) {
> +    if (NS_WARN_IF(NS_FAILED(rv = listener->AddDevice(device)))) {
> +      return rv;

We don't need to propagate error along the callback chain.

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +34,5 @@
> +  NS_DECL_NSIDNSREGISTRATIONLISTENER
> +  NS_DECL_NSIDNSSERVICERESOLVELISTENER
> +  NS_DECL_NSITCPPRESENTATIONSERVERLISTENER
> +
> +  MulticastDNSDeviceProvider() = default;

mark as explicit.

@@ +40,5 @@
> +  nsresult Uninit();
> +
> +private:
> +  virtual ~MulticastDNSDeviceProvider();
> +  nsresult RegisterService(uint32_t port);

nit: argument should be named with "a" prefix, e.g. aPort. All other files should fix this as well.

@@ +42,5 @@
> +private:
> +  virtual ~MulticastDNSDeviceProvider();
> +  nsresult RegisterService(uint32_t port);
> +
> +private:

this label is unnecessary, empty line between private method and attributes should be enough.

Comment 47

[Gary Chen [:xeonchen]]

Attachment: Part 1: Add libmdnssd.so to gecko library dependent list

To avoid link error of hazard build on try server.

Comment 48

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement XPCOM module for mDNSProvider

implement XPCOM module for mDNSProvider

Comment 49

[Gary Chen [:xeonchen]]

Attachment: Part 3: implement mDNS device provider

refinements according to review comments

Comment 50

[Patrick McManus [:mcmanus]]

Comment on attachment 8586624 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

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

This is very nicely developed code. thanks!

I was hoping that this was going to be a cross platform implementation :(.. but it seems that while it is described as being gonk specific, its really (mostly) libmbdnssd specific - which should work on a bunch of platforms, right?

I think we can make some baby steps here in unifying it because I think the other platforms look a lot like this one!
 - bring mdns/gonk/* into mdns/libmdns/*
 - use the nsSocketTransportService code for cross platform management of poll instead of just going to epoll with the montior service.. that's the only obvious thing I see that is gonk specific. you can use the libmdnssd api to get a FD to give to the STS. Among other things that will centralize our shutdown handling around polling which has been a problem in the past (and is timeout based in your patch)
 - perhaps separately, get the mdns lib into other platform builds

I'm trying to figure out the reason for the new thread other than epoll (which can go away).. It looks like we spin on get_property()? but we do that on the main thread (that would have to be changed) Is there no alternative to that? It would be nice if that (or whatever the real reason is) was documented in the thread management code as the rationale..

::: netwerk/dns/mdns/gonk/MDNSResponderMonitor.cpp
@@ +9,5 @@
> +#include <unistd.h>
> +
> +namespace mozilla {
> +namespace net {
> +namespace mdns {

just mozilla::net please. (many think it should just be mozilla, but mozilla::net is established)

::: netwerk/dns/mdns/nsIDNSServiceDiscovery.idl
@@ +144,5 @@
> +/**
> + * The callback interface for service resolve
> + */
> +[scriptable, uuid(24ee6408-648e-421d-accf-c6e5adeccf97)]
> +interface nsIDNSServiceResolveListener : nsISupports

you can also make this derived from nsICancelable

@@ +179,5 @@
> +   */
> +  void startDiscovery(in AUTF8String aServiceType, in nsIDNSServiceDiscoveryListener aListener);
> +
> +  /**
> +   * Terminate a previously started discovery.

you don't need this if the listener is cancelable

Comment 51

[Patrick McManus [:mcmanus]]

Comment on attachment 8586624 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

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

::: netwerk/dns/mdns/gonk/MDNSResponderOperator.h
@@ +120,5 @@
> +  nsCOMPtr<nsIDNSServiceInfo> mServiceInfo;
> +  nsCOMPtr<nsIDNSServiceResolveListener> mListener;
> +
> +  // hold self until callback is made.
> +  nsRefPtr<ResolveOperator> mKungFuDeathGrip;

this is a nit you can ignore if you like, but I dislike that member name. mDeleteProtector is preferred.

::: netwerk/dns/mdns/gonk/nsDNSServiceDiscovery.cpp
@@ +29,5 @@
> +
> +nsresult
> +WaitForProperty(const char* aName, const char* aDesiredValue, int aMaxWait)
> +{
> +    int maxNaps = (aMaxWait * 1000) / NAP_TIME;

so nap_time is 200ms, but the callers seem to be asking for 5ms max which seems incongruous

@@ +36,5 @@
> +        maxNaps = 1;
> +    }
> +
> +    while (maxNaps-- > 0) {
> +        usleep(NAP_TIME * 1000);

assert not main thread

@@ +37,5 @@
> +    }
> +
> +    while (maxNaps-- > 0) {
> +        usleep(NAP_TIME * 1000);
> +        if (CheckProperty(aName, aDesiredValue)) {

can we sleep after we check?

@@ +74,5 @@
> +nsresult
> +nsDNSServiceDiscovery::Init()
> +{
> +  nsresult rv;
> +  if (NS_WARN_IF(NS_FAILED(rv = StartService()))) {

seems to me like this janks the main thread

Comment 52

[Gary Chen [:xeonchen]]

(In reply to Patrick McManus [:mcmanus] from comment #50)
> I was hoping that this was going to be a cross platform implementation :(..
> but it seems that while it is described as being gonk specific, its really
> (mostly) libmbdnssd specific - which should work on a bunch of platforms,
> right?

I think it's should work on most platforms that's compatible with mDNSResponder library.
However, some platforms have their own mDNS service (e.g. NsdManager of Android), and we are supposed to deal with their mDNS related APIs.

> I think we can make some baby steps here in unifying it because I think the
> other platforms look a lot like this one!
>  - bring mdns/gonk/* into mdns/libmdns/*
>  - use the nsSocketTransportService code for cross platform management of
> poll instead of just going to epoll with the montior service.. that's the
> only obvious thing I see that is gonk specific. you can use the libmdnssd
> api to get a FD to give to the STS. Among other things that will centralize
> our shutdown handling around polling which has been a problem in the past
> (and is timeout based in your patch)
>  - perhaps separately, get the mdns lib into other platform builds

Sounds good, I'll work on it.

> I'm trying to figure out the reason for the new thread other than epoll
> (which can go away).. It looks like we spin on get_property()? but we do
> that on the main thread (that would have to be changed) Is there no
> alternative to that? It would be nice if that (or whatever the real reason
> is) was documented in the thread management code as the rationale..

A better solution is to launch mdnsd via netd, I'm studying about it.

> ::: netwerk/dns/mdns/nsIDNSServiceDiscovery.idl
> @@ +144,5 @@
> > +/**
> > + * The callback interface for service resolve
> > + */
> > +[scriptable, uuid(24ee6408-648e-421d-accf-c6e5adeccf97)]
> > +interface nsIDNSServiceResolveListener : nsISupports
> 
> you can also make this derived from nsICancelable
> 
> @@ +179,5 @@
> > +   */
> > +  void startDiscovery(in AUTF8String aServiceType, in nsIDNSServiceDiscoveryListener aListener);
> > +
> > +  /**
> > +   * Terminate a previously started discovery.
> 
> you don't need this if the listener is cancelable

I'm not sure the reason to make a callback interface derived from |nsICancelable|?
|nsIDNSServiceDiscovery| cannot be notified when |nsICancelable::Cancel| is called.

Do you mean returning a |nsICancelable| like the way [1] does?

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNSService.idl#45

Comment 53

[Patrick McManus [:mcmanus]]

> 
> Do you mean returning a |nsICancelable| like the way [1] does?

you're right - that's what I meant. And thinking about it more its not a big deal either way.

Comment 54

[Gary Chen [:xeonchen]]

(In reply to Liang-Heng Chen [:xeonchen] from comment #47)
> Created attachment 8586528 [details] [review]
> Part 1: Add libmdnssd.so to gecko library dependent list
> 
> To avoid link error of hazard build on try server.

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

Comment 55

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8586625 [details] [diff] [review]
Part 3: implement mDNS device provider

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

overall look good!

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +85,5 @@
> +  }
> +
> +  nsresult rv;
> +
> +  mMulticastDNS = do_GetService("@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1", &rv);

use DNSSERVICEDISCOVERY_CONTRACT_ID

@@ +156,5 @@
> +MulticastDNSDeviceProvider::RegisterService(uint32_t aPort)
> +{
> +  nsresult rv;
> +
> +  nsCOMPtr<nsIDNSServiceInfo> serviceInfo = do_CreateInstance("@mozilla.org/toolkit/components/mdnsresponder/dns-info;1", &rv);

use DNSSERVICEINFO_CONTRACT_ID instead since nsIDNSServiceDiscovery.h is included.

::: dom/presentation/provider/PresentationDeviceProviderModule.cpp
@@ +9,5 @@
> +#define MULTICAST_DNS_PROVIDER_CID \
> +  {0x814f947a, 0x52f7, 0x41c9, \
> +    { 0x94, 0xa1, 0x36, 0x84, 0x79, 0x72, 0x84, 0xac }}
> +
> +#define MULTICAST_DNS_PROVIDER_CONTRACT_ID "@mozilla.org/dom/presentation/multicastdns-provider;1"

suggest to use "@mozilla.org/presentation-device/multicastdns-provider;1" corresponding to the contractID of PresentationDeviceManager [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationDeviceManager.idl#11

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +195,5 @@
> +      Assert.equal(serviceInfo.serviceType, mockDevice.serviceType);
> +      listener.onServiceResolved(createDevice(mockDevice.host, mockDevice.port, mockDevice.serviceName, mockDevice.serviceType));
> +    }
> +  };
> +  let contractHook = new ContractHook(SD_CONTRACT_ID, mockObj);

SD_CONTRACT_ID has been hooked twice so there will be two cleanup function been hooked. However the |_oldFactory| for the second ContractHook will be the first MockFactory, which mean the factory for SD_CONTRACT_ID will be overwritten.

Comment 56

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement XPCOM module for mDNSProvider

[WIP] 
Since we have to make sure all MDNS operations are running after the service launched without blocking main thread, the MDNS operations are moved into another thread to avoid blocking main thread.

Patrick, do you have any suggestion on this solution?

Comment 57

[Gary Chen [:xeonchen]]

Attachment: Part 3: implement mDNS device provider

Comment 58

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8590776 [details] [diff] [review]
Part 3: implement mDNS device provider

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

lgtm! Thanks for the hard work!

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +47,5 @@
> +
> +  this.init();
> +}
> +
> +ContractHook.hookedMap = new Map(); // remember only the most original factory.

you can put |hookedMap| initialization into ContractHook.prototype.

@@ +53,5 @@
> +ContractHook.prototype = {
> +  init: function() {
> +    this.reset();
> +
> +    Assert.ok(ContractHook.hookedMap.has(this._contractID));

mock object setup/cleanup is not part of our test target. It's ok to remove these |Assert.ok|.

@@ +68,5 @@
> +  cleanup: function() {
> +    this.reset();
> +
> +    Assert.ok(ContractHook.hookedMap.has(this._contractID));
> +    Assert.ok(ContractHook.hookedMap.get(this._contractID).length > 0);

here too.

@@ +217,5 @@
> +      listener.onServiceFound(createDevice("", 0, mockDevice.serviceName, mockDevice.serviceType));
> +      return {
> +        QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]),
> +        cancel: function() {
> +        }.bind(this)

bind to an empty function seems useless.

@@ +229,5 @@
> +    }
> +  };
> +  let contractHook = new ContractHook(SD_CONTRACT_ID, mockObj);
> +
> +  let provider = Cc["@mozilla.org/presentation-device/multicastdns-provider;1"].createInstance(Ci.nsIPresentationDeviceProvider);

suggest to make this contract Id as a const variable like others.

Comment 59

[Gary Chen [:xeonchen]]

Attachment: Part 3: implement mDNS device provider

refine and carry r+

Comment 60

[Steve Fink [:sfink] [:s:]]

(In reply to Gary Chen [:xeonchen] from comment #44)
> (In reply to Steve Fink [:sfink, :s:] from comment #36)
> > The hazard build currently just does |build.sh gecko|, except it also passes
> > a custom mozconfig that looks like
> > 
> >   . gonk-misc/default-gecko-config
> >   ac_add_options --with-compiler-wrapper=...
> >   ac_add_options --without-ccache
> > 
> 
> The link error seems because it *only* does |build.sh gecko| instead of
> |build.sh|, therefore the libraries in the external/* are not built.
> I'm going to find out how to avoid this situation.

Ok. I saw your pull request. If that fixes it, fine. Otherwise, you can add whatever you like into the mozharness build script mozharness/scripts/spidermonkey/build.b2g

> Steve, do we have a document to build the hazard analysis process on my
> computer instead of try server?

No. Theoretically, you could run the mozharness script hazard_build.py with the right config settings, but in practice it's a total PITA because it requires access to a tooltool server that requires LDAP credentials and there's something broken in that setup anyway. But now there's a public tooltool server. I need to get back to cleaning that up.

(To be sure, I *do* occasionally run the analysis on my local machine, but I have my own tooltool server set up and I have to spend a while to fix it every time I do this.)

Comment 61

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement XPCOM module for mDNSProvider

[WIP] trying to solve main thread blocking issue.

Dispatch only |StartMDNSService()| into another thread.
Any API call before service will get a failure callback with error code |ERROR_SERVICE_NOT_RUNNING|.

Comment 62

[Gary Chen [:xeonchen]]

(In reply to Patrick McManus [:mcmanus] from comment #50)
>  - use the nsSocketTransportService code for cross platform management of
> poll instead of just going to epoll with the montior service.. that's the
> only obvious thing I see that is gonk specific. you can use the libmdnssd
> api to get a FD to give to the STS. Among other things that will centralize
> our shutdown handling around polling which has been a problem in the past
> (and is timeout based in your patch)

STS requires |PRFileDesc| type, I see transformation from osfd to PRFileDesc in [1],
but I feel it might not be a good idea.

Instead, |FdWatcher| in [2], which uses libevent, seems an alternative solution.

[1] https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/pthreads/ptio.c#3300
[2] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsDumpUtils.h#35

Comment 63

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement XPCOM module for mDNSProvider

use |MessageLoopForIO| to read data instead of creating another thread.

Comment 64

[Gary Chen [:xeonchen]]

Attachment: Part 3: implement mDNS device provider

handle registration event failure in |OnRegistrationFailed| callback.

Comment 65

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8593279 [details] [diff] [review]
Part 3: implement mDNS device provider

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

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +169,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv = serviceInfo->SetPort(aPort)))) {
> +    return rv;
> +  }
> +
> +  mRegisterRequest = nullptr;

need to cancel previous request?

@@ +209,5 @@
> +  MOZ_ASSERT(mMulticastDNS);
> +
> +  nsresult rv;
> +
> +  mDiscoveryRequest = nullptr;

need cancel, too?

@@ +210,5 @@
> +
> +  nsresult rv;
> +
> +  mDiscoveryRequest = nullptr;
> +  if (NS_WARN_IF(NS_FAILED(rv = mMulticastDNS->StartDiscovery(NS_LITERAL_CSTRING(SERVICE_TYPE), mWrappedListener, getter_AddRefs(mDiscoveryRequest))))) {

nit: too long.

Comment 66

[Gary Chen [:xeonchen]]

Attachment: Part 3: implement mDNS device provider

refinement

Comment 67

[Gary Chen [:xeonchen]]

Attachment: Part 3: implement mDNS device provider

fix try error, carry r+

Comment 68

[Michael Wu [:mwu]]

Comment on attachment 8586528 [details] [review]
Part 1: Add libmdnssd.so to gecko library dependent list

Avoid the shell call and check if we're on ICS (15) instead. We don't/won't support anything older than ICS.

Comment 69

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

fix try server warnings of public destructors

Comment 70

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

fix mochitest error because the provider does not exist on some platforms.

Comment 71

[Gary Chen [:xeonchen]]

Attachment: Part 0: Add libmdnssd.so to gecko library dependent list

Comment 72

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8595099 [details] [diff] [review]
Part 2: implement mDNS device provider

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

::: dom/presentation/provider/PresentationDeviceProviderModule.cpp
@@ +27,5 @@
> +  { nullptr }
> +};
> +
> +static const mozilla::Module::CategoryEntry kPresentationDeviceProviderCategories[] = {
> +#if MOZ_WIDGET_GONK && ANDROID_VERSION >= 16

#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 16

Comment 73

[Patrick McManus [:mcmanus]]

Comment on attachment 8595097 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

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

This seems to be making great progress! Can we get at least one non-gonk platform enabled?

::: netwerk/dns/mdns/libmdns/MDNSResponderOperator.cpp
@@ +48,5 @@
> +  }
> +
> +  void Init()
> +  {
> +    if (mService) {

typically we format this as

if (!mService) {
 return;
}

normal-case-code-minimally indented;
return;

@@ +50,5 @@
> +  void Init()
> +  {
> +    if (mService) {
> +      int fd = DNSServiceRefSockFD(mService);
> +      XRE_GetIOMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(this, &ServiceWatcher::StartWatching, fd));

I really want this done with the SocketTransportService .. other uses of the ServiceWatcher aren't really for networking (and if they are, they ought to be consolidated to the STS as well) - its important to have a central point to manage shutdown from, track file descriptor use from, and be able to do reporting in about:networking on. Potentially we will want to be able to do things like control routing on app-id basis for various vpns as well, so its important to have a centralized dispatching layer.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +40,5 @@
> +    while (maxNaps-- > 0) {
> +        if (CheckProperty(aName, aDesiredValue)) {
> +          return NS_OK;
> +        }
> +        usleep(NAP_TIME * 1000);

really importnant here to assert that you aren't on the main thread or the STS thread - janking either of those with a blocking sleep is something we need to be careful of.

@@ +157,5 @@
> +{
> +  NS_NewNamedThread("mDNS Starter", getter_AddRefs(mThread));
> +
> +  nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(this, &nsDNSServiceDiscovery::StartService);
> +  mThread->Dispatch(event, NS_DISPATCH_NORMAL);

as far as I understand, all of the methods of this class are meant to run on this new thread. That should be documented in the h file each method should assert it. This is especially important to help future implementors understand the implicit locking of the hash tables.

Comment 74

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

fix suggestion, carry r+

Comment 75

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

enable log only when PR_LOGGING is define.

Comment 76

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

use STS to check readable

Comment 77

[Gary Chen [:xeonchen]]

(In reply to Patrick McManus [:mcmanus] from comment #73)
> Comment on attachment 8595097 [details] [diff] [review]
> Part 1: implement XPCOM module for mDNSProvider
> 
> Review of attachment 8595097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to be making great progress! Can we get at least one non-gonk
> platform enabled?
> 
Yes, I've created Bug 1158029 to follow-up.

> ::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
> @@ +40,5 @@
> > +    while (maxNaps-- > 0) {
> > +        if (CheckProperty(aName, aDesiredValue)) {
> > +          return NS_OK;
> > +        }
> > +        usleep(NAP_TIME * 1000);
> 
> really importnant here to assert that you aren't on the main thread or the
> STS thread - janking either of those with a blocking sleep is something we
> need to be careful of.
> 
> @@ +157,5 @@
> > +{
> > +  NS_NewNamedThread("mDNS Starter", getter_AddRefs(mThread));
> > +
> > +  nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(this, &nsDNSServiceDiscovery::StartService);
> > +  mThread->Dispatch(event, NS_DISPATCH_NORMAL);
> 
> as far as I understand, all of the methods of this class are meant to run on
> this new thread. That should be documented in the h file each method should
> assert it. This is especially important to help future implementors
> understand the implicit locking of the hash tables.

Of course it guarantees server is ready if we put |StartService| and following
API calls into another thread, but it's a little bit complicated to cancel an
unstarted task in the queue (maybe set a cancel flag, I guess).
So I've removed this checking and thread since it's not really helpful.

The default behavior is that it will invoke an error callback if an API is called
before server is ready, and it's caller's responsibility to handle the error.

Comment 78

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

fix build error on some platforms

Comment 79

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

fix build error on Windows platform.

Comment 80

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8601286 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

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

::: netwerk/dns/mdns/libmdns/MDNSResponderReply.cpp
@@ +188,5 @@
> +ResolveReplyRunnable::Reply(DNSServiceRef aSdRef,
> +                            DNSServiceFlags aFlags,
> +                            uint32_t aInterfaceIndex,
> +                            DNSServiceErrorType aErrorCode,
> +                            const char* fullname,

nit: aFullname / aHostTarget / etc...

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +179,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv = UnregisterService(aListener)))) {
> +    return rv;
> +  }
> +
> +  nsRefPtr<RegisterOperator> registerOp(new RegisterOperator(aServiceInfo,

nit: use | foo = new Foo()| instead.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

Comment 81

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

fit nit parts

Comment 82

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8603698 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

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

::: netwerk/dns/mdns/libmdns/MDNSResponderOperator.cpp
@@ +31,5 @@
> +  static PRLogModuleInfo* log = PR_NewLogModule("MDNSResponderOperator");
> +  return log;
> +}
> +
> +#ifdef PR_LOGGING

PR_LOGGING is going to be removed, see Bug 1161238.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +39,5 @@
> +}
> +
> +} // namespace anonymous
> +
> +namespace {

only need one anonymous namespace.

@@ +66,5 @@
> +  , mListener(aListener)
> +{
> +}
> +
> +NS_IMETHODIMP DiscoveryRequest::Cancel(nsresult aReason)

nit: split return value to a separate line.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.h
@@ +32,5 @@
> +  **/
> +  nsresult Init();
> +
> +  nsresult StopDiscovery(nsIDNSServiceDiscoveryListener *aListener);
> +  nsresult UnregisterService(nsIDNSRegistrationListener *aListener);

nit: nsIDNSServiceDiscoveryListener* aListener / nsIDNSRegistrationListener* aListener.

::: netwerk/dns/mdns/libmdns/nsDNSServiceInfo.cpp
@@ +23,5 @@
> +  nsAutoCString str;
> +  uint16_t value;
> +
> +  if (NS_SUCCEEDED(aServiceInfo->GetHost(str))) {
> +    MOZ_ASSERT(NS_SUCCEEDED(SetHost(str)));

MOZ_ASSERT shouldn't be use on an expression with side effect.

::: netwerk/dns/mdns/moz.build
@@ +9,5 @@
> +XPIDL_SOURCES += [
> +    'nsIDNSServiceDiscovery.idl',
> +]
> +
> +XPIDL_MODULE = 'mdns'

The XPIDL_MODULE under netwerk folder all have "necko_" prefix, maybe we should use "necko_mdns" here.

Comment 83

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

fix some logic errors

Comment 84

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

remove PR_LOGGING and add some messages

Comment 85

[GitHub Autolander Bot]

Attachment: Part 3: UI test of Presentation API

Comment 86

[Gary Chen [:xeonchen]]

Attachment: Part 3: UI test of Presentation API

Comment 87

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8606081 [details] [review]
Part 3: UI test of Presentation API

Looks good to me. Please check PR for the nits.

If you want to land this test code before gecko patch, please add code to detect the existence of mozPresentationDeviceInfo.

Comment 88

[Gary Chen [:xeonchen]]

Comment on attachment 8606081 [details] [review]
Part 3: UI test of Presentation API

check capability

Comment 89

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8606081 [details] [review]
Part 3: UI test of Presentation API

Looks good to me. This version looks like that it can be landed before gecko patch. Thanks.

Comment 90

[Gary Chen [:xeonchen]]

Attachment: Part 1: implement XPCOM module for mDNSProvider

update log to "mozilla/Logging.h"

Comment 91

[Gary Chen [:xeonchen]]

Attachment: Part 2: implement mDNS device provider

update log to "mozilla/Logging.h"

Comment 92

[Patrick McManus [:mcmanus]]

Comment on attachment 8608516 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

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

I really appreciate the updates. lgtm

Comment 93

[Gary Chen [:xeonchen]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc165d9f51cc

Comment 94

[Gary Chen [:xeonchen]]

Part 1 depends on Part 0, so please land Part 0 and b2g/config/*/sources.xml should be updated first
Otherwise it may cause the problem in [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56&filter-searchStr=b2g_try_linux64-b2g-haz_dep

Comment 95

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gonk-misc/commit/5deaf27fd266316f27e68206cc3be0e6f47ded54

Master: https://github.com/mozilla-b2g/gaia/commit/084f9ef593d201fcc077c55309d945d9b378412a

Comment 96

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/dda118cb0bcc
https://hg.mozilla.org/integration/b2g-inbound/rev/7ada5429c80d

Comment 97

[Ryan VanderMeulen [:RyanVM]]

Not sure why you removed the head/tail lines from xpcshell.ini, but it blew up the remote xpcshell harness (and the failures were visible in your Try push as well). I pushed a follow-up fix since backing out would have been more problematic than just fixing it.
https://hg.mozilla.org/integration/b2g-inbound/rev/fa0c93ed6885

https://treeherder.mozilla.org/logviewer.html#?job_id=2075752&repo=b2g-inbound

Comment 98

[Gary Chen [:xeonchen]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #97)
> Not sure why you removed the head/tail lines from xpcshell.ini, but it blew
> up the remote xpcshell harness (and the failures were visible in your Try
> push as well). I pushed a follow-up fix since backing out would have been
> more problematic than just fixing it.
> https://hg.mozilla.org/integration/b2g-inbound/rev/fa0c93ed6885
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=2075752&repo=b2g-
> inbound

Thank you, it's my fault.

Comment 99

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dda118cb0bcc
https://hg.mozilla.org/mozilla-central/rev/7ada5429c80d
https://hg.mozilla.org/mozilla-central/rev/fa0c93ed6885

Comment 100

[Junior [inactive]]

Finally. Congrats!