Add a null check in ConnectivityManagerDelegate.getNetworkState

Add a null check in ConnectivityManagerDelegate.getNetworkState

ConnectivityManager.getNetworkInfo can return null if the Network is invalid.
This CL adds a null check so we don't crash with a NullPointerException when
invoking networkInfo.getType()

BUG=613599
Committed: https://crrev.com/ff2ec75896222f8a2ddc6b9e1551276929a0b671
Cr-Commit-Position: refs/heads/master@{#395375}

[xunjieli, 2016-05-20 15:37:46]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, pauljensen@chromium.org

[xunjieli, 2016-05-20 15:37:47]

PTAL. Next dev cut is probably next Tuesday. As long as this gets in before
that, I think we will be good.

[mef, 2016-05-20 16:31:17]

On 2016/05/20 15:37:47, xunjieli wrote:
> PTAL. Next dev cut is probably next Tuesday. As long as this gets in before
> that, I think we will be good.

Should we enter a crbug for this?

[mef, 2016-05-20 16:37:14]

https://codereview.chromium.org/2003503003/diff/1/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2003503003/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:100:
if (networkInfo.getType() == TYPE_VPN) {
I would probably just add "networkInfo != null &&" condition here instead of
duplicating logic from getNetworkState(NetworkInfo networkInfo).

[xunjieli, 2016-05-20 16:50:48]

I will create a crbug in a moment.

https://codereview.chromium.org/2003503003/diff/1/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2003503003/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:100:
if (networkInfo.getType() == TYPE_VPN) {
On 2016/05/20 16:37:14, mef wrote:
> I would probably just add "networkInfo != null &&" condition here instead of
> duplicating logic from getNetworkState(NetworkInfo networkInfo).

great idea. Done.

[xunjieli, 2016-05-20 16:55:42]

Description was changed from

==========
Add a null check in ConnectivityManagerDelegate.getNetworkState

ConnectivityManager.getNetworkInfo can return null if the Network is invalid.
This CL adds a null check so we don't crash with a NullPointerException when
invoking networkInfo.getType()
==========

to

==========
Add a null check in ConnectivityManagerDelegate.getNetworkState

ConnectivityManager.getNetworkInfo can return null if the Network is invalid.
This CL adds a null check so we don't crash with a NullPointerException when
invoking networkInfo.getType()

BUG=613599
==========

[xunjieli, 2016-05-20 16:55:53]

On 2016/05/20 16:31:17, mef wrote:
> On 2016/05/20 15:37:47, xunjieli wrote:
> > PTAL. Next dev cut is probably next Tuesday. As long as this gets in before
> > that, I think we will be good.
> 
> Should we enter a crbug for this?

Done.

[mef, 2016-05-20 20:30:18]

On 2016/05/20 16:55:53, xunjieli wrote:
> On 2016/05/20 16:31:17, mef wrote:
> > On 2016/05/20 15:37:47, xunjieli wrote:
> > > PTAL. Next dev cut is probably next Tuesday. As long as this gets in
before
> > > that, I think we will be good.
> > 
> > Should we enter a crbug for this?
> 
> Done.

Looks good. Can we add a test to NetworkChangeNotifierTest.java? 
testNetworkCallbacks seems like a good fit.

[xunjieli, 2016-05-20 21:58:43]

On 2016/05/20 20:30:18, mef wrote:
> On 2016/05/20 16:55:53, xunjieli wrote:
> > On 2016/05/20 16:31:17, mef wrote:
> > > On 2016/05/20 15:37:47, xunjieli wrote:
> > > > PTAL. Next dev cut is probably next Tuesday. As long as this gets in
> before
> > > > that, I think we will be good.
> > > 
> > > Should we enter a crbug for this?
> > 
> > Done.
> 
> Looks good. Can we add a test to NetworkChangeNotifierTest.java? 
> testNetworkCallbacks seems like a good fit.


Done. Added a test which crashed without the null check.

05-20 17:56:25.011 I/TestRunner(25880): failed:
testConnectivityManagerDelegateDoesNotCrash(org.chromium.net.NetworkChangeNotifierTest)
05-20 17:56:25.011 I/TestRunner(25880): ----- begin exception -----
05-20 17:56:25.011 I/TestRunner(25880): 
05-20 17:56:25.011 I/TestRunner(25880): java.lang.NullPointerException: Attempt
to invoke virtual method 'int android.net.NetworkInfo.getType()' on a null
object reference
05-20 17:56:25.011 I/TestRunner(25880): 	at
org.chromium.net.NetworkChangeNotifierAutoDetect$ConnectivityManagerDelegate.getNetworkState(NetworkChangeNotifierAutoDetect.java:97)
05-20 17:56:25.011 I/TestRunner(25880): 	at
org.chromium.net.NetworkChangeNotifierTest.testConnectivityManagerDelegateDoesNotCrash(NetworkChangeNotifierTest.java:697)
05-20 17:56:25.011 I/TestRunner(25880): 	at
java.lang.reflect.Method.invoke(Native Method)
05-20 17:56:25.011 I/TestRunner(25880): 	at
java.lang.reflect.Method.invoke(Method.java:372)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.test.InstrumentationTestCase.access$000(InstrumentationTestCase.java:36)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.test.InstrumentationTestCase$2.run(InstrumentationTestCase.java:189)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1871)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.os.Handler.handleCallback(Handler.java:739)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.os.Handler.dispatchMessage(Handler.java:95)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.os.Looper.loop(Looper.java:135)
05-20 17:56:25.011 I/TestRunner(25880): 	at
android.app.ActivityThread.main(ActivityThread.java:5254)
05-20 17:56:25.011 I/TestRunner(25880): 	at
java.lang.reflect.Method.invoke(Native Method)
05-20 17:56:25.011 I/TestRunner(25880): 	at
java.lang.reflect.Method.invoke(Method.java:372)
05-20 17:56:25.011 I/TestRunner(25880): 	at
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:903)
05-20 17:56:25.011 I/TestRunner(25880): 	at
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:698)
05-20 17:56:25.011 I/TestRunner(25880): ----- end exception -----

[pauljensen, 2016-05-21 00:33:04]

lgtm

https://codereview.chromium.org/2003503003/diff/40001/net/android/javatests/s...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2003503003/diff/40001/net/android/javatests/s...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:696:
Network invalidNetwork = null;
nit: instead of null, maybe you could use netIdToNetwork(NetId.INVALID)

[mef, 2016-05-22 17:08:13]

lgtm

[xunjieli, 2016-05-23 13:05:54]

https://codereview.chromium.org/2003503003/diff/40001/net/android/javatests/s...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2003503003/diff/40001/net/android/javatests/s...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:696:
Network invalidNetwork = null;
On 2016/05/21 00:33:03, pauljensen wrote:
> nit: instead of null, maybe you could use netIdToNetwork(NetId.INVALID)

Done.

[xunjieli, 2016-05-23 13:06:03]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-05-23 13:06:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2003503003/#ps60001
(title: "Address Paul's comments")

[commit-bot: I haz the power, 2016-05-23 13:06:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003503003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003503003/60001

[commit-bot: I haz the power, 2016-05-23 15:44:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 15:44:37]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xunjieli, 2016-05-23 16:45:04]

https://codereview.chromium.org/2003503003/diff/80001/net/android/javatests/s...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2003503003/diff/80001/net/android/javatests/s...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:698:
invalidNetwork = netIdToNetwork(NetId.INVALID);
Paul, netIdToNetwork uses mNetworkConstructor which is only initialized on L+.
Should I surround the call using a conditional? Paul, PTAL.

[xunjieli, 2016-05-23 17:11:54]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 17:12:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003503003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003503003/80001

[pauljensen, 2016-05-23 17:19:40]

https://codereview.chromium.org/2003503003/diff/80001/net/android/javatests/s...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2003503003/diff/80001/net/android/javatests/s...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:698:
invalidNetwork = netIdToNetwork(NetId.INVALID);
On 2016/05/23 16:45:03, xunjieli wrote:
> Paul, netIdToNetwork uses mNetworkConstructor which is only initialized on L+.
> Should I surround the call using a conditional? Paul, PTAL.

is delegate.getNetworkState(Network) callable on pre-Lollipop?
The Network class didn't even exist back then
I think we should move this whole test into the "if (Build.VERSION.SDK_INT >=
Build.VERSION_CODES.LOLLIPOP) {" block below

[xunjieli, 2016-05-23 17:27:01]

https://codereview.chromium.org/2003503003/diff/80001/net/android/javatests/s...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2003503003/diff/80001/net/android/javatests/s...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:698:
invalidNetwork = netIdToNetwork(NetId.INVALID);
On 2016/05/23 17:19:40, pauljensen wrote:
> On 2016/05/23 16:45:03, xunjieli wrote:
> > Paul, netIdToNetwork uses mNetworkConstructor which is only initialized on
L+.
> > Should I surround the call using a conditional? Paul, PTAL.
> 
> is delegate.getNetworkState(Network) callable on pre-Lollipop?
> The Network class didn't even exist back then
> I think we should move this whole test into the "if (Build.VERSION.SDK_INT >=
> Build.VERSION_CODES.LOLLIPOP) {" block below

Done.

[pauljensen, 2016-05-23 17:41:21]

lgtm

[xunjieli, 2016-05-23 17:42:11]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-05-23 17:42:11]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2003503003/#ps100001
(title: "Address comments")

[commit-bot: I haz the power, 2016-05-23 17:42:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003503003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003503003/100001

[commit-bot: I haz the power, 2016-05-23 18:50:43]

Description was changed from

==========
Add a null check in ConnectivityManagerDelegate.getNetworkState

ConnectivityManager.getNetworkInfo can return null if the Network is invalid.
This CL adds a null check so we don't crash with a NullPointerException when
invoking networkInfo.getType()

BUG=613599
==========

to

==========
Add a null check in ConnectivityManagerDelegate.getNetworkState

ConnectivityManager.getNetworkInfo can return null if the Network is invalid.
This CL adds a null check so we don't crash with a NullPointerException when
invoking networkInfo.getType()

BUG=613599
==========

[commit-bot: I haz the power, 2016-05-23 18:50:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-23 18:51:42]

Description was changed from

==========
Add a null check in ConnectivityManagerDelegate.getNetworkState

ConnectivityManager.getNetworkInfo can return null if the Network is invalid.
This CL adds a null check so we don't crash with a NullPointerException when
invoking networkInfo.getType()

BUG=613599
==========

to

==========
Add a null check in ConnectivityManagerDelegate.getNetworkState

ConnectivityManager.getNetworkInfo can return null if the Network is invalid.
This CL adds a null check so we don't crash with a NullPointerException when
invoking networkInfo.getType()

BUG=613599

Committed: https://crrev.com/ff2ec75896222f8a2ddc6b9e1551276929a0b671
Cr-Commit-Position: refs/heads/master@{#395375}
==========

[commit-bot: I haz the power, 2016-05-23 18:51:42]

Patchset 6 (id:??) landed as
https://crrev.com/ff2ec75896222f8a2ddc6b9e1551276929a0b671
Cr-Commit-Position: refs/heads/master@{#395375}