1000040 - [B2G][Emulator] Implement EthernetManager and test cases on KK emulator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Emulator, defect)

Description

[Ken Chang[:kenkai]]

We are implementing a new component for Ethernet Manager. That means we need to have Ethernet test cases for this component in Emulator.

Comment 1

[John Shih]

Attachment: Bug 1000040 - Part 1: Supports Ethernet in gecko.

Comment 2

[John Shih]

Attachment: Bug 1000040 - Part 2: Implement EthernetManager.

Comment 3

[John Shih]

Attachment: Bug 1000040 - Part 3: Test cases.

Comment 4

[John Shih]

Attachment: Bug 1000040 - Part 0-a: Add eth1 into emulator.

Comment 5

[John Shih]

Attachment: Bug 1000040 - Part 0-b: Add support dhcpd for eth1.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8442015 [details] [review]
Bug 1000040 - Part 0-a: Add eth1 into emulator.

Commented on GitHub.  Please move the |net_clients[nb_net_clients++] = "nic";| line after slirp iface.

Comment 7

[John Shih]

Attachment: Bug 1000040 - Part 0-a: Add eth1 into emulator.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8442007 [details] [diff] [review]
Bug 1000040 - Part 3: Test cases.

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

::: dom/network/tests/marionette/head.js
@@ +59,5 @@
> +   * @parm ifname
> +   *       Target interface name.
> +   *
> +   * @return result
> +   *         A js object of { name, flag, ip }

@return is for function return values.  Here it's a deferred promise.  Please use the same |Fulfill params: ...| format to document resolved result instead.  Applies to several places below.

@@ +61,5 @@
> +   *
> +   * @return result
> +   *         A js object of { name, flag, ip }
> +   */
> +  function getNetworkConfig(ifname) {

Can you give some example output here? Or no one would be able to know what's those regexp for. Applies to several places below.

Besides, sometimes you have an 'a' prefix for argument names, but sometimes don't. Unifying them would be really nice at the time you're creating a whole new file. Applies to several places below, too.

@@ +94,5 @@
> +   *         Ip address in string.
> +   */
> +  function getDhcpIpAddr(ifname) {
> +    return runEmulatorShellSafe(['getprop', 'dhcp.' + ifname + '.ipaddress'])
> +      .then(function (ipAddr) {

nit: no space between keyword 'function' and left parenthesis. Applies to several places below.

@@ +122,5 @@
> +   * Get the default route.
> +   *
> +   * Use shell command 'ip route' to get the default of device.
> +   *
> +   * @parm (none)

nit: you don't need this.

@@ +127,5 @@
> +   *
> +   * @return result
> +   *         a list of { name, gateway }
> +   */
> +  function getDefaultRoute() {

Examples please.

@@ +166,5 @@
> +          return;
> +        }
> +
> +        is(config.flag, INTERFACE_DOWN, "Interface is disabled as expectation.");
> +        return;

nit: an |if { ... } else { ... }| would be okay in this case.

@@ +184,5 @@
> +  function checkInterfaceIpAddr(ifname, ip) {
> +    return getNetworkConfig(ifname)
> +      .then(function (config) {
> +        is(config.ip, ip, "IP is right as expectation.");
> +        return;

nit: you don't need a return statement right before the end scope of a function.

@@ +185,5 @@
> +    return getNetworkConfig(ifname)
> +      .then(function (config) {
> +        is(config.ip, ip, "IP is right as expectation.");
> +        return;
> +      })

nit: missing ';'

@@ +216,5 @@
> +          return;
> +        }
> +
> +        throw "Default route is not set properly";
> +      })

nit: missing ';'

@@ +247,5 @@
> +   *
> +   * @return A deferred promise.
> +   */
> +  function setupInterfaces() {
> +  	let deferred = Promise.defer();

nit: indentation. Applies to several places below.

@@ +438,5 @@
> +   *
> +   * @return (none)
> +   */
> +  function interfaceConfigUpdate(ifname, config) {
> +    ethernetManager.interfaceConfigUpdate(ifname, config);

method names should be better a verb or an action, so this should probably be 'updateInterfaceConfig' rather than 'interfaceConfigUpdate'.

@@ +474,5 @@
> +   * Reject params: (none)
> +   *
> +   * @return A deferred promise.
> +   */
> +  function cleanUp() {

It does not return a deferred promise anyway. Please correct the documentation above. Besides, returning a promise inside a callback to waitFor() is also meaningless. Just have:

  waitFor(finish, function() {
    return pendingEmulatorShellCount === 0;
  });

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8442589 [details] [review]
Bug 1000040 - Part 0-a: Add eth1 into emulator.

r+ with a nit.

Comment 10

[John Shih]

Attachment: Bug 1000040 - Part 3: Test cases.

Patch updates with addressing the comment 8.

Comment 11

[John Shih]

Attachment: Bug 1000040 - Part 3: Test cases.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8442684 [details] [diff] [review]
Bug 1000040 - Part 3: Test cases.

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

::: dom/network/tests/marionette/head.js
@@ +53,5 @@
> +   * Get the system network conifg by the given interface name.
> +   *
> +   * Use shell command 'netcfg' to get the list of network cofig.
> +   *
> +   * Fulfill params: A object of { name, flag, ip }

nit: An object ...

@@ +120,5 @@
> +   * @return A deferred promise.
> +   */
> +  function getDhcpGateway(ifname) {
> +      return runEmulatorShellSafe(['getprop', 'dhcp.' + ifname + '.gateway'])
> +        .then(function(gateway) {

nit: indentation

@@ +252,5 @@
> +  function setupInterfaces() {
> +  	let deferred = Promise.defer();
> +
> +  	ethernetManager.setupInterfaces(function onSetup(success, message) {
> +  	  ok(success, "setupInterfaces success.");

nit: indentation.

@@ +422,5 @@
> +   *        .usingDhcp: an boolean value indicates using dhcp or not.
> +   */
> +  function updateInterfaceConfig(ifname, config) {
> +    ethernetManager.interfaceConfigUpdate(ifname, config);
> +    return;

nit: redundant return statement.

::: dom/network/tests/marionette/test_ethernet_connect_with_static_ip.js
@@ +14,5 @@
> +};
> +
> +function checkStaticResult(ifname) {
> +  let dhcpIp;
> +  let dhcpGateway;

nit: unused variables.

::: dom/network/tests/marionette/test_ethernet_disconnect.js
@@ +8,5 @@
> +const INTERFACE_IP_NONE = "0.0.0.0";
> +
> +function checkIpAddrIsReset(ifname) {
> +  let dhcpIp;
> +  let dhcpGateway;

ditto.

::: dom/network/tests/marionette/test_ethernet_reconnect.js
@@ +14,5 @@
> +};
> +
> +function checkStaticResult(ifname) {
> +  let dhcpIp;
> +  let dhcpGateway;

yet again.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8442006 [details] [diff] [review]
Bug 1000040 - Part 2: Implement EthernetManager.

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

::: dom/network/interfaces/nsIEthernetManager.idl
@@ +26,5 @@
> +{
> +  /**
> +   * List of exisiting interface name.
> +   */
> +  attribute jsval interfaceList;

readonly ...

@@ +76,5 @@
> +   *        .httpProxyPort: http porxy port.
> +   *        .usingDhcp: an boolean value indicates using dhcp or not.
> +   */
> +  void interfaceConfigUpdate(in DOMString ifname,
> +                             in jsval config);

updateInterfaceConfig

@@ +131,5 @@
> +
> +  /**
> +   * Service shout down.
> +   */
> +  void shutdown();

We don't have to expose shutdown() as an interface method.

::: dom/network/src/EthernetManager.js
@@ +75,5 @@
> +  isUsingDhcp: function() {
> +    return this.usingDhcp;
> +  },
> +
> +  getName: function () {

nit: 'function()'

@@ +111,5 @@
> +function EthernetManager() {
> +  debug("EthernetManager start");
> +
> +  // Interface list.
> +  this.ethernetInterfaces = Object.create(null);

|this.ethernetInterfaces = {};|

Please also place a |ethernetInterfaces: null,| in |EthernetManager.prototype|.

@@ +146,5 @@
> +      }
> +
> +      index += 1;
> +      this.removeInterface.call(this, ifnameList[index], onRemove);
> +    }.bind(this));

(function onremove(ifNameList) {
  if (!ifNameList.length) {
    return;
  }

  let ifName = ifNameList.shift();
  this.removeInterface(ifName, onremove.bind(this, ifNameList));
}).call(this, Object.keys(this.ethernetInterfaces));

@@ +158,5 @@
> +    debug("setupInterfaces");
> +
> +    gNetworkService.getInterfaces(function(success, list) {
> +      if (!success) {
> +        return;

Report failure if callback is passed.

@@ +164,5 @@
> +
> +      let ethList = [];
> +      for (let i = 0; i < list.length; i++) {
> +        debug("Found interface " + list[i]);
> +        if (!list[i].match(ETHERNET_NETWORK_IFACE_PREFIX)) {

if (!list[i].startsWith(ETHERNET_NETWORK_IFACE_PREFIX))

@@ +172,5 @@
> +      }
> +
> +      let index = 0;
> +      this.addInterface(ethList[index],
> +                        function onInterfaceAdded(success, result) {

What if we fail to add some of the interfaces? Or, why do we have to configure all interfaces at once?

@@ +191,5 @@
> +    debug("addInterfaces " + ifname);
> +
> +    // Callback may also be used internally, so make callback a consistent form.
> +    if (callback.notify) {
> +      callback = callback.notify;

I don't understand what does this mean. Don't try to alter the data type or meanings of a variable in the middle of your program for that only brings you more chaotic. Always use |callback.notify(...)|.

@@ +194,5 @@
> +    if (callback.notify) {
> +      callback = callback.notify;
> +    }
> +
> +    if (!ifname || !ifname.match(ETHERNET_NETWORK_IFACE_PREFIX)) {

ifname.startsWith(...)

@@ +247,5 @@
> +    if (callback.notify) {
> +      callback = callback.notify;
> +    }
> +
> +    if (!ifname || !ifname.match(ETHERNET_NETWORK_IFACE_PREFIX)) {

ditto.

@@ +261,5 @@
> +      }
> +      return;
> +    }
> +
> +    // Make sure interface is diable before removing.

nit: disabled

@@ +303,5 @@
> +    let iface = this.ethernetInterfaces[ifname];
> +    if (!iface ||
> +        iface.getState() == Ci.nsINetworkInterface.NETWORK_STATE_ENABLED) {
> +      if (callback) {
> +        callback.notify(true, "already enabled.");

Please still report an error while invalid ifname was passed.

@@ +351,5 @@
> +    let iface = this.ethernetInterfaces[ifname];
> +    if (!iface ||
> +        iface.getState() == Ci.nsINetworkInterface.NETWORK_STATE_DISABLED) {
> +      if (callback) {
> +        callback(true, "Interface is already disable");

ditto.

@@ +398,5 @@
> +    let iface = this.ethernetInterfaces[ifname];
> +    if (!iface ||
> +        iface.getState() == Ci.nsINetworkInterface.NETWORK_STATE_DISABLED) {
> +      if (callback) {
> +        callback.notify(true, "Interface " + ifname + " is not available.");

ditto.  You can really extract these duplicated lines into an utility function:

  _ensureIfName: function(ifname, callback, func) {
    // If no given ifname, use the default one.
    if (!ifname) {
      ifname = DEFAULT_ETHERNET_NETWORK_IFACE;
    }

    let iface = this.ethernetInterfaces[ifname];
    if (!iface) {
      if (callback) {
        callback.notify(true, "Interface " + ifname + " is not available.");
      }
      return;
    }

    func.call(this, iface);
  },

  connect: function(ifname, config, callback) {
    _ensureIfName(ifname, callback, function(iface) {
      ...
    });
  }

@@ +494,5 @@
> +  _runDhcp: function(ifname, callback) {
> +    debug("runDhcp with " + ifname);
> +
> +    if (!callback) {
> +      return;

Why?

@@ +530,5 @@
> +  },
> +
> +  _setStaticIP: function(ifname, config, callback) {
> +    if (!callback) {
> +      return;

ditto.

::: dom/system/gonk/NetworkManager.js
@@ +363,5 @@
>    getNetworkId: function(network) {
>      let id = "device";
> +    if (this.isNetworkTypeEthernet(network.type)) {
> +      id = network.name.substring(3, 4);
> +    }

if (this.isNetworkTypeEthernet(network.type)) {
  id = network.name.substring(3);
} else if ...

@@ +430,1 @@
>  #endif

Let's eliminate one MOZ_B2G_RIL usage here.  NETWORK_TYPE_MOBILE is always defined in xpidl.

  if ([Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
       Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE,
       Ci.nsINetworkInterface.NETWORK_TYPE_ETHERNET].indexOf(val) == -1)

Comment 14

[John Shih]

Attachment: Bug 1000040 - Part 3: Test cases.

Patch update according to the change of Part 2.

Comment 15

[John Shih]

Attachment: Bug 1000040 - Part 2: Implement EthernetManager.

Patch updated.

Comment 16

[John Shih]

Attachment: Bug 1000040 - Part 2: Implement EthernetManager.

Fix nits.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8444187 [details] [diff] [review]
Bug 1000040 - Part 3: Test cases.

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

Please, check, you, editor's, indentation, settings, again.

::: dom/network/tests/marionette/head.js
@@ +269,5 @@
> +   *
> +   * @return A deferred promise.
> +   */
> +  function scanInterfaces() {
> +  	let deferred = Promise.defer();

indentation.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8444189 [details] [diff] [review]
Bug 1000040 - Part 2: Implement EthernetManager.

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

Please only request for review with all previous comments addressed.

::: dom/network/src/EthernetManager.js
@@ +72,5 @@
> +                       config.usingDhcp : this.usingDhcp;
> +  },
> +
> +  isUsingDhcp: function() {
> +    return this.usingDhcp;

This is neither an interface method, nor some complicated procedure.  Please use |this.usingDhcp| directly.

@@ +76,5 @@
> +    return this.usingDhcp;
> +  },
> +
> +  getName: function() {
> +    return this.name;

So is this. It's a readonly interface attribute, so you can access |iface.name| directly.

@@ +80,5 @@
> +    return this.name;
> +  },
> +
> +  getState: function() {
> +    return this.state;

ditto.

@@ +193,5 @@
> +      }
> +      return;
> +    }
> +
> +    gNetworkService.getInterfaceConfig(ifname, function (success, result) {

comment 13.

@@ +242,5 @@
> +      return;
> +    }
> +
> +    // Make sure interface is disable before removing.
> +    this.disable(ifname, { notify: function (success, message) {

ditto.

::: dom/system/gonk/NetworkManager.js
@@ +445,1 @@
>  #endif

You have to remove "#ifdef MOZ_B2G_RIL" and "#endif", too.

Comment 19

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8444189 [details] [diff] [review]
Bug 1000040 - Part 2: Implement EthernetManager.

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

::: dom/network/interfaces/nsIEthernetManager.idl
@@ +56,5 @@
> +   *        Interface name. Should be the form of "eth*".
> +   * @param callback
> +   *        Callback function.
> +   */
> +  void addInterface(in DOMString ifname,

How do you think if we add one more parameter to indicate how to get the ip address for this interface?

@@ +83,5 @@
> +   *        .gateway: gateway.
> +   *        .dnses: dnses.
> +   *        .httpProxyHost: http proxy host.
> +   *        .httpProxyPort: http porxy port.
> +   *        .usingDhcp: an boolean value indicates using dhcp or not.

I would suggest to use more generic term for this s/usingDhcp/ipMode.

@@ +124,5 @@
> +   * @param callback
> +   *        Callback function.
> +   */
> +  void connect(in DOMString ifname,
> +               in jsval config,

Do you still need config, you can get it according to ifname internally?

::: dom/network/src/EthernetManager.js
@@ +56,5 @@
> +
> +  updateConfig: function(config) {
> +    debug("Interface " + this.name + " updateConfig " + JSON.stringify(config));
> +    this.state = (config.state != undefined) ?
> +                 config.state : this.state;

Nit: please align this line to the position of first character in above line.

@@ +59,5 @@
> +    this.state = (config.state != undefined) ?
> +                 config.state : this.state;
> +    this.ips = (config.ip != undefined) ? [config.ip] : this.ips;
> +    this.prefixLengths = (config.prefixLength != undefined) ?
> +                        [config.prefixLength] : this.prefixLengths;

Ditto.

@@ +61,5 @@
> +    this.ips = (config.ip != undefined) ? [config.ip] : this.ips;
> +    this.prefixLengths = (config.prefixLength != undefined) ?
> +                        [config.prefixLength] : this.prefixLengths;
> +    this.gateways = (config.gateway != undefined) ?
> +                    [config.gateway] : this.gateways;

Ditto.

@@ +64,5 @@
> +    this.gateways = (config.gateway != undefined) ?
> +                    [config.gateway] : this.gateways;
> +    this.dnses = (config.dnses != undefined) ? config.dnses : this.dnses;
> +    this.httpProxyHost = (config.httpProxyHost != undefined) ?
> +                         config.httpProxyHost : this.httpProxyHost;

Ditto.

@@ +66,5 @@
> +    this.dnses = (config.dnses != undefined) ? config.dnses : this.dnses;
> +    this.httpProxyHost = (config.httpProxyHost != undefined) ?
> +                         config.httpProxyHost : this.httpProxyHost;
> +    this.httpProxyPort = (config.httpProxyPort != undefined) ?
> +                         config.httpProxyPort : this.httpProxyPort;

Ditto.

@@ +68,5 @@
> +                         config.httpProxyHost : this.httpProxyHost;
> +    this.httpProxyPort = (config.httpProxyPort != undefined) ?
> +                         config.httpProxyPort : this.httpProxyPort;
> +    this.usingDhcp = (config.usingDhcp != undefined) ?
> +                       config.usingDhcp : this.usingDhcp;

Ditto.

@@ +196,5 @@
> +
> +    gNetworkService.getInterfaceConfig(ifname, function (success, result) {
> +      // Since the operation will still success with an invalid interface name,
> +      // check the mac address as well.
> +      if (!success || result.macAddr == INTERFACE_MACADDR_NULL) {

Since your first parameter has indicated the return status of the command, why not just check the result inside getInterfaceConfig()?

@@ +351,5 @@
> +      }.bind(this));
> +    }.bind(this));
> +  },
> +
> +  connect: function(ifname, config, callback) {

You could get config from internal data structure, right?

@@ +363,5 @@
> +        return;
> +      }
> +
> +      if (iface.getState() == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +        this._reconnect(iface, config, callback);

It's weird to reconnect while it is still in connected state. It's more like to update the configuration for specific interface, right? 
We may do this in updateInterfaceConfig, or rename here. It's easier to do update and connect within one function.

::: dom/system/gonk/NetworkManager.js
@@ +422,5 @@
>    _dataDefaultServiceId: null,
>  
> +  _networkTypePriorityList: [Ci.nsINetworkInterface.NETWORK_TYPE_ETHERNET,
> +                             Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                             Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE],

It would be nice if we can modify the default priority list. Different platform might have their prefered list.

@@ +425,5 @@
> +                             Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                             Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE],
> +  getPriority: function(type) {
> +    if (this._networkTypePriorityList.indexOf(type) == -1) {
> +      return 0;

Return 0 means highest priority, right? Is it your intention?

Comment 20

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8444187 [details] [diff] [review]
Bug 1000040 - Part 3: Test cases.

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

You have gotten the answer. Cancel the review request for now.

Comment 21

[John Shih]

Attachment: Bug 1000040 - Part 3: Test cases.

Patch update with addressing nits and add some additional cases.

Comment 22

[John Shih]

Attachment: Bug 1000040 - Part 3: Test cases.

Comment 23

[John Shih]

Attachment: Bug 1000040 - Part 2: Implement EthernetManager.

Patch update.

Comment 24

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8442003 [details] [diff] [review]
Bug 1000040 - Part 1: Supports Ethernet in gecko.

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

Looks good. Thank you.

Comment 25

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8447597 [details] [diff] [review]
Bug 1000040 - Part 2: Implement EthernetManager.

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

Looks good. Thank you.

Comment 27

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8447596 [details] [diff] [review]
Bug 1000040 - Part 3: Test cases.

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

Looks good. But you still need Vicamo's approval.

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

https://github.com/mozilla-b2g/device_generic_goldfish/commit/f486d0316aef2b3dea772dfb186e447e7a6a5de5
https://github.com/mozilla-b2g/platform_external_qemu/commit/bf9aaf39dd5a6491925a022db167c460f8207d34

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=7ec4d8b9649d

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8442003 [details] [diff] [review]
Bug 1000040 - Part 1: Supports Ethernet in gecko.

Hi Bobby, this part touches a webidl file (dom/webidl/NetworkOptions.webidl) and needs DOM peers' review.  It used to be reviewed by fabrice, hsinyi, vchang, and me, but none of us is a DOM peer.

The dictionary definitions inside NetworkOptions.webidl are only referenced by internal native components for convenience's sake and are never ever used in content pages.  Here we need additional attributes to support new interfaces for a new feature -- ethernet support in Firefox OS.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 8442003 [details] [diff] [review]
Bug 1000040 - Part 1: Supports Ethernet in gecko.

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

r=me on the webidl.

::: dom/webidl/NetworkOptions.webidl
@@ +50,1 @@
>    DOMString startIp;                  // for "setDhcpServer".

The comment at the top of this file has a typo - s/dictionnary/dictionary/.

Comment 31

[John Shih]

Attachment: Bug 1000040 - Part 1: Supports Ethernet in gecko. r=vchang, bholley

Fix typo and remove unused code.

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f54af6237ce9
https://hg.mozilla.org/integration/b2g-inbound/rev/78ba6f47a5a7
https://hg.mozilla.org/integration/b2g-inbound/rev/506038ddee2c

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f54af6237ce9
https://hg.mozilla.org/mozilla-central/rev/78ba6f47a5a7
https://hg.mozilla.org/mozilla-central/rev/506038ddee2c

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e2d7cfb04108
https://hg.mozilla.org/integration/b2g-inbound/rev/c40d7e27cff5
https://hg.mozilla.org/integration/b2g-inbound/rev/dea2d80c1c70

Comment 35

[Carsten Book [:Tomcat]]

merged to central and will merge this also to the other integration trees
https://hg.mozilla.org/mozilla-central/rev/e2d7cfb04108
https://hg.mozilla.org/mozilla-central/rev/c40d7e27cff5
https://hg.mozilla.org/mozilla-central/rev/dea2d80c1c70

Comment 36

[Jeff Chuang]

Dear all,

I would like to enable eth0 in Octopus project[Bug 1000664]. I found the EthernetManager would not be not initialized with the patches. Comparing to the implementations of Wifi and Ethernet Bug 969268, they both have a worker implementation and are init in SystemManagerWorker. Do I need to add EthernetWorker to init EthernetManager if I want EthernetManager to work in b2g?
Thank you

Comment 37

[John Shih]

(In reply to Jeff Chuang from comment #36)
> Dear all,
> 
> I would like to enable eth0 in Octopus project[Bug 1000664]. I found the
> EthernetManager would not be not initialized with the patches. Comparing to
> the implementations of Wifi and Ethernet Bug 969268, they both have a worker
> implementation and are init in SystemManagerWorker. Do I need to add
> EthernetWorker to init EthernetManager if I want EthernetManager to work in
> b2g?

No you don't. New worker is not needed anymore since we use NetworkWorker directly.

We do not do any initialization in EthernetManager. Our plan is leaving interfaces managed by NetworkManager and have a mozNetworkManager API to expose the control operation. So you may need to wait for the follow-up work (Bug 922584) being done.

> Thank you

Comment 38

[Ken Chang[:kenkai]]

John, do you want to keep taking this bug? If not, I want to tell Ethan to take over this bug.

Comment 39

[John Shih]

Hi Ken,
I will try to find where the problem is! Will ask for Ethan's help if found difficulties.

Comment 40

[Edgar Chen [:edgar]]

Just let you know, we are now working on new branching model for external/qemu (please see bug 1163968). After that emulator-ics will not use `master` any more, but `b2g-ics`.

Comment 41

[Shian-Yow Wu [:swu]]

(In reply to Edgar Chen [:edgar][:echen] from comment #40)
> Just let you know, we are now working on new branching model for
> external/qemu (please see bug 1163968). After that emulator-ics will not use
> `master` any more, but `b2g-ics`.

Thanks for this info.  It should make more sense to re-work this bug on KK, changed the summary as such.

Comment 42

[Shian-Yow Wu [:swu]]

Taking it per discussion with Ethan.

Comment 43

[Ethan Tseng [:ethan]]

Glad to see this has progress.
Shian-Yow, thank you for taking this bug.

Comment 44

[Shian-Yow Wu [:swu]]

Attachment: Part 1(for KK): Add required APIs for Ethernet.

Comment 45

[Shian-Yow Wu [:swu]]

Attachment: Part 2(for KK): Implement EthernetManager.

Comment 46

[Shian-Yow Wu [:swu]]

Attachment: Part 3(for KK): Test cases.

Comment 47

[Shian-Yow Wu [:swu]]

The new patches are re-factored for current network manager architecture, and tested on emulator-kk-x86.

There is a problem happens occasionally when running marionette test cases, as the error message below.  When connecting with DHCP, sometimes the callback function of _runDhcp() cannot be run(the NS_NOINTERFACE error).  It looks like the callback function was been GCed, this is strange!
It can be always reproduced if we do a |Components.utils.forceGC| before calling callback.notify() in _runDhcp().

05-22 09:38:59.411 I/Gecko   (  984): -*- EthernetManager: Connect interface eth1
05-22 09:38:59.411 I/Gecko   (  984): -*- EthernetManager: runDhcp with eth1
05-22 09:38:59.411 I/Gecko   (  984): -*- NetworkService: NetworkManager received message from worker: {"broadcast":true,"curExternalIfname":"","curInternalIfname":"","dns1":0,"dns1_str":"","dns2":0,"dns2_str":"","enable":false,"error":false,"flag":"down","gateway":0,"gateway_str":"","id":0,"ipAddr":"","ipaddr":0,"ipaddr_str":"","lease":0,"macAddr":"","mask":0,"mask_str":"","netId":"","prefixLength":0,"reason":"Iface linkstate eth1 up","reply":"","result":false,"resultCode":0,"resultReason":"","ret":false,"route":"","server":0,"server_str":"","success":false,"topic":"netd-interface-change","vendor_str":""}
05-22 09:38:59.841 I/Gecko   (  984): -*- NetworkService: NetworkManager received message from worker: {"broadcast":false,"curExternalIfname":"","curInternalIfname":"","dns1":50462730,"dns1_str":"10.0.2.3","dns2":0,"dns2_str":"","enable":false,"error":false,"flag":"down","gateway":33685514,"gateway_str":"10.0.2.2","id":107,"ipAddr":"","ipaddr":251789322,"ipaddr_str":"10.0.2.15","lease":86400,"macAddr":"","mask":16777215,"mask_str":"255.255.255.0","netId":"","prefixLength":24,"reason":"","reply":"","result":false,"resultCode":0,"resultReason":"","ret":false,"route":"","server":33685514,"server_str":"10.0.2.2","success":false,"topic":"","vendor_str":""}
05-22 09:38:59.841 I/Gecko   (  984): -*- EthernetManager: DHCP succeeded with {"broadcast":false,"curExternalIfname":"","curInternalIfname":"","dns1":50462730,"dns1_str":"10.0.2.3","dns2":0,"dns2_str":"","enable":false,"error":false,"flag":"down","gateway":33685514,"gateway_str":"10.0.2.2","id":107,"ipAddr":"","ipaddr":251789322,"ipaddr_str":"10.0.2.15","lease":86400,"macAddr":"","mask":16777215,"mask_str":"255.255.255.0","netId":"","prefixLength":24,"reason":"","reply":"","result":false,"resultCode":0,"resultReason":"","ret":false,"route":"","server":33685514,"server_str":"10.0.2.2","success":false,"topic":"","vendor_str":""}
05-22 09:38:59.841 I/Gecko   (  984): -*- EthernetManager: Interface eth1 updateConfig {"state":1,"ip":"10.0.2.15","gateway":"10.0.2.2","prefixLength":24,"dnses":["10.0.2.3",""]}
05-22 09:38:59.841 I/Gecko   (  984): -*- NetworkManager: Network 7/eth1 changed state to 1
05-22 09:38:59.841 I/Gecko   (  984): -*- EthernetManager: Connect interface eth1 with DHCP succeeded.
05-22 09:38:59.841 E/GeckoConsole(  984): [JavaScript Error: "NS_NOINTERFACE: Component does not have requested interface [nsIEthernetManagerCallback.notify]" {file: "jar:file:///system/b2g/omni.ja!/components/EthernetManager.js" line: 592}]
05-22 09:38:59.841 E/GeckoConsole(  984): [JavaScript Error: "NS_NOINTERFACE: Component does not have requested interface [nsIEthernetManagerCallback.notify]" {file: "jar:file:///system/b2g/omni.ja!/components/EthernetManager.js" line: 592}]

Have to figure out the problem before asking review.

Comment 48

[Shian-Yow Wu [:swu]]

It seems reproducible by calling Components.utils.forceGC() before any callback.notify() in EthernetManager.js, not just _runDhcp().

Backtrace as below when calling scan().

#0  nsScriptError::ToString (this=0xa3a44c10, aResult=...)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/nsScriptError.cpp:247
#1  0xb3d8fa25 in LogMessageWithMode (aOutputMode=nsConsoleService::OutputToLog, aMessage=0xa3a44c10, 
    this=0xb705bc40) at /home/sywu/work/mozilla-central/xpcom/base/nsConsoleService.cpp:212
#2  nsConsoleService::LogMessageWithMode (this=0xb705bc40, aMessage=0xa3a44c10, 
    aOutputMode=nsConsoleService::OutputToLog)
    at /home/sywu/work/mozilla-central/xpcom/base/nsConsoleService.cpp:175
#3  0xb3d8fbd3 in nsConsoleService::LogMessage (this=0xb705bc40, aMessage=0xa3a44c10)
    at /home/sywu/work/mozilla-central/xpcom/base/nsConsoleService.cpp:171
#4  0xb4186b01 in xpc::ErrorReport::LogToConsole (this=0xa1948560)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/nsXPConnect.cpp:267
#5  0xb44c093e in xpc::SystemErrorReporter (cx=0xb70311b0, 
    message=0xa38b8c80 "NS_NOINTERFACE: Component does not have requested interface [nsIEthernetManagerScanCallback.notify]", report=0xbfde658c) at /home/sywu/work/mozilla-central/dom/base/nsJSEnvironment.cpp:508
#6  0xb5b9c108 in js::CallErrorReporter (cx=0xb70311b0, 
    message=0xa38b8c80 "NS_NOINTERFACE: Component does not have requested interface [nsIEthernetManagerScanCallback.notify]", reportp=0xbfde658c) at /home/sywu/work/mozilla-central/js/src/jscntxt.cpp:815
#7  0xb5c1599e in js::ReportUncaughtException (cx=0xb70311b0)
    at /home/sywu/work/mozilla-central/js/src/jsexn.cpp:645
#8  0xb4188639 in nsXPCWrappedJSClass::CheckForException (ccx=..., 
    aPropertyName=0xb1b3b3e0 "getInterfacesResult", anInterfaceName=0xa1958020 "nsIGetInterfacesCallback", 
    aForceReport=true) at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedJSClass.cpp:800
#9  0xb4195896 in nsXPCWrappedJSClass::CallMethod (this=0xa5ab26d0, wrapper=0xa71f7b80, methodIndex=3, 
    info_=0xb1b3b3c8, nativeParams=0xbfde6a80)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedJSClass.cpp:1245
#10 0xb41962a1 in CallMethod (params=0xbfde6a80, info=0xb1b3b3c8, methodIndex=3, this=0xa71f7b80)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedJS.cpp:525
#11 nsXPCWrappedJS::CallMethod (this=0xa71f7b80, methodIndex=3, info=0xb1b3b3c8, params=0xbfde6a80)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedJS.cpp:515
#12 0xb3dcfab5 in PrepareAndDispatch (methodIndex=<optimized out>, self=0xacbd9ff0, args=<optimized out>)
    at ../../../../../../xpcom/reflect/xptcall/md/unix/xptcstubs_gcc_x86_unix.cpp:60
#13 0xb3dcefa8 in NS_InvokeByIndex ()
   from /home/sywu/work/mozilla-central/objdir-gonk-emulator-x86-kk/dist/bin/libxul.so
#14 0xb4192a0b in Invoke (this=0xbfde6c20)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2080
#15 Call (this=0xbfde6c20) at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1417
#16 XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1384
#17 0xb41930e6 in XPC_WN_CallMethod (cx=0xb70311b0, argc=2, vp=0xb17c5130)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1144
#18 0xb58ec44a in CallJSNative (args=<optimized out>, native=<optimized out>, cx=<optimized out>)
    at ../../../js/src/jscntxtinlines.h:235
#19 js::Invoke (cx=0xb70311b0, args=..., construct=js::NO_CONSTRUCT)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:727
#20 0xb58e786c in Interpret (cx=0xb70311b0, state=...)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:2956
#21 0xb58ebf63 in js::RunScript (cx=0xb70311b0, state=...)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:677
#22 0xb58ec3c7 in js::Invoke (cx=0xb70311b0, args=..., construct=js::NO_CONSTRUCT)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:747
#23 0xb5bd37c7 in js::fun_call (cx=0xb70311b0, argc=2, vp=0xb17c50b8)
    at /home/sywu/work/mozilla-central/js/src/jsfun.cpp:1216
#24 0xb58ec44a in CallJSNative (args=<optimized out>, native=<optimized out>, cx=<optimized out>)
    at ../../../js/src/jscntxtinlines.h:235
#25 js::Invoke (cx=0xb70311b0, args=..., construct=js::NO_CONSTRUCT)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:727
#26 0xb58e786c in Interpret (cx=0xb70311b0, state=...)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:2956
#27 0xb58ebf63 in js::RunScript (cx=0xb70311b0, state=...)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:677
#28 0xb58ec3c7 in js::Invoke (cx=0xb70311b0, args=..., construct=js::NO_CONSTRUCT)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:747
#29 0xb58ecf39 in js::Invoke (cx=0xb70311b0, thisv=..., fval=..., argc=1, argv=0xbfde8a20, rval=...)
    at /home/sywu/work/mozilla-central/js/src/vm/Interpreter.cpp:784
#30 0xb5b9cf31 in JS_CallFunctionValue (cx=0xb70311b0, obj=..., fval=..., args=..., rval=...)
    at /home/sywu/work/mozilla-central/js/src/jsapi.cpp:4409
#31 0xb419578c in nsXPCWrappedJSClass::CallMethod (this=0xacb6f970, wrapper=0xaf2ec380, methodIndex=3, 
    info_=0xb1b459a8, nativeParams=0xbfde8b60)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedJSClass.cpp:1212
#32 0xb41962a1 in CallMethod (params=0xbfde8b60, info=0xb1b459a8, methodIndex=3, this=0xaf2ec380)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedJS.cpp:525
#33 nsXPCWrappedJS::CallMethod (this=0xaf2ec380, methodIndex=3, info=0xb1b459a8, params=0xbfde8b60)
    at /home/sywu/work/mozilla-central/js/xpconnect/src/XPCWrappedJS.cpp:515
#34 0xb3dcfab5 in PrepareAndDispatch (methodIndex=<optimized out>, self=0xacb88440, args=<optimized out>)
    at ../../../../../../xpcom/reflect/xptcall/md/unix/xptcstubs_gcc_x86_unix.cpp:60
#35 0xb4d482dd in mozilla::NetworkWorker::DispatchNetworkResult (this=0xacb88410, aOptions=...)
    at /home/sywu/work/mozilla-central/dom/system/gonk/NetworkWorker.cpp:239
#36 0xb4d48335 in mozilla::NetworkResultDispatcher::Run (this=0xaa7dbbc0)
    at /home/sywu/work/mozilla-central/dom/system/gonk/NetworkWorker.cpp:46
#37 0xb3dc823c in nsThread::ProcessNextEvent (this=0xb7002630, aMayWait=false, aResult=0xbfde8d2f)
    at /home/sywu/work/mozilla-central/xpcom/threads/nsThread.cpp:846
#38 0xb3de2ab0 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=false)
    at /home/sywu/work/mozilla-central/xpcom/glue/nsThreadUtils.cpp:265
#39 0xb3fbe080 in mozilla::ipc::MessagePump::Run (this=0xb70ff100, aDelegate=0xb1bbe1a0)
    at /home/sywu/work/mozilla-central/ipc/glue/MessagePump.cpp:95
#40 0xb3fa5c62 in MessageLoop::RunInternal (this=0xb1bbe1a0)
    at /home/sywu/work/mozilla-central/ipc/chromium/src/base/message_loop.cc:233
#41 0xb3fa5d90 in RunHandler (this=0xb1bbe1a0)
    at /home/sywu/work/mozilla-central/ipc/chromium/src/base/message_loop.cc:226
#42 MessageLoop::Run (this=0xb1bbe1a0)
    at /home/sywu/work/mozilla-central/ipc/chromium/src/base/message_loop.cc:200
#43 0xb4e97ae5 in nsBaseAppShell::Run (this=0xb170d6a0)
    at /home/sywu/work/mozilla-central/widget/nsBaseAppShell.cpp:165
#44 0xb537b89f in nsAppStartup::Run (this=0xb1771910)
    at /home/sywu/work/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:280
#45 0xb53ade15 in XREMain::XRE_mainRun (this=0xbfde8f84) at ../../../toolkit/xre/nsAppRunner.cpp:4244
#46 0xb53ae0fc in XREMain::XRE_main (this=0xbfde8f84, argc=1, argv=0xb7004438, aAppData=0xb772fd58)
    at ../../../toolkit/xre/nsAppRunner.cpp:4328
#47 0xb53ae2fc in XRE_main (argc=1, argv=0xb7004438, aAppData=0xb772fd58, aFlags=0)
    at ../../../toolkit/xre/nsAppRunner.cpp:4417
#48 0xb7700791 in do_main (argc=1, argv=0xb7004438) at ../../../b2g/app/nsBrowserApp.cpp:167
#49 0xb770090d in b2g_main (argc=1, argv=0xbfdea2e4) at ../../../b2g/app/nsBrowserApp.cpp:299
#50 0xb7700534 in RunProcesses (aReservedFds=..., argv=0xbfdea2e4, argc=1)
    at ../../../b2g/app/B2GLoader.cpp:232
#51 main (argc=1, argv=0xbfdea2e4) at ../../../b2g/app/B2GLoader.cpp:297

Comment 49

[Henry Chang [:hchang]]

Attachment: Rebased Part 1

Comment 50

[Henry Chang [:hchang]]

Attachment: Rebased Part 2

Comment 51

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 1 (v2)

Comment 52

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 2 (v2)

Comment 53

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 1 (v3)

Comment 54

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 2 (v3)

Comment 55

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 3 (v3)

Comment 56

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 1 (v4): Add required APIs for Ethernet.

rebase only, carry r+

Comment 57

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 2 (v4): Implement EthernetManager.

rebase only, carry r+

Comment 58

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 3 (v4): Test cases.

rebase only, carry r+

Comment 59

[Gary Chen [:xeonchen]]

The tests are passed after rebasing to latest m-c.
Please help to check-in following patches:

attachment 8720192 [details] [diff] [review]
attachment 8720193 [details] [diff] [review]
attachment 8720194 [details] [diff] [review]

Comment 60

[Gary Chen [:xeonchen]]

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

Comment 61

[Gary Chen [:xeonchen]]

Attachment: Rebased Part 1 (v4.1): Add required APIs for Ethernet.

add r=bholley since he had reviewed origin patch. carry r+

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39a61e204d17
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0af4a677f59
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2b8087bd78

Comment 63

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/39a61e204d17
https://hg.mozilla.org/mozilla-central/rev/b0af4a677f59
https://hg.mozilla.org/mozilla-central/rev/ef2b8087bd78