B2G Network Manager: Unable to set default route and DNS when required system properties not available

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: swu, Assigned: swu)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
The currently network manager relies on the following system properties to set default route and DNS:
    net.<ifname>.gw
    net.<ifname>.dns1
    net.<ifname>.dns2

On some partner devices, they don't provide these required system properties, and caused network failure for data call or wifi, because no default route and DNS were configured.

As we already have these information in nsINetworkInterface, it's more reasonable to use gw and dns information here instead of system property.
(Assignee)

Comment 1

5 years ago
Created attachment 678711 [details] [diff] [review]
Patch V1
Assignee: nobody → swu
Status: NEW → ASSIGNED
Attachment #678711 - Flags: review?(vyang)
Comment on attachment 678711 [details] [diff] [review]
Patch V1

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

Thank you :) I guess it's not easy to add a test case for this :(

::: dom/system/gonk/net_worker.js
@@ +163,3 @@
>    if (!options.gateway || !options.dns1_str) {
>      options = getIFProperties(options.ifname);
>    }

'getIFProperties()' could override all of the three properties. I'd rather:

  let ifprops = getIFProperties(options.ifname);
  let gateway_str = options.gateway_str || ifprops.gateway_str;
  let dns1_str = options.dns1_str || ifprops.dns1_str;
  let dns2_str = options.dns2_str || ifprops.dns2_str;
  let gateway = netHelpers.stringToIP(gateway_str);

And remove `gateway` assignment in getIFProperties() as well. Just prevent any other possible null value problems in the future.
Attachment #678711 - Flags: review?(vyang) → review+
(Assignee)

Comment 3

5 years ago
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> Comment on attachment 678711 [details] [diff] [review]
> Patch V1
> 
> Review of attachment 678711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you :) I guess it's not easy to add a test case for this :(

I did manual test, that's what we can do now.

> 
> ::: dom/system/gonk/net_worker.js
> @@ +163,3 @@
> >    if (!options.gateway || !options.dns1_str) {
> >      options = getIFProperties(options.ifname);
> >    }
> 
> 'getIFProperties()' could override all of the three properties. I'd rather:
> 
>   let ifprops = getIFProperties(options.ifname);
>   let gateway_str = options.gateway_str || ifprops.gateway_str;
>   let dns1_str = options.dns1_str || ifprops.dns1_str;
>   let dns2_str = options.dns2_str || ifprops.dns2_str;
>   let gateway = netHelpers.stringToIP(gateway_str);
> 
> And remove `gateway` assignment in getIFProperties() as well. Just prevent
> any other possible null value problems in the future.

That's a good idea, thank you.
(Assignee)

Comment 4

5 years ago
Created attachment 679037 [details] [diff] [review]
Patch V2, r=vicamo

Addressed previous review comments.
Attachment #678711 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
blocking-kilimanjaro: --- → ?
(Assignee)

Comment 5

5 years ago
Noming basecamp because the issue blocks internet access on some devices.
blocking-basecamp: --- → ?
blocking-kilimanjaro: ? → ---

Updated

5 years ago
blocking-basecamp: ? → +
https://hg.mozilla.org/mozilla-central/rev/8606dc3554fe
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba469d2bf081
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.