Closed Bug 1079874 Opened 10 years ago Closed 8 years ago

crash in java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled(BrowserToolbarTabletBase.java)

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

35 Branch
All
Android
defect
Not set
critical

Tracking

(firefox35 affected, firefox49 fixed, firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox35 --- affected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: aaronmt, Assigned: mcomella)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-4961677c-a41b-4e03-9cf7-5be382141007.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled(BrowserToolbarTabletBase.java:137)
	at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.<init>(BrowserToolbarTabletBase.java:46)
	at org.mozilla.gecko.toolbar.BrowserToolbarTablet.<init>(BrowserToolbarTablet.java:45)
	at org.mozilla.gecko.toolbar.BrowserToolbar.create(BrowserToolbar.java:160)
	at org.mozilla.gecko.BrowserApp.onCreateView(BrowserApp.java:245)
	at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:676)
	at android.view.LayoutInflater.rInflate(LayoutInflater.java:746)
	at android.view.LayoutInflater.rInflate(LayoutInflater.java:749)
	at android.view.LayoutInflater.rInflate(LayoutInflater.java:749)
	at android.view.LayoutInflater.rInflate(LayoutInflater.java:749)
	at android.view.LayoutInflater.inflate(LayoutInflater.java:489)
	at de.robv.android.xposed.XposedBridge.invokeOriginalMethodNative(Native Method)
	at de.robv.android.xposed.XposedBridge.handleHookedMethod(XposedBridge.java:640)
	at android.view.LayoutInflater.inflate(Native Method)
	at android.view.LayoutInflater.inflate(LayoutInflater.java:396)
	at android.view.LayoutInflater.inflate(LayoutInflater.java:352)
	at com.android.internal.policy.impl.PhoneWindow.setContentView(PhoneWindow.java:270)
	at android.app.Activity.setContentView(Activity.java:1881)
	at org.mozilla.gecko.GeckoApp.onCreate(GeckoApp.java:1268)
	at org.mozilla.gecko.BrowserApp.onCreate(BrowserApp.java:533)
	at android.app.Activity.performCreate(Activity.java:5104)
	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1080)
	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2147)
	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2233)
	at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:3695)
	at android.app.ActivityThread.access$700(ActivityThread.java:144)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1243)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:5086)
	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:793)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:560)
	at de.robv.android.xposed.XposedBridge.main(XposedBridge.java:133)
	at dalvik.system.NativeStart.main(Native Method)
Keywords: regression
Phone running tablet code?
Exactly. We need this device to verify some tentative fixes.
See bug 1024717 as a potential fix.
See Also: → 1024717
tracking-fennec: ? → 35+
Assignee: nobody → lucasr.at.mozilla
I see one crash in crash-stats. Not tracking.

It would still be nice to get this device and improve our code: ASUS PadFone 2
tracking-fennec: 35+ → ---
Assignee: lucasr.at.mozilla → nobody
This crash is still high on 42 (nightly) and I see some Amazon and Samsung SM-T210 devices. I am worried this is becoming more common place. Way are these phones acting like tablets? How can we mitigate the issue?
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(kbrosnan)
Previous investigation that was not written down suspected users that were running custom ROMs or rooted devices which allow changing the reported DPI. /system/build.prop contains a preference ro.sf.lcd_density suppose we could try to validate that on the system but that seems like a nightmare and would need to be done in the startup path.
Flags: needinfo?(kbrosnan)
It seems relatively straight-forward to implement bug 1024717 – we define these constants in resources and inflate the BrowserToolbar configuration based on these.

Working on a patch...
Assignee: nobody → michael.l.comella
Flags: needinfo?(michael.l.comella)
Depends on: 1024717
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled(BrowserToolbarTabletBase.java)] → [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled(BrowserToolbarTabletBase.java)] [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled]
Crash stats says we haven't seen this crash since FF 42.0.2, but setButtonEnabled appears to be missing from the source in mozilla-release builds so perhaps that's to be expected. There are other similar crash signatures in the top crashers, however, such as [1], which occurs on 49 (albeit, rarely). It's worth noting that there are two crashes on N9 with Android N.

[1]: https://crash-stats.mozilla.com/signature/?date=%3C2016-06-01T18%3A38%3A39&date=%3E%3D2016-05-25T18%3A38%3A39&product=FennecAndroid&version=49.0a1&signature=java.lang.NullPointerException%3A%20Attempt%20to%20invoke%20virtual%20method%20%27%27void%20android.view.View.setEnabled%28boolean%29%27%27%20on%20a%20null%20object%20reference%20at%20org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.%3Cinit%3E%28BrowserToolbarTabletBase.java%29
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled(BrowserToolbarTabletBase.java)] [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled] → [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled(BrowserToolbarTabletBase.java)] [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.setButtonEnabled] [@ java.…
See the added code comments for motivations.

Note that this is a speculative patch - I was unable to reproduce the crash and
thus do not know if it fixes it, however, I did test that the appropriate
toolbar configuration is created on vanilla Android for phone & tablet.

Review commit: https://reviewboard.mozilla.org/r/57000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57000/
Attachment #8758902 - Flags: review?(s.kaspari)
Isn't this partially implementing bug 1277379?
https://reviewboard.mozilla.org/r/57000/#review54374

::: mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbar.java:164
(Diff revision 1)
> +        final boolean isLargeResource = context.getResources().getBoolean(R.bool.is_large_resource);
>          final BrowserToolbar toolbar;
> -        if (HardwareUtils.isTablet()) {
> +        if (isLargeResource) {

Somehow I have a bad feeling about this. We use HardwareUtils.isTablet() in many places. I wonder if it's going to be a problem if this here behaves differently than the other code paths.
I can reproduce the crash on the N9 with N. But isn't this a different crash? BrowserToolbarTabletBase.setButtonEnabled() vs. View.setEnabled(boolean)?
(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> Isn't this partially implementing bug 1277379?

Partially, yes, but not fully.

It's more like implementing bug 1024717.
https://reviewboard.mozilla.org/r/57000/#review54374

> Somehow I have a bad feeling about this. We use HardwareUtils.isTablet() in many places. I wonder if it's going to be a problem if this here behaves differently than the other code paths.

My thought was to try a partial fix for this crash bug, see how it works out, and then move everything over to the definition in resources if so. My concern with moving everything over right away is that there could be unknown side effects on a change like this.

That being said, I probably wouldn't have changed everything over any time soon because of the potential long-tail of clean-up bugs.

Would you prefer if I just changed HardwareUtils.isTablet over to using this method? fwiw, there might be some startup time implications from accessing resources so early.
(In reply to Michael Comella (:mcomella) from comment #14)
> My thought was to try a partial fix for this crash bug, see how it works
> out

It's worth mentioning that BrowserToolbar is a particular bad case and is self-contained. We inflate different views in code based on the HardwareUtils value [1]. This branch assumes HardwareUtils lines up with the resources we inflated, e.g. BrowserToolbarTablet & BrowserToolbarPhone, and makes calls on views that may exist in one configuration but not in another. This changeset changes the branch to accurately model the assumptions when this code was written – i.e. if we're using the tablet resource, use the tablet code.

Other HardwareUtils methods are isolated from this context and should not be affected (though, the BrowserToolbar crashes may have been the first in a series of crashes related to the inflated configuration and the assumptions we make, which is incentivizing me to just try switching HardwareUtils over to the inflated resources method).

[1]: https://dxr.mozilla.org/mozilla-central/rev/3e8ee3599a67edd971770af4982ad4b0fe77f073/mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbar.java#163
(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> I can reproduce the crash on the N9 with N. But isn't this a different
> crash? BrowserToolbarTabletBase.setButtonEnabled() vs.
> View.setEnabled(boolean)?

See comment 8 – the code changed a bit, and generally, we're making wrong assumptions with which layout we're using in the code (as explained in comment 15) so it's going to cause other similar crashes.
Sebastian, would you prefer if I switched all of the HardwareUtils code over to loading the layout from resources?
Flags: needinfo?(s.kaspari)
Comment on attachment 8758902 [details]
MozReview Request: Bug 1079874 - Define device configuration in inflated resources. r=sebastian

https://reviewboard.mozilla.org/r/57000/#review55010

Okay, seeing that we use HardwareUtils to create BrowserToolbar but use the resource system to inflate the views, it makes a lot of sense to use the resource system for both here.

(In reply to Michael Comella (:mcomella) from comment #17)
> Sebastian, would you prefer if I switched all of the HardwareUtils code over
> to loading the layout from resources?

While this would be nice I agree that this is calling for trouble now. So let's not do it now.
Attachment #8758902 - Flags: review?(s.kaspari) → review+
(In reply to Michael Comella (:mcomella) from comment #15)
> fwiw, there might be some startup time implications from accessing
> resources so early.

I wonder if this will actually affect startup: There are plenty of other things happening that require the resources in one way or the other. Right now I can't see how accessing resources in BrowserToolbar or in HardwareUtils will have different results in regards to app startup. But that's something for someone else to try and measure. ;)
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/5af54ccbe317
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment on attachment 8758902 [details]
MozReview Request: Bug 1079874 - Define device configuration in inflated resources. r=sebastian

Approval Request Comment

[Feature/regressing bug #]: We still see this crash on Firefox 48 and 49 (bug 1297772). It can be triggered by using Android 7.0 and the split screen feature.

[User impact if declined]: Crashes in certain situations when using Firefox and the split screen feature.

[Describe test coverage new/current, TreeHerder]: We do not have any STR to reproduce this.

[Risks and why]: Low. Patch has been in 50 for 3 months.

[String/UUID change made/needed]: -
Attachment #8758902 - Flags: approval-mozilla-beta?
Comment on attachment 8758902 [details]
MozReview Request: Bug 1079874 - Define device configuration in inflated resources. r=sebastian

Crash fix, taking this for beta 7.
Attachment #8758902 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: