1193605 - Crash [@ java.lang.IllegalArgumentException: service discovery not active on listener at android.net.nsd.NsdManager.stopServiceDiscovery(NsdManager.java) ]

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Several of the crashes (I didn't look at them all) were from low end Cubot devices. I wonder if those devices don't have the Nsd service? Are we checking that the "start" was successful? If not, there is no need to try to "stop" the service.

A report:
https://crash-stats.mozilla.com/report/index/a85445ac-9f93-43ce-a0aa-81c942150809

More reports:
https://crash-stats.mozilla.com/report/list?product=FennecAndroid&signature=java.lang.IllegalArgumentException%3A+service+discovery+not+active+on+listener+at+android.net.nsd.NsdManager.stopServiceDiscovery%28NsdManager.java%29#tab-reports

Comment 1

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1193605-Part-1-enable-mdns-on-Fennec-r-mcmanus-r.patch

In Fennec, ANDROID_VERSION is not used for check platform version.
So I removed this compile-time checking and fix some behavior in part 2.

Comment 2

[Gary Chen [:xeonchen]]

Attachment: 0002-Bug-1193605-Part-2-process-callbacks-correctly-r-rne.patch

In Android prior than Lollipop, it will create a |DummyMulticastDNSManager|, and return failure for operations on this dummy manager.

Comment 3

[Gary Chen [:xeonchen]]

Attachment: 0003-Bug-1193605-Part-3-do-not-stop-non-started-service-r.patch

Not to stop an operation with failure start.

Comment 4

[Gary Chen [:xeonchen]]

Attachment: 0004-Bug-1193605-Part-4-add-UUID-for-listeners-r-mcmanus.patch

Add UUID for listeners send to Java-side

Comment 5

[Gary Chen [:xeonchen]]

Attachment: 0005-Bug-1193605-Part-5-separate-different-discovery-regi.patch

Original MulticastDNSManager cannot process multiple registration/discovery request, use an extra UUID attribute to distinguish between different types of registration/discovery.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8652325 [details] [diff] [review]
0001-Bug-1193605-Part-1-enable-mdns-on-Fennec-r-mcmanus-r.patch

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

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

If you want to be really efficient, you can use

  MOZ_ANDROID_MIN_SDK_VERSION
  MOZ_ANDROID_MAX_SDK_VERSION

to check a range here. They default to [11, 999] for most builds. Constrained builds are [9, 10].

If MOZ_ANDROID_MAX_SDK_VERSION < 21, then this APK can't be installed on a device that supports this, and you can just short-circuit here. Otherwise, you need a runtime check.

::: netwerk/dns/mdns/libmdns/moz.build
@@ +3,5 @@
>  # 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/.
>  
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android':

Same here. This'll exclude these files from the constrained build altogether, which is nice to have.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8652329 [details] [diff] [review]
0002-Bug-1193605-Part-2-process-callbacks-correctly-r-rne.patch

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

::: mobile/android/base/mdns/MulticastDNSManager.java
@@ +179,2 @@
>      public DummyMulticastDNSManager() {
> +        Log.i(LOGTAG, "DummyMulticastDNSManager created");

Take this out or drop it to .v.

@@ +210,5 @@
> +
> +    @Override
> +    public void handleMessage(final String event, final NativeJSObject message, final EventCallback callback) {
> +        Log.v(LOGTAG, "handleMessage: " + event);
> +        callback.sendError(FAILURE_INTERNAL_ERROR);

Wouldn't it be better to explicitly return a code like FAILURE_NOT_SUPPORTED here? INTERNAL_ERROR seems very vague when we know exactly what's going on.

::: mobile/android/modules/MulticastDNS.jsm
@@ +156,5 @@
>          aListener.onStartDiscoveryFailed(aServiceType, err);
>        } else {
>          aListener.onDiscoveryStarted(result);
>        }
> +    }).bind(this));

Why do you need `bind` when you're using arrow notation?

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8652325 [details] [diff] [review]
0001-Bug-1193605-Part-1-enable-mdns-on-Fennec-r-mcmanus-r.patch

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

lgtm - also fine with rnewman's suggestion

Comment 9

[Gary Chen [:xeonchen]]

Attachment: 0002-Bug-1193605-Part-2-process-callbacks-correctly-r-rne.patch

fix per reviewer's comment

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8653340 [details] [diff] [review]
0002-Bug-1193605-Part-2-process-callbacks-correctly-r-rne.patch

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

::: mobile/android/base/mdns/MulticastDNSManager.java
@@ +185,5 @@
>      }
>  
>      @Override
>      public void tearDown() {
> +        unregisterEvents();

This will throw if registerEvents was never called. That might happen if the lifecycle ever gets more complicated, or if onDestroy is called without onCreate ever reaching init. 

That's happening right now in Bug 1119915, so this is not a hypothetical problem!

You might want to save yourself a future patch or review by requiring that tearDown and init are called from the UI thread (annotate them with @UiThread and add ThreadUtils.assertOnUiThread() to each), and adding a simple boolean flag that controls whether you call unregisterEvents.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8652337 [details] [diff] [review]
0005-Bug-1193605-Part-5-separate-different-discovery-regi.patch

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

Canceling review to save myself time; please go convince yourself that your changes preserve thread safety!

::: mobile/android/base/mdns/MulticastDNSManager.java
@@ +63,5 @@
>  
>      @Override
>      public void init() {
> +        mDiscoveryListeners = new HashMap<String, DiscoveryListener>();
> +        mRegistrationListeners = new HashMap<String, RegistrationListener>();

These are touched from the UI thread.

@@ +102,5 @@
>          switch (event) {
> +            case "NsdManager:DiscoverServices": {
> +                DiscoveryListener listener = new DiscoveryListener(nsdManager);
> +                listener.discoverServices(message.getString("serviceType"), callback);
> +                mDiscoveryListeners.put(message.getString("uniqueId"), listener);

… and this is touched from the Gecko thread. Java does not guarantee that mDiscoveryListeners is non-null here.

The simplest fix is to allocate those two maps from the constructor.

But furthermore: the map itself isn't thread safe. Those should be ConcurrentHashMaps or something else.

Comment 12

[Gary Chen [:xeonchen]]

Attachment: [v3] 0002-Bug-1193605-Part-2-process-callbacks-correctly-r-rne.patch

Changes in this patch:
* extract event registration into a mix-in class
* add thread checking annotation & assertion.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8654082 [details] [diff] [review]
[v3] 0002-Bug-1193605-Part-2-process-callbacks-correctly-r-rne.patch

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

r+ with nits.

::: mobile/android/base/mdns/MulticastDNSManager.java
@@ +110,3 @@
>  class NsdMulticastDNSManager extends MulticastDNSManager implements NativeEventListener {
>      private final NsdManager nsdManager;
> +    private MulticastDNSEventManager mEventManager = null;

Nit: no need to default these to null. Believe it or not, this generates bytecode for no good reason!
Nit: final.

@@ +214,5 @@
>  }
>  
> +class DummyMulticastDNSManager extends MulticastDNSManager implements NativeEventListener {
> +    static final int FAILURE_UNSUPPORTED = -65544;
> +    private MulticastDNSEventManager mEventManager = null;

final, no null.

Comment 14

[Gary Chen [:xeonchen]]

Attachment: [v4] 0002-Bug-1193605-Part-2-process-callbacks-correctly-r-rne.patch

fix nits, carry r+

Comment 15

[Gary Chen [:xeonchen]]

Attachment: [v2] 0005-Bug-1193605-Part-5-separate-different-discovery-regi.patch

use ConcurrentHashMap to keep thread safety.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8654696 [details] [diff] [review]
[v2] 0005-Bug-1193605-Part-5-separate-different-discovery-regi.patch

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

::: mobile/android/base/mdns/MulticastDNSManager.java
@@ +149,5 @@
> +            case "NsdManager:StopServiceDiscovery": {
> +                String uuid = message.getString("uniqueId");
> +                DiscoveryListener listener = mDiscoveryListeners.remove(uuid);
> +                if (listener == null) {
> +                    Log.e(LOGTAG, "DiscoveryListener " + uuid + " is not found.");

was not found.

@@ +169,5 @@
> +            case "NsdManager:UnregisterService": {
> +                String uuid = message.getString("uniqueId");
> +                RegistrationListener listener = mRegistrationListeners.remove(uuid);
> +                if (listener == null) {
> +                    Log.e(LOGTAG, "RegistrationListener " + uuid + " is not found.");

was not found.

@@ -289,3 @@
>          }
> -        mStartCallback.sendError(errorCode);
> -        mStartCallback = null;

You're no longer clearing m*Callback after notifying them. Are you confident that you no longer need to?

Comment 17

[Gary Chen [:xeonchen]]

(In reply to Richard Newman [:rnewman] from comment #16)
> Comment on attachment 8654696 [details] [diff] [review]
> [v2] 0005-Bug-1193605-Part-5-separate-different-discovery-regi.patch
> 
> Review of attachment 8654696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/mdns/MulticastDNSManager.java
> @@ -289,3 @@
> >          }
> > -        mStartCallback.sendError(errorCode);
> > -        mStartCallback = null;
> 
> You're no longer clearing m*Callback after notifying them. Are you confident
> that you no longer need to?

Yes. It's not supposed to be called twice.
If it is really called twice, I hope it will be found asap.

Comment 18

[Gary Chen [:xeonchen]]

Attachment: [v3] 0005-Bug-1193605-Part-5-separate-different-discovery-regi.patch

fix wording, carry r+

Comment 19

[Gary Chen [:xeonchen]]

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/609b337bf7ab
https://hg.mozilla.org/integration/fx-team/rev/fa04b5843674
https://hg.mozilla.org/integration/fx-team/rev/bb221f207c1a
https://hg.mozilla.org/integration/fx-team/rev/62e37d61a096
https://hg.mozilla.org/integration/fx-team/rev/bd4e19973ec2

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/609b337bf7ab
https://hg.mozilla.org/mozilla-central/rev/fa04b5843674
https://hg.mozilla.org/mozilla-central/rev/bb221f207c1a
https://hg.mozilla.org/mozilla-central/rev/62e37d61a096
https://hg.mozilla.org/mozilla-central/rev/bd4e19973ec2