1205237 - [Presentation WebAPI] get self IP address on Fennec for initiating presentation session

Status: RESOLVED FIXED

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

Description

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

We need to get self IP address to initiate presentation session request. We only have B2G implementation right now. [1]

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#407

Comment 1

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Hi Josh,

Could you help mark 2.5+ to this one?

Comment 2

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

Hi @xeonchen, could you help on this bug?

Comment 3

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

NSPR uses SIOCGIFCONF ioctl to query self network interface, however I didn't found any NSPR API for retrieving this information in Gecko.
https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/misc/prnetdb.c#148

On Android you can get IP address via WifiManager but you'll need to call Java API.
http://stackoverflow.com/a/6071963/1312532

Comment 4

[Gary Chen [:xeonchen]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #2)
> Hi @xeonchen, could you help on this bug?

sure!

Comment 5

[Gary Chen [:xeonchen]]

Attachment: Part 1: Get Wi-Fi IP address via Android API

Hi Richard,

I'm going to retrieve the Wi-Fi IP address on Android devices for this issue, would you please take some time to check the implementation?

Comment 6

[Gary Chen [:xeonchen]]

Attachment: [WIP] Get self Wi-Fi IP address

Comment 7

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

Attachment: [WIP-v2] bug1205237-self-ip-address.patch

Here is the patch fixed several XPCOM registration issues. However I still get following error while invoking |getWifiIpAddress|.
> OnError: java.lang.NullPointerException: Attempt to get length of null array

Comment 8

[Gary Chen [:xeonchen]]

Attachment: [WIP-v3] Get self Wi-Fi IP address

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8672541 [details] [diff] [review]
Part 1: Get Wi-Fi IP address via Android API

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

::: mobile/android/base/GeckoNetworkManager.java
@@ +180,5 @@
> +                break;
> +            }
> +            case "Wifi:GetIpAddress": {
> +                try {
> +                    getWifiIpAddress(callback);

I have a moderate preference for capitalizing "IPAddress" throughout, both in method names and message names.

@@ +199,5 @@
> +
> +        byte[] macBytes = new byte[6];
> +
> +        for (int i = 0; i < mac.length; ++i) {
> +            macBytes[i] = Integer.decode("0x" + mac[i]).byteValue();

macBytes[i] = Byte.parseByte(mac[i], 16);

@@ +242,5 @@
> +
> +        Enumeration<NetworkInterface> eni = NetworkInterface.getNetworkInterfaces();
> +        while (eni.hasMoreElements()) {
> +            NetworkInterface ni = eni.nextElement();
> +            if (ni.getName().equals(wifiName)) {

This is a bit silly. We use `getWifiInterfaceName` -- which walks the list of network interfaces! -- to get the name of the interface, then we walk the list again checking by name.

Just kill this method and replace `getWifiInterfaceName` with `getWifiInterface`, skipping the name entirely.

@@ +258,5 @@
> +            return;
> +        }
> +
> +        JSONObject obj = new JSONObject();
> +        Enumeration<InetAddress> addresses = ni.getInetAddresses();

What's wrong with just

  WifiManager mgr = ...;
  if (mgr == null) { ... }
  WifiInfo info = mgr.getConnectionInfo();
  if (info == null) { ... }
  int ip = info.getIpAddress();

?

Is it just that you need to get IPv6 addresses too?

Comment 10

[Gary Chen [:xeonchen]]

(In reply to Richard Newman [:rnewman] from comment #9)
> What's wrong with just
> 
>   WifiManager mgr = ...;
>   if (mgr == null) { ... }
>   WifiInfo info = mgr.getConnectionInfo();
>   if (info == null) { ... }
>   int ip = info.getIpAddress();
> 
> ?
> 
> Is it just that you need to get IPv6 addresses too?

Yes, but thought we may need only IPv4 now.
Maybe use this simpler implementation is enough.

Comment 11

[Gary Chen [:xeonchen]]

Attachment: Part 1: get Wi-Fi IP address via Android API

Use WifiManager to get IPv4 address

Comment 12

[Gary Chen [:xeonchen]]

Attachment: Part 2: add PresentationNetworkHelper

Comment 13

[Gary Chen [:xeonchen]]

Attachment: Part 3: get self Wi-Fi IP address

|NotifyOpened()| -> |GetAddress()| sequence is adjusted to support Fennec asynchronous callback.

Comment 14

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8674752 [details] [diff] [review]
Part 3: get self Wi-Fi IP address

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

::: dom/presentation/PresentationSessionInfo.cpp
@@ +31,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::services;
>  
> +inline static PRLogModuleInfo*
> +GetLog()

nit: GetLog => GetPresentationSessionInfoLog (just like what other files do.)

@@ +431,5 @@
>    // scenarios.
>    nsAutoString ip;
>    ip.Assign(ips[0]);
> +
> +  NS_DispatchToCurrentThread(

nit: please add a comment to briefly describe the reason why we need to use a runnable (rather than simply call |nsresult rv = OnGetAddress(NS_ConvertUTF16toUTF8(ip))|).

@@ +449,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  if (NS_WARN_IF(NS_FAILED(rv = networkHelper->GetWifiIPAddress(this)))) {

nit: let's split this into two lines to follow the convention in this file (and for the readability).

rv = networkHelper->GetWifiIPAddress(this);
if (NS_WARN_IF(NS_FAILED(rv)) {
  ...
}

::: dom/presentation/PresentationSessionInfo.h
@@ +171,5 @@
>  
>    void Shutdown(nsresult aReason) override;
>  
> +  nsresult GetAddress();
> +  nsresult OnGetAddress(const nsACString& aAddress);

nit: add an empty line between the two methods (the convention of this file).

Comment 15

[Gary Chen [:xeonchen]]

Attachment: [v2] Part 3: get self Wi-Fi IP address

fix nit parts, carry r+

Comment 16

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

Comment on attachment 8674747 [details] [diff] [review]
Part 2: add PresentationNetworkHelper

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

overall looks good to me but need some minor tweak before check-in.

::: dom/presentation/PresentationNetworkHelper.js
@@ +15,5 @@
> +function PresentationNetworkHelper() {}
> +
> +PresentationNetworkHelper.prototype = {
> +  classID: NETWORKHELPER_CID,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference,

remove the weak reference interface since you don't actually use it.

::: dom/presentation/interfaces/moz.build
@@ +10,5 @@
>      'nsIPresentationDeviceManager.idl',
>      'nsIPresentationDevicePrompt.idl',
>      'nsIPresentationDeviceProvider.idl',
>      'nsIPresentationListener.idl',
> +    'nsIPresentationNetworkHelper.idl',

Wrap it with compile option if this is only used in Fennec.

Comment 17

[Gary Chen [:xeonchen]]

Attachment: [v3] Part 3: get self Wi-Fi IP address

Rebase because Bug 1207245 changes the name of nsRefPtr to RefPtr.

Comment 18

[Gary Chen [:xeonchen]]

Attachment: [v2] Part 2: add PresentationNetworkHelper

fix per comment

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 8674743 [details] [diff] [review]
Part 1: get Wi-Fi IP address via Android API

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

::: mobile/android/base/GeckoNetworkManager.java
@@ +175,5 @@
> +            }
> +        }
> +    }
> +
> +    private void getWifiIPAddress(final EventCallback callback) {

Comment that this only works for IPv4 addresses, file a follow-up if you'd like it to support IPv6.

@@ +189,5 @@
> +            callback.sendError("Cannot get connection info");
> +            return;
> +        }
> +
> +        callback.sendSuccess(Formatter.formatIpAddress(info.getIpAddress()));

getIpAddress() returns zero if the connection isn't IPv4:

  https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/wifi/java/android/net/wifi/WifiInfo.java#481

You need to check for zero here.

Comment 20

[Gary Chen [:xeonchen]]

Attachment: [v2] Part 1: get Wi-Fi IP address via Android API

add comment and check have IPv4 address, carry r+.

Comment 21

[Gary Chen [:xeonchen]]

Attachment: [v4] Part 3: get self Wi-Fi IP address

To make |nsIPresentationNetworkHelperListener| invisible in non-Fennec builds, move origin implementation into a wrapper class and deliver the result by callback.

Comment 22

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8676074 [details] [diff] [review]
[v4] Part 3: get self Wi-Fi IP address

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

Overall it looks good to me.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +67,5 @@
> +  using Function = nsresult(PresentationControllingInfo::*)(const nsACString&);
> +
> +  explicit PresentationNetworkHelper(PresentationControllingInfo* aInfo);
> +
> +  nsresult GetWifiIPAddress(const Function& aFunc);

nit: can we move |const Function& aFunc| from this function to the constructor? Then |mFunc| becomes determined after initialization.

@@ +123,5 @@
> +
> +  mThread->Dispatch(NS_NewRunnableMethodWithArg<nsCString>(mInfo,
> +                                                           mFunc,
> +                                                           aIPAddress),
> +                    NS_DISPATCH_NORMAL);

Since there's an assertion |MOZ_ASSERT(NS_IsMainThread())| in |PresentationControllingInfo::OnGetAddress|, which is supposed to be assigned to |mFunc|, why don't we simply use |NS_DispatchToMainThread| here? (Then |mThread| would become unnecessarily.)

Comment 23

[Gary Chen [:xeonchen]]

Attachment: [v5] Part 3: get self Wi-Fi IP address

slightly adjust function/thread calls, carry r+

Comment 24

[Gary Chen [:xeonchen]]

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

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aca15a8d20d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c0b212a2879
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47029d8d419

Comment 26

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64ea72607527
https://hg.mozilla.org/integration/mozilla-inbound/rev/487dd755e3a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b21c60c5eac

Comment 27

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16018768&repo=mozilla-inbound

Comment 28

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

Comment on attachment 8676109 [details] [diff] [review]
[v5] Part 3: get self Wi-Fi IP address

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

This should be the reason of test failure on firefox desktop.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +536,2 @@
>  #else
>    // TODO Get host IP via other platforms.

We need to dispatch a runnable for "PresentationControllingInfo::OnGetAddress" to continue the session request flow on other platform.

Comment 29

[Gary Chen [:xeonchen]]

Attachment: [v6] Part 3: get self Wi-Fi IP address

fix mochitest timeout

Comment 30

[Gary Chen [:xeonchen]]

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

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c72eaef7f82
https://hg.mozilla.org/integration/mozilla-inbound/rev/6558cc9ebe66
https://hg.mozilla.org/integration/mozilla-inbound/rev/099ca9784468

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c72eaef7f82
https://hg.mozilla.org/mozilla-central/rev/6558cc9ebe66
https://hg.mozilla.org/mozilla-central/rev/099ca9784468