889094 - java.lang.NullPointerException: at org.mozilla.gecko.BrowserToolbar.updateFocusOrder(BrowserToolbar.java)

Status: RESOLVED FIXED

Description

[Scoobidiver (away)]

There's one crash in 25.0a1/20130701: bp-28431fe0-ecff-4403-aefc-8fb392130701.

It might be related to bug 888641.

java.lang.NullPointerException
	at org.mozilla.gecko.BrowserToolbar.updateFocusOrder(BrowserToolbar.java:877)
	at org.mozilla.gecko.BrowserToolbar.setPageActionVisibility(BrowserToolbar.java:839)
	at org.mozilla.gecko.BrowserToolbar.setSecurityMode(BrowserToolbar.java:967)
	at org.mozilla.gecko.BrowserToolbar.onTabChanged(BrowserToolbar.java:469)
	at org.mozilla.gecko.Tabs$5.run(Tabs.java:550)
	at android.os.Handler.handleCallback(Handler.java:615)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4745)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?product=FennecAndroid&signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.BrowserToolbar.updateFocusOrder%28BrowserToolbar.java%29

Comment 1

[Michal Kajda [:mkajda]]

As I was working on bug 888641, I can look into this one as well.
Could anyone please assign it to me? Thanks.

Comment 2

[Scoobidiver (away)]

A comment says: "trying to get a health report on my mobile phone"

Comment 3

[Michal Kajda [:mkajda]]

Yes, it is the same story as in case of bug 888641.

It crashes here:
http://hg.mozilla.org/mozilla-central/file/d3a746cf592d/mobile/android/base/BrowserToolbar.java#l877
and mFocusOrder is null at this point if attachedToWindow wasn't called.
The same was with mProgressSpinner in case of bug 888641.

Initialization of mFocusOrder needs to be moved to constructor of BrowserToolbar.

Comment 4

[Michal Kajda [:mkajda]]

Current revision looks like that:
http://hg.mozilla.org/mozilla-central/file/17fe59f6c54a/mobile/android/base/BrowserToolbar.java#l423

so patch will move whole block into constructor.
I've already done this, but now I'm preparing test build on my old, slow laptop ;)
I will attach patch if build is successful.

Comment 5

[Michal Kajda [:mkajda]]

Created attachment 771710 [details] [diff] [review]
Fixes NPE in BrowserToolbar caused by mFocusOrder

Comment 6

[Scoobidiver (away)]

It spiked in 25.0a1/20130705 (code regression) or on July 5 (FHR service regression?).

Comment 7

[Michal Kajda [:mkajda]]

Created attachment 771851 [details] [diff] [review]
Fixes NPE in updateFocusOrder

Comment 8

[Michal Kajda [:mkajda]]

Created attachment 771852 [details] [diff] [review]
Fixes NPE in updateFocusOrder (simplified, preferred)

Comment 9

[Michal Kajda [:mkajda]]

Created attachment 771853 [details]
Wrong elements linking

Comment 10

[Michal Kajda [:mkajda]]

My previous patch fixes only NPE when mFocusOrder is null at line:
http://hg.mozilla.org/mozilla-central/file/f8a08d0a1b2a/mobile/android/base/BrowserToolbar.java#l888

But now there is another problem introduced by fix for bug 888497.
There is 'if' checking for number of children in mActionItemBar, but line causing crash is outside this 'if' block, so there is an attempt to get child at negative index and getChildAt returns null which is dereferenced later on.
http://hg.mozilla.org/mozilla-central/file/f8a08d0a1b2a/mobile/android/base/BrowserToolbar.java#l904

Further more I'm not sure if first and last child in mActionItemBar should be linked from and to the same element in focusable list. I attached picture illustrating how it works now without a patch. At line 904 child is assigned to 'view' which is iterator of the main for loop (iterating through elements of mFocusOrder). Was it suppose to be 'prevView'?

I prepared two patches.

One iterates through all children in mActionItemBar and link them in one focusable chain. If mActionItemBar has no children it is skipped from the chain. 

Second patch assumes that items inside mActionItemBar are in correct order and operates only on first and last elements. I think it was the original idea introduced by fix for bug 888497.

Requesting focus was extracted from for loop and is called only once.

Comment 11

[Teodora Vermesan (:TeoVermesan)]

I can easily reproduce this:
1. with a clean profile load yahoo.com
2. load gmail.com
The app crashes: https://crash-stats.mozilla.com/report/index/62381fb6-9cd4-4426-b568-dbc0a2130708

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 771851 [details] [diff] [review]
Fixes NPE in updateFocusOrder

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

Thanks for finding and fixing all of these silly errors.

::: mobile/android/base/BrowserToolbar.java
@@ +891,5 @@
>                      needsNewFocus = true;
>                  }
>                  continue;
>              }
> +            if (view == mActionItemBar) {

Nit: Please add an empty line above this if statement.

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 771852 [details] [diff] [review]
Fixes NPE in updateFocusOrder (simplified, preferred)

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

I think I actually prefer the approach you used in the first patch. We're adding support for page actions in bug 734877, so setting focus order only for the first/last elements in the list will break dynamically added and removed items. For example:

a [ 1 2 ] b

With elements 1 and 2 in the list, a is linked to 1, and 2 is linked to b. If we then add another page action icon to the list, we'll end up with this:

a [ 1 2 3 ] b

But since we only update the first and last elements in the list, 2 will still be linked to b. As a result, I think we'll want to manually update the focus order for every item.

Comment 14

[Michal Kajda [:mkajda]]

Created attachment 772191 [details] [diff] [review]
Fixes NPE in updateFocusOrder

Thanks for comments Brian.
Empty line added.

Comment 15

[Nathan Froyd [:froydnj]]

FWIW, I was experiencing this crash and attachment 772191 [details] [diff] [review] has fixed the crash for me.

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 772191 [details] [diff] [review]
Fixes NPE in updateFocusOrder

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

Looks good. Thanks for the patch!

Comment 17

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 772191 [details] [diff] [review]
Fixes NPE in updateFocusOrder

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

Trusting bnicholson ;)

::: mobile/android/base/BrowserToolbar.java
@@ +894,5 @@
>              }
>  
> +            if (view == mActionItemBar) {
> +                final int childCount = mActionItemBar.getChildCount();
> +                for (int child = 0; child < childCount; ++child) {

Nit: child++ is the convention used.

Comment 18

[Michal Kajda [:mkajda]]

I'm C++ guy, I always use prefix operator instead of postfix when it doesn't change meaning, because it tends to be more efficient (probably it doesn't matter for built-in types, but anyway it's good habit ;) )

Should I change it or leave it as it is?

Thanks.

Comment 19

[Sriram Ramasubramanian [:sriram]]

(In reply to Michal Kajda [:mkajda] from comment #18)
> I'm C++ guy, I always use prefix operator instead of postfix when it doesn't
> change meaning, because it tends to be more efficient (probably it doesn't
> matter for built-in types, but anyway it's good habit ;) )
> 
> Should I change it or leave it as it is?
> 
> Thanks.

It's better to have it child++, as in Java we use that convention.

Comment 20

[Michal Kajda [:mkajda]]

Created attachment 772314 [details] [diff] [review]
Fixes NPE in updateFocusOrder

Thanks for all comments. I hope this time it ready to land.

Comment 21

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdea46bd0eae

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdea46bd0eae