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
As I was working on bug 888641, I can look into this one as well. Could anyone please assign it to me? Thanks.
A comment says: "trying to get a health report on my mobile phone"
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.
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.
Created attachment 771710 [details] [diff] [review] Fixes NPE in BrowserToolbar caused by mFocusOrder
It spiked in 25.0a1/20130705 (code regression) or on July 5 (FHR service regression?).
Created attachment 771851 [details] [diff] [review] Fixes NPE in updateFocusOrder
Created attachment 771852 [details] [diff] [review] Fixes NPE in updateFocusOrder (simplified, preferred)
Created attachment 771853 [details] Wrong elements linking
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
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 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.
Created attachment 772191 [details] [diff] [review] Fixes NPE in updateFocusOrder Thanks for comments Brian. Empty line added.
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!
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.
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.
(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.
Created attachment 772314 [details] [diff] [review] Fixes NPE in updateFocusOrder Thanks for all comments. I hope this time it ready to land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdea46bd0eae
https://hg.mozilla.org/mozilla-central/rev/fdea46bd0eae
or
