Last Comment Bug 888641 - java.lang.NullPointerException: at org.mozilla.gecko.BrowserToolbar.setProgressVisibility(BrowserToolbar.java)
: java.lang.NullPointerException: at org.mozilla.gecko.BrowserToolbar.setProgre...
Status: RESOLVED FIXED
[native-crash]
: crash, regression, topcrash
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 25 Branch
: ARM Android
: -- critical (vote)
: Firefox 25
Assigned To: Michal Kajda [:mkajda]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks: 887020
  Show dependency treegraph
 
Reported: 2013-06-29 09:30 PDT by Scoobidiver (away)
Modified: 2016-07-29 14:33 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
Changed initialization point of mProgressSpinner (2.07 KB, patch)
2013-06-30 05:06 PDT, Michal Kajda [:mkajda]
sriram.mozilla: review+
Details | Diff | Splinter Review
Return from method if mProgressSpinner is null (1.40 KB, patch)
2013-06-30 05:07 PDT, Michal Kajda [:mkajda]
no flags Details | Diff | Splinter Review
Initialization point of mProgressSpinner changed and made consistent with other drawables. (2.06 KB, patch)
2013-07-01 14:12 PDT, Michal Kajda [:mkajda]
sriram.mozilla: review+
Details | Diff | Splinter Review
Initializes mProgressSpinner in the constructor of BrowserToolbar (2.12 KB, patch)
2013-07-01 14:35 PDT, Michal Kajda [:mkajda]
sriram.mozilla: review+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2013-06-29 09:30:17 PDT
It has been hit by three users in 25.0a1/2013029. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e3a124c9c1a&tochange=c5ce065936fa
I think it's a regression from bug 887020.

Here is a crash report: bp-132521ef-622b-4264-bb27-115762130629.

java.lang.NullPointerException
	at org.mozilla.gecko.BrowserToolbar.setProgressVisibility(BrowserToolbar.java:805)
	at org.mozilla.gecko.BrowserToolbar.onTabChanged(BrowserToolbar.java:470)
	at org.mozilla.gecko.Tabs$5.run(Tabs.java:550)
	at android.os.Handler.handleCallback(Handler.java:725)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:5227)
	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:795)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:562)
	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.setProgressVisibility%28BrowserToolbar.java%29
Comment 1 Michal Kajda [:mkajda] 2013-06-29 12:35:49 PDT
Hi All,

May I take this bug and start working on it?
I am completely new here, but would like to start contributing.
If someone could be my mentor, it would be great.

I looked into this a bit and it seems that it crashes because mProgressSpinner is null when mProgressSpinner.start(); gets called.

mProgressSpinner is instantiated in onAttachedToWindow callback (@ line 347 in BrowserToolbar.java) which in turn might be called later than onTabChanged.
The only what is guaranteed according to Android docs that attachedToWindow would be invoked before onDraw, but this could happen at any time.
So I suspect race condition occurred.

I haven't reproduced this though.
If anyone had testing scenario for it I would appreciate it.

Please let me know your thoughts and if I can go for it.
Comment 2 Michal Kajda [:mkajda] 2013-06-29 13:33:38 PDT
Previously there was 'public void from(RelativeLayout layout)' method in BrowserToolbar class and was replaced (more or less) by 'public void onAttachedToWindow()' callback.
http://hg.mozilla.org/mozilla-central/rev/cc5850cf4042#l2.177

As mentioned in previous comment 'onAttachedToWindow' is called asynchronously before 'onDraw' gets called. The 'from' method was called synchronously in BrowserApp's 'onCreate' callback, so mProgressSpinner had valid value when BrowserApp was created.
http://hg.mozilla.org/mozilla-central/rev/cc5850cf4042#l1.31

All these changes where introduced by this revision http://hg.mozilla.org/mozilla-central/rev/cc5850cf4042 when working on https://bugzilla.mozilla.org/show_bug.cgi?id=887020 (as Scoobidiver suspected).
Comment 3 Scoobidiver (away) 2013-06-30 02:41:07 PDT
There are 20 crashes in one build.
Comment 4 Michal Kajda [:mkajda] 2013-06-30 03:51:17 PDT
The crash is caused by Tabs' notifyListeners method which is run in UI thread and because it is current thread (main thread) it is called immediately. Apparently one of the listeners is BrowserToolbar which has not been yet attached to window, so its mProgressSpinner is equal to null.

I still haven't reproduced it on my Nexus 4.

How can I debug Fennec using DDMS? I tried this https://wiki.mozilla.org/Mobile/Fennec/Android#Debugging_with_eclipse but description or me are missing something.

I think that moving 
ProgressSpinner = (AnimationDrawable) getResources().getDrawable(R.drawable.progress_spinner); from onAttachedToWindow method to constructor of BrowserToolbar is the most straightforward solution.

The question is if it is sufficient.
Comment 5 Michal Kajda [:mkajda] 2013-06-30 03:55:42 PDT
Here are some comments from recent crash report (translated from Russian to English by google translator)

"If you attempt to use the built-in search Yandex, your browser has fallen."

"When you try to open an official project page CyanogenMod, your browser has fallen."
Comment 6 Michal Kajda [:mkajda] 2013-06-30 05:06:40 PDT
Created attachment 769480 [details] [diff] [review]
Changed initialization point of mProgressSpinner
Comment 7 Michal Kajda [:mkajda] 2013-06-30 05:07:41 PDT
Created attachment 769481 [details] [diff] [review]
Return from method if mProgressSpinner is null
Comment 8 Michal Kajda [:mkajda] 2013-06-30 06:26:33 PDT
I added to patches with different solutions and hope any of them is OK ;)
Comment 9 Michal Kajda [:mkajda] 2013-06-30 11:34:05 PDT
It's worth to look into all crash reports :)
Some of them contain more detailed stack trace:
https://crash-stats.mozilla.com/report/index/d42e5c74-19bc-4789-9a30-5302b2130630
maybe it differs because activity was resuming not launching.
Comment 10 Teodora Vermesan (:TeoVermesan) 2013-07-01 02:24:14 PDT
On Samsung Galaxy Nexus (Android 4.1.1) using Nightly 25.0a1 (2013-06-30) the app crashes with this crash signature.
STR:
1. go to a website( for example gmail.com)
2. go to news.google.com
The app crashes.

https://crash-stats.mozilla.com/report/index/6e371b98-4c24-459e-9342-cfbc82130701
Comment 11 Michal Kajda [:mkajda] 2013-07-01 11:07:33 PDT
Thanks for STR Teodora, but still wasn't able to reproduce.
I tried nigthly builds and built locally from repo as well. No luck.

Are you using tabs sync between PC and mobile devices?
Comment 12 Sriram Ramasubramanian [:sriram] 2013-07-01 13:24:55 PDT
Comment on attachment 769480 [details] [diff] [review]
Changed initialization point of mProgressSpinner

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

Sounds good to me.
Comment 13 Sriram Ramasubramanian [:sriram] 2013-07-01 13:25:45 PDT
(In reply to Michal Kajda from comment #7)
> Created attachment 769481 [details] [diff] [review]
> Return from method if mProgressSpinner is null

This might not be needed.
Comment 14 Michal Kajda [:mkajda] 2013-07-01 14:12:07 PDT
Created attachment 769839 [details] [diff] [review]
Initialization point of mProgressSpinner changed and made consistent with other drawables.

@Sriram: Thank you for review of previous patch. I prepared another one which as I realized that this one would be more consistent with initialization of other drawables. It uses stored resources reference instead of calling getResources() method again.
Comment 15 Sriram Ramasubramanian [:sriram] 2013-07-01 14:13:58 PDT
Comment on attachment 769839 [details] [diff] [review]
Initialization point of mProgressSpinner changed and made consistent with other drawables.

Could you please move the initialization a bit below?
May be after https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/BrowserToolbar.java#l219

r+ with that.
Comment 16 Michal Kajda [:mkajda] 2013-07-01 14:35:43 PDT
Created attachment 769850 [details] [diff] [review]
Initializes mProgressSpinner in the constructor of BrowserToolbar

Done.
In case of 'r+': Could anyone push it to the try server please? I don't have permissions as I'm a 'new starter' ;)
Comment 17 Sriram Ramasubramanian [:sriram] 2013-07-02 13:18:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a081df164311
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-07-03 11:34:59 PDT
https://hg.mozilla.org/mozilla-central/rev/a081df164311

Note You need to log in before you can comment on or make changes to this bug.