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

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: Scoobidiver (away), Assigned: mkajda)

Tracking

(4 keywords)

25 Branch
Firefox 25
ARM
Android
crash, regression, reproducible, topcrash
Points:
---

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ fixed)

Details

(Whiteboard: [native-crash], crash signature)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 1

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

Comment 2

4 years ago
A comment says: "trying to get a health report on my mobile phone"
Assignee: nobody → michal.kajda
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 887020
status-firefox24: --- → unaffected
Keywords: regression
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Created attachment 771710 [details] [diff] [review]
Fixes NPE in BrowserToolbar caused by mFocusOrder
Attachment #771710 - Flags: review?(sriram)
Attachment #771710 - Flags: review?(bnicholson)
(Reporter)

Comment 6

4 years ago
It spiked in 25.0a1/20130705 (code regression) or on July 5 (FHR service regression?).
tracking-fennec: --- → ?
tracking-firefox25: --- → ?
Keywords: topcrash
(Assignee)

Comment 7

4 years ago
Created attachment 771851 [details] [diff] [review]
Fixes NPE in updateFocusOrder
Attachment #771710 - Attachment is obsolete: true
Attachment #771710 - Flags: review?(sriram)
Attachment #771710 - Flags: review?(bnicholson)
Attachment #771851 - Flags: review?(sriram)
Attachment #771851 - Flags: review?(bnicholson)
(Assignee)

Comment 8

4 years ago
Created attachment 771852 [details] [diff] [review]
Fixes NPE in updateFocusOrder (simplified, preferred)
Attachment #771852 - Flags: review?(sriram)
Attachment #771852 - Flags: review?(bnicholson)
(Assignee)

Comment 9

4 years ago
Created attachment 771853 [details]
Wrong elements linking
(Assignee)

Comment 10

4 years ago
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.
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

Updated

4 years ago
Keywords: reproducible
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.
Attachment #771851 - Flags: review?(bnicholson) → review+
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.
Attachment #771852 - Flags: review?(bnicholson) → review-
(Assignee)

Comment 14

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

Thanks for comments Brian.
Empty line added.
Attachment #771851 - Attachment is obsolete: true
Attachment #771852 - Attachment is obsolete: true
Attachment #771851 - Flags: review?(sriram)
Attachment #771852 - Flags: review?(sriram)
Attachment #772191 - Flags: review?(sriram)
Attachment #772191 - Flags: review?(bnicholson)
FWIW, I was experiencing this crash and attachment 772191 [details] [diff] [review] has fixed the crash for me.
Comment on attachment 772191 [details] [diff] [review]
Fixes NPE in updateFocusOrder

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

Looks good. Thanks for the patch!
Attachment #772191 - Flags: review?(bnicholson) → review+
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.
Attachment #772191 - Flags: review?(sriram) → review+
(Assignee)

Comment 18

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(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.
Flags: needinfo?(sriram)
(Assignee)

Comment 20

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

Thanks for all comments. I hope this time it ready to land.
Attachment #772191 - Attachment is obsolete: true
Attachment #772314 - Flags: review?(sriram)
Attachment #772314 - Flags: review?(bnicholson)
Attachment #772314 - Flags: review?(bnicholson) → review+
Attachment #772314 - Flags: review?(sriram) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdea46bd0eae
https://hg.mozilla.org/mozilla-central/rev/fdea46bd0eae
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
(Reporter)

Updated

4 years ago
status-firefox25: affected → fixed

Updated

4 years ago
tracking-firefox25: ? → +
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.