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)
Tracking
(firefox35 affected, firefox49 fixed, firefox50 fixed)
RESOLVED
FIXED
Firefox 50
People
(Reporter: aaronmt, Assigned: mcomella)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
sebastian
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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)
Reporter | ||
Updated•10 years ago
|
Keywords: regression
Reporter | ||
Comment 1•10 years ago
|
||
Phone running tablet code?
Comment 2•10 years ago
|
||
Exactly. We need this device to verify some tentative fixes.
Updated•10 years ago
|
tracking-fennec: ? → 35+
Updated•10 years ago
|
Assignee: nobody → lucasr.at.mozilla
Comment 4•10 years ago
|
||
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+ → ---
Updated•9 years ago
|
Assignee: lucasr.at.mozilla → nobody
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Updated•9 years ago
|
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]
Assignee | ||
Comment 8•8 years ago
|
||
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.…
Assignee | ||
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
Isn't this partially implementing bug 1277379?
Comment 11•8 years ago
|
||
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.
Comment 12•8 years ago
|
||
I can reproduce the crash on the N9 with N. But isn't this a different crash? BrowserToolbarTabletBase.setButtonEnabled() vs. View.setEnabled(boolean)?
Assignee | ||
Comment 13•8 years ago
|
||
(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.
Assignee | ||
Comment 14•8 years ago
|
||
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.
Assignee | ||
Comment 15•8 years ago
|
||
(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
Assignee | ||
Comment 16•8 years ago
|
||
(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.
Assignee | ||
Comment 17•8 years ago
|
||
Sebastian, would you prefer if I switched all of the HardwareUtils code over to loading the layout from resources?
Flags: needinfo?(s.kaspari)
Comment 18•8 years ago
|
||
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+
Comment 19•8 years ago
|
||
(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)
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5af54ccbe317f7e4ba192b2960f7f7cb758f9001 Bug 1079874 - Define device configuration in inflated resources. r=sebastian
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5af54ccbe317
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment 23•8 years ago
|
||
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?
Updated•8 years ago
|
status-firefox49:
--- → affected
Comment 25•8 years ago
|
||
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+
Comment 26•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a20b8498eaa2
Updated•3 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
•