961773 - crash in java.lang.NullPointerException: at org.mozilla.gecko.home.TabMenuStrip.onPageSelected(TabMenuStrip.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Flaviu Cos]

This bug was filed from the Socorro interface and is 
report bp-c9a5f65e-0ad3-47db-8c9b-2aa292140120.
=============================================================

Steps to reproduce:
1. Load an article from the news.google.com;
2. Tap on the reader mode button;
3. Tap on the 'Add to reading list'  button;
4. Tap on 'Reading list' button.

Stack trace:
0	libmozalloc.so	mozalloc_abort(char const*)	memory/mozalloc/mozalloc_abort.cpp
1	libxul.so	Java_org_mozilla_gecko_GeckoAppShell_reportJavaCrash	widget/android/AndroidJNI.cpp
2	libmozglue.so	Java_org_mozilla_gecko_GeckoAppShell_reportJavaCrash	mozglue/android/jni-stubs.inc
3	libdvm.so	libdvm.so@0x1dbce	
4	data@[e-mail]@classes.dex	data@[e-mail]@classes.dex@0x1cafdb	
5	dalvik-heap (deleted)	dalvik-heap (deleted)@0x44c3e	
6	libdvm.so	libdvm.so@0x4e125	
7	data@[e-mail]@classes.dex	data@[e-mail]@classes.dex@0x1cafd9	
8	libmozglue.so	Java_org_mozilla_gecko_GeckoAppShell_dispatchMemoryPressure	mozglue/android/jni-stubs.inc
9		@0x6a395aca	
10	libmozglue.so	Java_org_mozilla_gecko_GeckoAppShell_dispatchMemoryPressure	mozglue/android/jni-stubs.inc
11	libc.so	libc.so@0x49ffe	
12	libc.so	libc.so@0xdcc3	
13	libdvm.so	libdvm.so@0x4fd55	
14	libdvm.so	libdvm.so@0x73917	
15	libdvm.so	libdvm.so@0xaac72	
16	libdvm.so	libdvm.so@0x794bb	
17	core.odex	core.odex@0x95e82	
18	core.odex	core.odex@0x95e88	
19	dalvik-LinearAlloc (deleted)	dalvik-LinearAlloc (deleted)@0x46aae	
20	dalvik-LinearAlloc (deleted)	dalvik-LinearAlloc (deleted)@0x36d39a	
21	dalvik-heap (deleted)	dalvik-heap (deleted)@0x44c3e	
22	dalvik-LinearAlloc (deleted)	dalvik-LinearAlloc (deleted)@0x36d386	
23	data@[e-mail]@classes.dex	data@[e-mail]@classes.dex@0x1f28d3	
24	libdvm.so	libdvm.so@0x6be39	
25	data@[e-mail]@classes.dex	data@[e-mail]@classes.dex@0x1f28d3	
26	data@[e-mail]@classes.dex	data@[e-mail]@classes.dex@0x445e6	
27	dalvik-heap (deleted)	dalvik-heap (deleted)@0x44c3e	
28	data@[e-mail]@classes.dex	data@[e-mail]@classes.dex@0x1f28d3	
29		@0x68399ffe	
30	libdvm.so	libdvm.so@0x4fc5b

Comment 1

[Ioana Chiorean]

This works for me on latest Nightly Build on Nexus 4 (4.4) nor Galaxy Nexus (4.3).
This was encountered on Nexus 7 and also reproduced on Asus Transformers so I guess it might be tablet only.

Comment 2

[:Margaret Leibovic]

Could this be related to bug 949178?

Comment 3

[Lucas Rocha (:lucasr)]

This seems more like a regression from bug 942231. The tabs strip gets somehow out of sync with the pager somehow. And yes, this is tablet-specific because the TabStripMenu is only used on tablets.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #2)
> Could this be related to bug 949178?

Yes, it is indirectly related. We need to find a better approach for showing the reading list from reader and get rid of this whole pageid-as-homepager-argument mess.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> This seems more like a regression from bug 942231. The tabs strip gets
> somehow out of sync with the pager somehow. And yes, this is tablet-specific
> because the TabStripMenu is only used on tablets.

Scratch that, this is actually a regression from bug 958175. The page id passed by the reader UI ("reading_list") doesn't match any panel id anymore.

Comment 6

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > This seems more like a regression from bug 942231. The tabs strip gets
> > somehow out of sync with the pager somehow. And yes, this is tablet-specific
> > because the TabStripMenu is only used on tablets.
> 
> Scratch that, this is actually a regression from bug 958175. The page id
> passed by the reader UI ("reading_list") doesn't match any panel id anymore.

Doh. Sorry I missed that!

Comment 8

[Aaron Train [:aaronmt]]

Last good revision: 324e2cba1029 (2014-01-16)

First bad revision: 9bcc52594322 (2014-01-17)

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=324e2cba1029&tochange=9bcc52594322


c518a5f33268	Margaret Leibovic — Bug 959772 - Create a generic PanelType and corresponding *Panel class for third-party panels. r=lucasr

f72fac727137	Margaret Leibovic — Bug 958175 - Get rid of PanelConfig constructors that don't take an id. r=lucasr

Any of those?

Comment 9

[:Margaret Leibovic]

Attachment: (Part 1) Update reading list panel id

This fixes the crash that happens when clicking the reading list item in reader mode, but we should add a check in the right place to avoid crashing if someone tries loading a panel id that doesn't exist.

Comment 10

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #8)
> Last good revision: 324e2cba1029 (2014-01-16)
> 
> First bad revision: 9bcc52594322 (2014-01-17)
> 
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=324e2cba1029&tochange=9bcc52594322
> 
> 
> c518a5f33268	Margaret Leibovic — Bug 959772 - Create a generic PanelType and
> corresponding *Panel class for third-party panels. r=lucasr
> 
> f72fac727137	Margaret Leibovic — Bug 958175 - Get rid of PanelConfig
> constructors that don't take an id. r=lucasr
> 
> Any of those?

Yup, we already determined that this was caused by bug 958175.

Comment 11

[:Margaret Leibovic]

Attachment: (Part 2) Don't crash if the panel doesn't exist

When we filed bug 949178, I don't think we thought about the case where someone might just try loading about:home with any random "page" parameter. This patch makes us more gracefully fail back to the default panel if the panel you're trying to load doesn't exist.

We'll still need to find some more user-friendly solution to bug 949178, but with this patch, at least we won't crash if the user has disabled the reading list panel.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8374438 [details] [diff] [review]
(Part 1) Update reading list panel id

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

Looks like a good pragmatic measure, thanks for taking care of this :-)

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8374455 [details] [diff] [review]
(Part 2) Don't crash if the panel doesn't exist

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

Nice.

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/90a3bd215dff
https://hg.mozilla.org/integration/fx-team/rev/bf91ce1c9102

Comment 15

[:Margaret Leibovic]

Comment on attachment 8374438 [details] [diff] [review]
(Part 1) Update reading list panel id

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 958175
User impact if declined: crash when opening reading list from reader mode
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, only affects button in reader mode
String or IDL/UUID changes made by this patch: none

Comment 16

[:Margaret Leibovic]

Comment on attachment 8374455 [details] [diff] [review]
(Part 2) Don't crash if the panel doesn't exist

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 958175 
User impact if declined: crash when opening reading list from reader mode if reading list panel is disabled
Testing completed (on m-c, etc.): just landed on fx-team 
Risk to taking this patch (and alternatives if risky): low-risk, adds extra check before trying to open about:home to a specific panel
String or IDL/UUID changes made by this patch: none

Comment 17

[Sylvestre Ledru [:Sylvestre]]

I will be waiting for the commit in m-c before uplifting it.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90a3bd215dff
https://hg.mozilla.org/mozilla-central/rev/bf91ce1c9102

Comment 20

[Flaviu Cos]

Verified as fixed in build 30.0a1 (2013-02-14);
Device: Lenovo Yoga Tab 10 (Android 4.2.2).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5b8db2fdbae
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1dfc5d159a0

Comment 22

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 29.0a2 (2013-02-16)
Device: Samsung Galaxy Tab 
OS: Android 4.0.4