Closed
Bug 889094
Opened 10 years ago
Closed 10 years ago
java.lang.NullPointerException: at org.mozilla.gecko.BrowserToolbar.updateFocusOrder(BrowserToolbar.java)
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox24 unaffected, firefox25+ fixed)
RESOLVED
FIXED
Firefox 25
Tracking | Status | |
---|---|---|
firefox24 | --- | unaffected |
firefox25 | + | fixed |
People
(Reporter: scoobidiver, Assigned: mkajda)
References
Details
(4 keywords, Whiteboard: [native-crash])
Crash Data
Attachments
(2 files, 4 obsolete files)
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•10 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•10 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•10 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•10 years ago
|
Assignee | ||
Comment 4•10 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•10 years ago
|
||
Attachment #771710 -
Flags: review?(sriram)
Attachment #771710 -
Flags: review?(bnicholson)
Reporter | ||
Comment 6•10 years ago
|
||
It spiked in 25.0a1/20130705 (code regression) or on July 5 (FHR service regression?).
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
||
Attachment #771852 -
Flags: review?(sriram)
Attachment #771852 -
Flags: review?(bnicholson)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 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.
Comment 11•10 years ago
|
||
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•10 years ago
|
Keywords: reproducible
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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•10 years ago
|
||
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)
![]() |
||
Comment 15•10 years ago
|
||
FWIW, I was experiencing this crash and attachment 772191 [details] [diff] [review] has fixed the crash for me.
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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•10 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•10 years ago
|
Flags: needinfo?(sriram)
Comment 19•10 years ago
|
||
(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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #772314 -
Flags: review?(bnicholson) → review+
Updated•10 years ago
|
Attachment #772314 -
Flags: review?(sriram) → review+
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fdea46bd0eae
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Reporter | ||
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
tracking-fennec: ? → ---
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•