929982 - ClassCastException: org.mozilla.gecko.home.TwoLinePageRow cannot be cast to org.mozilla.gecko.home.BookmarkFolderView

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

This is crash #3 in Fx26 now:
https://crash-stats.mozilla.com/report/index/453796fa-4de9-4101-8527-7612f2131020

Comment 1

[Aaron Train [:aaronmt]]

ava.lang.ClassCastException: org.mozilla.gecko.home.TwoLinePageRow cannot be cast to org.mozilla.gecko.home.BookmarkFolderView
	at org.mozilla.gecko.home.BookmarksListAdapter.bindView(BookmarksListAdapter.java:185)
	at org.mozilla.gecko.home.MultiTypeCursorAdapter.getView(MultiTypeCursorAdapter.java:62)
	at android.widget.HeaderViewListAdapter.getView(HeaderViewListAdapter.java:220)
	at android.widget.AbsListView.obtainView(AbsListView.java:2022)

Comment 2

[Lucas Rocha (:lucasr)]

Couldn't reproduce this in Fx26.

Comment 3

[Lucas Rocha (:lucasr)]

From the looks of it, I suspect there is some mismatch between what getItemViewType() returns and the actual views.

I found at least one racy condition in BookmarksPage when we refresh the cursor to load a new folder. When we use moveToParentFolder() and moveToChildFolder(), we immediately pop/push an item from the parent stack and *then* refresh the cursor off the main thread. This means there's a short period of time where the parent stack and the cursor don't match.

More importantly, the parent stack state affects the adapter's getCount(), which means we should either call notifyDataSetChanged() whenever the parent stack changes or only change the parent stack once we get the new cursor. The latter is probably the right thing to do here.

Comment 4

[Brian Nicholson (:bnicholson)]

Looks similar to bug 773944 from pre-fig. The "fix" there was to just do an instanceof check.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Change Bookmark's parent stack to use a custom type (r=margaret)

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Update parent stack and cursor at the same time (r=margaret)

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8334504 [details] [diff] [review]
Update parent stack and cursor at the same time (r=margaret)

This patch addresses the race described in comment #3. I suspect this might fix the root cause of bug 883500 too. We should uplift this patch to Aurora/Beta to see how it affects our crash rate.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8334503 [details] [diff] [review]
Change Bookmark's parent stack to use a custom type (r=margaret)

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

::: mobile/android/base/home/BookmarksPage.java
@@ -46,5 @@
>      // Adapter for list of bookmarks.
>      private BookmarksListAdapter mListAdapter;
>  
>      // Adapter's parent stack.
> -    private List<Pair<Integer, String>> mSavedParentStack;

You should also get rid of the Pair import in this file.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8334504 [details] [diff] [review]
Update parent stack and cursor at the same time (r=margaret)

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

Sorry for the slow review, it took me some time to parse this. This seems reasonable to me, although I have a few questions/suggestions.

It also looks like some extraneous toolbar changes ended up in here... I assume you'll remove those before landing :)

::: mobile/android/base/home/BookmarksListAdapter.java
@@ -63,5 @@
>              mParentStack = new LinkedList<FolderInfo>();
> -
> -            // Add the root folder to the stack
> -            FolderInfo rootFolder = new FolderInfo(Bookmarks.FIXED_ROOT_ID);
> -            mParentStack.addFirst(rootFolder);

So is this taken care of by the BookmarksLoader constructor now?

@@ +134,5 @@
>          if (mParentStack.size() == 1) {
>              return false;
>          }
>  
> +        if (mListener != null) {

Do we actually expect mListener to ever be null here? If it is, aren't we in a bad state, since moveToParentFolder() wouldn't actually move to the parent folder?

@@ +249,1 @@
>          return (mParentStack.peek().id != Bookmarks.FIXED_ROOT_ID);

Can't we just change this whole method to return `mParentStack.size() != 0`?

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #8)
> Comment on attachment 8334503 [details] [diff] [review]
> Change Bookmark's parent stack to use a custom type (r=margaret)
> 
> Review of attachment 8334503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BookmarksPage.java
> @@ -46,5 @@
> >      // Adapter for list of bookmarks.
> >      private BookmarksListAdapter mListAdapter;
> >  
> >      // Adapter's parent stack.
> > -    private List<Pair<Integer, String>> mSavedParentStack;
> 
> You should also get rid of the Pair import in this file.

Done.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #10)
> Comment on attachment 8334504 [details] [diff] [review]
> Update parent stack and cursor at the same time (r=margaret)
> 
> Review of attachment 8334504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the slow review, it took me some time to parse this. This seems
> reasonable to me, although I have a few questions/suggestions.
> 
> It also looks like some extraneous toolbar changes ended up in here... I
> assume you'll remove those before landing :)

Oops! Yeah, removed ;-)

> ::: mobile/android/base/home/BookmarksListAdapter.java
> @@ -63,5 @@
> >              mParentStack = new LinkedList<FolderInfo>();
> > -
> > -            // Add the root folder to the stack
> > -            FolderInfo rootFolder = new FolderInfo(Bookmarks.FIXED_ROOT_ID);
> > -            mParentStack.addFirst(rootFolder);
> 
> So is this taken care of by the BookmarksLoader constructor now?

The parent stack is only updated when the cursor is loaded now, see the new swapCursor() method. 

> @@ +134,5 @@
> >          if (mParentStack.size() == 1) {
> >              return false;
> >          }
> >  
> > +        if (mListener != null) {
> 
> Do we actually expect mListener to ever be null here? If it is, aren't we in
> a bad state, since moveToParentFolder() wouldn't actually move to the parent
> folder?

Good point. Without this patch, the bad state would actually happen because we'd update the parent stack but not the cursor. With this patch, it will simply not do anything but remain in a valid state. So, not having a listener is 'fine' now.

> @@ +249,1 @@
> >          return (mParentStack.peek().id != Bookmarks.FIXED_ROOT_ID);
> 
> Can't we just change this whole method to return `mParentStack.size() != 0`?

Yep, although I kinda like the fact that this check is more explicit about the expected state.

Comment 13

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/78735552e391
https://hg.mozilla.org/integration/fx-team/rev/05cf1ce93054

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8334503 [details] [diff] [review]
Change Bookmark's parent stack to use a custom type (r=margaret)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: This is one the top crashers in Fx26 right now.
Testing completed (on m-c, etc.): Let's bake this in Nightly for a couple days and then uplift to Aurora and Beta.
Risk to taking this patch (and alternatives if risky): Mild risk. This is changing the way we update bookmarks lists in about:home's Bookmarks page. 
String or IDL/UUID changes made by this patch: n/a

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8334504 [details] [diff] [review]
Update parent stack and cursor at the same time (r=margaret)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: This is one the top crashers in Fx26 right now.
Testing completed (on m-c, etc.): Let's bake this in Nightly for a couple days and then uplift to Aurora and Beta.
Risk to taking this patch (and alternatives if risky): Mild risk. This is changing the way we update bookmarks lists in about:home's Bookmarks page. 
String or IDL/UUID changes made by this patch: n/a

Comment 16

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #12)

> > @@ +249,1 @@
> > >          return (mParentStack.peek().id != Bookmarks.FIXED_ROOT_ID);
> > 
> > Can't we just change this whole method to return `mParentStack.size() != 0`?
> 
> Yep, although I kinda like the fact that this check is more explicit about
> the expected state.

Yes, although now that we're checking mParentStack.size() at the beginning of the method, it's redundant to also have that second check, since mParentStack.peek().id should never equal Bookmarks.FIXED_ROOT_ID if mParentStack.size() isn't 0.

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #16)
> (In reply to Lucas Rocha (:lucasr) from comment #12)
> 
> > > @@ +249,1 @@
> > > >          return (mParentStack.peek().id != Bookmarks.FIXED_ROOT_ID);
> > > 
> > > Can't we just change this whole method to return `mParentStack.size() != 0`?
> > 
> > Yep, although I kinda like the fact that this check is more explicit about
> > the expected state.
> 
> Yes, although now that we're checking mParentStack.size() at the beginning
> of the method, it's redundant to also have that second check, since
> mParentStack.peek().id should never equal Bookmarks.FIXED_ROOT_ID if
> mParentStack.size() isn't 0.

The empty stack only happens just before we finish loading the root folder once the bookmark page is initially displayed. Once the root folder is loaded, we'll still append the FIXED_ROOT_ID to the stack. This hasn't really changed.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78735552e391
https://hg.mozilla.org/mozilla-central/rev/05cf1ce93054

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by -- letting this bake on Nightly, we go to build on Monday with the next mobile beta so will check in whether it's successfully landed & positively impacting crash data at that time.

Comment 20

[bhavana bajaj [:bajaj]]

(In reply to [e-mail] [:lsblakk] from comment #19)
> Triage drive-by -- letting this bake on Nightly, we go to build on Monday
> with the next mobile beta so will check in whether it's successfully landed
> & positively impacting crash data at that time.

Taking this on aurora to have better crash data stats or know fallouts for beta consideration.

Comment 21

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/edcf89fbe953
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4bb868db6fb

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8334503 [details] [diff] [review]
Change Bookmark's parent stack to use a custom type (r=margaret)

On IRC we discussed this and it's hard to tell but looks like this is less frequent so we'll take the low risk uplift.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/007e71a400d7
https://hg.mozilla.org/releases/mozilla-beta/rev/79ece4f7ff8f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/007e71a400d7
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/79ece4f7ff8f

Comment 25

[Chris Peterson [:cpeterson]]

Removing steps-wanted keyword because this bug has been resolved.

Comment 26

[Chris Peterson [:cpeterson]]

Removing steps-wanted keyword because this bug has been resolved.