Closed Bug 929982 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect, critical)

All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
firefox28 --- fixed
b2g-v1.2 --- fixed
fennec 26+ ---

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

(Keywords: crash, steps-wanted, Whiteboard: [native-crash])

Crash Data

Attachments

(2 files)

tracking-fennec: --- → ?
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)
Severity: normal → critical
Keywords: crash, steps-wanted
Whiteboard: [native-crash]
Couldn't reproduce this in Fx26.
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.
Looks similar to bug 773944 from pre-fig. The "fix" there was to just do an instanceof check.
tracking-fennec: ? → 26+
Attachment #8334503 - Flags: review?(margaret.leibovic)
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.
Attachment #8334504 - Flags: review?(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.
Attachment #8334503 - Flags: review?(margaret.leibovic) → review+
Duplicate of this bug: 930644
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`?
Attachment #8334504 - Flags: review?(margaret.leibovic) → review+
(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.
(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 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
Attachment #8334503 - Flags: approval-mozilla-beta?
Attachment #8334503 - Flags: approval-mozilla-aurora?
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
Attachment #8334504 - Flags: approval-mozilla-beta?
Attachment #8334504 - Flags: approval-mozilla-aurora?
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/78735552e391
https://hg.mozilla.org/mozilla-central/rev/05cf1ce93054
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
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.
(In reply to lsblakk@mozilla.com [: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.
Attachment #8334503 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8334504 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 941825
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.
Attachment #8334503 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8334504 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.