1381359 - Crash in java.lang.IllegalStateException: Tried changing pinned state before it''s determined. at org.mozilla.gecko.home.HomeFragment$ToggleASPinTask.<init>(HomeFragment.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-8800fa74-6043-4ad0-aa0d-1fbf70170716.
=============================================================

This is the Fennec #1 topcrash in Nightly 20170714131541.

Here's the top part of the stack trace:

> java.lang.IllegalStateException: Tried changing pinned state before it's determined.
> at org.mozilla.gecko.home.HomeFragment$ToggleASPinTask.<init>(HomeFragment.java:461)
> at org.mozilla.gecko.home.HomeFragment.onContextItemSelected(HomeFragment.java:367)
> at org.mozilla.gecko.home.TopSitesPanel.onContextItemSelected(TopSitesPanel.java:369)
> at android.support.v4.app.Fragment.performContextItemSelected(Unknown Source)
> at android.support.v4.app.FragmentManagerImpl.dispatchContextItemSelected(Unknown Source)
> at android.support.v4.app.FragmentController.dispatchContextItemSelected(Unknown Source)
> at android.support.v4.app.FragmentActivity.onMenuItemSelected(Unknown Source)
> at android.support.v7.app.AppCompatActivity.onMenuItemSelected(Unknown Source)
> at android.support.v7.view.WindowCallbackWrapper.onMenuItemSelected(Unknown Source)
> at com.android.internal.policy.impl.PhoneWindow$DialogMenuCallback.onMenuItemSelected(PhoneWindow.java:4770)
> at com.android.internal.view.menu.MenuBuilder.dispatchMenuItemSelected(MenuBuilder.java:761)
> at com.android.internal.view.menu.MenuItemImpl.invoke(MenuItemImpl.java:152)
> at com.android.internal.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:904)
> at com.android.internal.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:894)
> at com.android.internal.view.menu.MenuDialogHelper.onClick(MenuDialogHelper.java:180)
> at com.android.internal.app.AlertController$AlertParams$3.onItemClick(AlertController.java:1106)
> at android.widget.AdapterView.performItemClick(AdapterView.java:305)
> at android.widget.AbsListView.performItemClick(AbsListView.java:1166)
> at android.widget.AbsListView$PerformClick.run(AbsListView.java:3073)
> at android.widget.AbsListView.onTouchUp(AbsListView.java:3907)
> at android.widget.AbsListView.onTouchEvent(AbsListView.java:3666)
> at android.view.View.dispatchTouchEvent(View.java:8744)

First occurrence is in the July 14 Nightly, which suggests this regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=30ea2905130e85f9e1d8d56fa3097901eec6514b&tochange=400584289c8fa44060d6443a3288ef57cf73ff4e

Bug 1377286 is probably the cause. Grisha, can you please investigate?

Comment 1

[Wesly Huang (EPM)]

Grisha, I was going to add "[MobileAS]" in whiteboard but then think it's good to ask in advance. Is it truly related to Bug 1377286? If not please let us know since it's FE related. Thanks.

Comment 2

[Oana Horvath [:ohorvath]]

On Nightly 56 there are 2 "Pin site" options in the Top sites context menu. The second one, located under the Remove option, triggers this crash.

build: Nightly 56.0a1 (2017-07-19).

Comment 3

[Oana Horvath [:ohorvath]]

(In reply to Oana Horvath from comment #2)
> On Nightly 56 there are 2 "Pin site" options in the Top sites context menu.
> The second one, located under the Remove option, triggers this crash.
> 
> build: Nightly 56.0a1 (2017-07-19).

note: Need to mention that it's on the old Top sites panel, with Activity stream off.

Comment 4

[Wesly Huang (EPM)]

[traige@0726] +, already taken.

Comment 5

[Calixte Denizet (:calixte)]

This signature is ranked #4 in top-crashers.

Comment 6

[:Grisha Kruglov]

Activity Stream is enabled by default for 57, without ability to disable it to access classic top sites. So a fix here would be a direct uplift.

Comment 7

[:Grisha Kruglov]

Attachment: Bug 1381359 - Hide AS Pin Site item from classic Top Sites context menu

Our home panel context menu is shared by classic top sites, history and bookmark panels.
For acitivty stream, we've added a new context menu, and a new type of pins. However,
old context menu is still used for history and bookmarks. New style of pins allows for
items to be pinned from those panels to activity stream. This requires having both "classic"
pin buttons for old Top Sites panel, as well as the new AS Pin button.

However, initial implementation forgot to disable the new AS Pin button for the classic
Top Sites panel context menu. This caused two "pin site" menu items to appear, and the
AS one would crash the browser if it's interacted with.

We'll be unifying context menus at some point, and removing the old Top Sites code,
and so this simple patch should be sufficient.

Review commit: https://reviewboard.mozilla.org/r/167698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167698/

Comment 8

[:Grisha Kruglov]

Attachment: hide-as-pin-top-sites-beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1377286

[User impact if declined]: Users will see a duplicate "Pin Site" item on the Top Sites context menu. If they interact with it, their browser will crash.

[Is this code covered by automated tests?]: No.

[Has the fix been verified in Nightly?]: This patch is a no-op on nightly, since the classic Top Sites are disabled there.

[Needs manual test from QE? If yes, steps to reproduce]: In beta, open regular (non-Activity Stream) top sites. Long press on any item in the top sites grid. You should only see one Pin Site item, and pressing that item should work.

[List of other uplifts needed for the feature/fix]: N/A

[Is the change risky?]: No

[Why is the change risky/not risky?]: Very simple change, simply turning off a menu item for context menu on one panel.

[String changes made/needed]: N/A

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8896448 [details]
Bug 1381359 - Hide AS Pin Site item from classic Top Sites context menu

https://reviewboard.mozilla.org/r/167698/#review172942

lgtm, disabling activity stream pin in non-AS panel. home_as_pin is only present in home_contextmenu and TopSitesPanel is the only non-AS file to use that context menu, so there shouldn't anywhere else to disable it.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a19e930e2793
Hide AS Pin Site item from classic Top Sites context menu r=liuche

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a19e930e2793

Comment 12

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8896450 [details] [diff] [review]
hide-as-pin-top-sites-beta.patch

Fix a top crash. Beta56+. Should be in 56.0b3.

Comment 14

[Gerry Chang [:gchang]]

Track 56+ as top crash.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ae9b45662dd

Comment 16

[u549602]

Verified as fixed on the latest Beta build (56.0b3).
Tested on a Samsung Galaxy S8 (Android 7.0) and on a Nexus 6P (Android 8.0).