Closed Bug 1205237 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
FxOS-S10 (30Oct)
feature-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: schien, Assigned: xeonchen)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [ft:conndevices])

Attachments

(3 files, 11 obsolete files)

5.33 KB, patch
schien
: review+
Details | Diff | Splinter Review
4.81 KB, patch
xeonchen
: review+
Details | Diff | Splinter Review
8.78 KB, patch
xeonchen
: review+
Details | Diff | Splinter Review
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
Summary: [Presentation WebAPI] Make Presentation API available for browser chrome code → [Presentation WebAPI] get self IP address on Fennec for initiating presentation session
Hi Josh,

Could you help mark 2.5+ to this one?
Flags: needinfo?(jocheng)
feature-b2g: --- → 2.5+
Flags: needinfo?(jocheng)
Priority: -- → P1
Hi @xeonchen, could you help on this bug?
Flags: needinfo?(xeonchen)
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
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #2)
> Hi @xeonchen, could you help on this bug?

sure!
Assignee: nobody → xeonchen
Status: NEW → ASSIGNED
Flags: needinfo?(xeonchen)
Target Milestone: --- → FxOS-S9 (16Oct)
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?
Attachment #8672541 - Flags: feedback?(rnewman)
Attached patch [WIP] Get self Wi-Fi IP address (obsolete) — Splinter Review
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 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?
Attachment #8672541 - Flags: feedback?(rnewman)
(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.
Use WifiManager to get IPv4 address
Attachment #8672541 - Attachment is obsolete: true
Attachment #8674743 - Flags: review?(rnewman)
Attachment #8674747 - Flags: review?(schien)
|NotifyOpened()| -> |GetAddress()| sequence is adjusted to support Fennec asynchronous callback.
Attachment #8674752 - Flags: review?(selin)
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).
Attachment #8674752 - Flags: review?(selin) → review+
fix nit parts, carry r+
Attachment #8674752 - Attachment is obsolete: true
Attachment #8674780 - Flags: review+
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.
Attachment #8674747 - Flags: review?(schien)
Rebase because Bug 1207245 changes the name of nsRefPtr to RefPtr.
Attachment #8674780 - Attachment is obsolete: true
Attachment #8675481 - Flags: review+
fix per comment
Attachment #8675498 - Flags: review?(schien)
Attachment #8674747 - Attachment is obsolete: true
Attachment #8675498 - Flags: review?(schien) → review+
Attachment #8672550 - Attachment is obsolete: true
Attachment #8673054 - Attachment is obsolete: true
Attachment #8673537 - Attachment is obsolete: true
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.
Attachment #8674743 - Flags: review?(rnewman) → review+
add comment and check have IPv4 address, carry r+.
Attachment #8674743 - Attachment is obsolete: true
Attachment #8675983 - Flags: review+
To make |nsIPresentationNetworkHelperListener| invisible in non-Fennec builds, move origin implementation into a wrapper class and deliver the result by callback.
Attachment #8676074 - Flags: review?(selin)
Attachment #8675481 - Attachment is obsolete: true
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.)
Attachment #8676074 - Flags: review?(selin) → review+
slightly adjust function/thread calls, carry r+
Attachment #8676074 - Attachment is obsolete: true
Attachment #8676109 - Flags: review+
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16018768&repo=mozilla-inbound
Flags: needinfo?(xeonchen)
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.
fix mochitest timeout
Attachment #8676109 - Attachment is obsolete: true
Flags: needinfo?(xeonchen)
Attachment #8676863 - Flags: review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.