Closed Bug 1120441 Opened 5 years ago Closed 3 years ago

crash in java.lang.IllegalStateException: Can not perform this action after onSaveInstanceState at android.support.v4.app.FragmentManagerImpl.checkStateLoss(Unknown Source)

Categories

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

37 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: aaronmt, Assigned: ahunt)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-5931c08a-a924-42df-9115-f97402150111.
=============================================================

java.lang.IllegalStateException: Can not perform this action after onSaveInstanceState
	at android.support.v4.app.FragmentManagerImpl.checkStateLoss(Unknown Source)
	at android.support.v4.app.FragmentManagerImpl.enqueueAction(Unknown Source)
	at android.support.v4.app.BackStackRecord.commitInternal(Unknown Source)
	at android.support.v4.app.BackStackRecord.commit(Unknown Source)
	at org.mozilla.gecko.tabs.TabHistoryFragment.show(TabHistoryFragment.java:125)
	at org.mozilla.gecko.BrowserApp$3$1.run(BrowserApp.java:535)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:135)
	at android.app.ActivityThread.main(ActivityThread.java:5221)
	at java.lang.reflect.Method.invoke(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:372)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:899)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:694)
Looks like this is dealing with the go back session menu from bug 1093209. CC'ed Vivek.
Assignee: nobody → vivekb.balakrishnan
Crash Signature: [@ java.lang.IllegalStateException: Can not perform this action after onSaveInstanceState at android.support.v4.app.FragmentManagerImpl.checkStateLoss(Unknown Source)] → [@ java.lang.IllegalStateException: Can not perform this action after onSaveInstanceState at android.support.v4.app.FragmentManagerImpl.checkStateLoss(Unknown Source)] [@ java.lang.IllegalStateException: Can not perform this action after onSaveInstanceSt…
From the javadocs [1]:

Note: A fragment transaction can only be created/committed prior to an activity saving its state. If you try to commit a transaction after Activity.onSaveInstanceState() (and prior to a following Activity.onStart or Activity.onResume(), you will get an error. This is because the framework takes care of saving your current fragments in the state, and if changes are made after the state is saved then they will be lost.

[1]: https://developer.android.com/reference/android/app/FragmentManager.html#beginTransaction%28%29
To be more explicit, the method that calls into TabHistoryFragment is defined in BrowserApp.onCreate:

        tabHistoryController = new TabHistoryController(new OnShowTabHistory() {
            @Override
            public void onShowHistory(final List<TabHistoryPage> historyPageList, final int toIndex) {
                runOnUiThread(new Runnable() {
                    @Override
                    public void run() {
                        final TabHistoryFragment fragment = TabHistoryFragment.newInstance(historyPageList, toIndex);
                        final FragmentManager fragmentManager = getSupportFragmentManager();
                        GeckoAppShell.vibrateOnHapticFeedbackEnabled(getResources().getIntArray(R.array.long_press_vibrate_msec));
                        fragment.show(R.id.tab_history_panel, fragmentManager.beginTransaction(), TAB_HISTORY_FRAGMENT_TAG);
                    }
                });
            }
        });

And TabHistoryFragment.show:

    // Function to add this fragment to activity state with containerViewId as parent.
    // This similar in functionality to DialogFragment.show() except that containerId is provided here.
    public void show(final int containerViewId, final FragmentTransaction transaction, final String tag) {
        dismissed = false;
        transaction.add(containerViewId, this, tag);
        transaction.addToBackStack(tag);
        backStackId = transaction.commit();
    }

Line 125 is `transaction.commit();` and the javadoc from comment 2 comes from FragmentManager.beginTransaction.
And the javadoc from commit [1]:

Schedules a commit of this transaction. The commit does not happen immediately; it will be scheduled as work on the main thread to be done the next time that thread is ready.

A transaction can only be committed with this method prior to its containing activity saving its state. If the commit is attempted after that point, an exception will be thrown. This is because the state after the commit can be lost if the activity needs to be restored from its state. See commitAllowingStateLoss() for situations where it may be okay to lose the commit.

[1]: https://developer.android.com/reference/android/support/v4/app/FragmentTransaction.html#commit%28%29
Quick and easy fix: change transaction.commit to transaction.commitAllowingStateLoss.

Correct-fix next steps: figure out how we're hitting the state where Activity.onSaveInstanceState is called before transaction.commit. I'd guess it's related to how onShowHistory pushes off the Fragment methods to the UiThread, so maybe the UI is pressed to make the fragment changes, then the Activity starts to finish, and then this Runnable with the transaction commit runs, causing the failure.

The stack trace in comment 0 references onKeyDown (by line numbers in BrowserApp) so I'm thinking maybe this has something to do with hitting the back button which starts to finish the browser and then hitting it again to trigger the tab history menu (or vice versa).
(In reply to Michael Comella (:mcomella) from comment #5)
> Quick and easy fix: change transaction.commit to
> transaction.commitAllowingStateLoss.

I don't know the side effects for this.
To be explicit: no longer looking into this.
NI liuche because she said she was looking at crash bugs but don't let that stop someone else from digging in.
Flags: needinfo?(liuche)
Some notes:
http://www.androiddesignpatterns.com/2013/08/fragment-transaction-commit-state-loss.html

Another alternative to try is see if right after calling commit(), also call getFragmentManager().executePendingTransactions(). There may be some side effects, where other pending transactions may also be executed. (In Android N, there is a commitNow() API that commits and then executes that one transaction, but that's not available here.)
Flags: needinfo?(liuche)
Taking this, since it's still quite significant, #19 on release right now (it's also not visible on nightly, which makes testing the fix harder...).

Between the back-button long-press, and the fragment being shown, TabHistoryController sends Gecko request to Gecko, which can sometimes be very slow (I've experienced multi-second delays between long-pressing a link and its context-menu appearing, so I can imagine the same happens here). By the time that request returns, and onShowHistory() is called, and who knows what state the app will be in. We probably just need to check if the app is even running at that point (or otherwise the crash happens because we've shut down already).

I'm not sure how we to check that we're still in a good state in onShowHistory(): setting a flag in onSaveInstanceState() seems like the most foolproof approach, but I'll spend some more time figuring out if there's a more elegant way of querying Activity state. (There might not well be, and there might not be an unambiguous way of doing so given the changes in onSaveInstanceState() is called between Android versions.)


FWIW I'm surprised that this crash is so common - I feel like the tab-history popup isn't that well known (but we don't seem to have telemetry for it). One possible scenario is that the app is being slow, pressing back won't hide the app, users long-press back out of frustration (which triggers the popup), and at some point during that time onSaveInstanceState() has been called and we therefore crash when the gecko-request (which will be extra slow) finally returns?
Assignee: vivekb.balakrishnan → ahunt
A much simpler solution than the above is to use commitAllowingStateLoss(): we actually _do_ want that this in this scenario: if the app is shutting down it doesn't really matter if we lose the popup state.

I.e. it would be nice to save the state, but in this case the only true "fix" is to make gecko return immediately - and in the meantime we can either lose state on the rare occasion that gecko was too slow, or implement complicated code to save the state ourselves.

To summarise, the solutions appear to be:
- Make gecko return immediately to "Session:GetHistory" calls (not feasible)
- Cache the session history in fennec frontend (lots of code, minimal user value)
- Save the fact that we requested the tab history panel, and try again the next time fennec is started (seems complex, minimal user value)
- Give up if the app is being shut down.

The user impact of the last solution seems minimal - if they've already decided to shut down the app, they probably won't care that we ignored their request to show tab history (and we don't even know if they actively wanted the panel, or activated it accidentally).
Depending on how far along things are, the activity might have already been destroyed (I've seen a few reports online of such crashes in other apps) - so to be completely safe we want to both check whether the activity still exists AND allow state loss:

The relevant commit implementation code (that does the state-loss and Activity liveliness checks):
http://androidxref.com/7.1.1_r6/xref/frameworks/support/fragment/java/android/support/v4/app/FragmentManager.java#1554
Comment on attachment 8828113 [details]
Bug 1120441 - Don't try to show tab-history panel if app has been shutdown

https://reviewboard.mozilla.org/r/105628/#review109226
Attachment #8828113 - Flags: review?(s.kaspari) → review+
Pushed by ahunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/771197f30d4a
Don't try to show tab-history panel if app has been shutdown r=sebastian
I'll let this stabilise on nightly for a few days before requesting uplift.

Currently this crash isn't visible on aurora 53, but is visible on aurora 52 (it seems some people haven't updated yet, since aurora 53 crash numbers are low). It's also cleary visible on beta 52, so we should probably just uplift to both to see if it helps. (It's occasionally visible on nightly, but there haven't been any instances in the past week - so nightly isn't all that useful for verifying that the fix works.)
https://hg.mozilla.org/mozilla-central/rev/771197f30d4a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment on attachment 8828113 [details]
Bug 1120441 - Don't try to show tab-history panel if app has been shutdown

Approval Request Comment
[Feature/Bug causing the regression]: Probably bug 1093209
[User impact if declined]: Occasional crashes while (or after) closing the app. This is the no. 12 top-crash on release.
[Is this code covered by automated tests?]: no.
[Has the fix been verified in Nightly?]: no - the crash volume on nightly is too low to verify (happens maybe once a month). The crash is more significant on aurora and beta: most users still seem to be stuck on aurora 52, so aurora is probably not useful enough to verify. The crash is clearly visible on beta, hence I'd like to uplift there to verify the fix.
[Needs manual test from QE? If yes, steps to reproduce]: no - effectiveness should be visible in crashstats.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: low risk.
[Why is the change risky/not risky?]: The patch adds two extra conditionals, which prevent the crashing code from being run when unsafe (i.e. we now protect against the app being shut down).
[String changes made/needed]: none.
Attachment #8828113 - Flags: approval-mozilla-beta?
Attachment #8828113 - Flags: approval-mozilla-aurora?
Comment on attachment 8828113 [details]
Bug 1120441 - Don't try to show tab-history panel if app has been shutdown

I looked at the crash signature and Nightly data since this fix landed. We don't have data (or hits) from Nightly that prove whether this fix works. Even so it makes sense to uplift to Aurora53 at least.
Attachment #8828113 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8828113 [details]
Bug 1120441 - Don't try to show tab-history panel if app has been shutdown

fennec crash fix, let's take this in beta52.

There's some broken indentation being introduced with the isForegrounded conditional in this patch :(
Attachment #8828113 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1336084
Uggh, I messed up here and pushed a non-updated patch to RB:

The if (BrowserApp.this.isForegrounded()) condition isn't needed at all (but isn't harmful either). I'll provide a new patch for Beta without it, and remove the unnecessary call in Bug 1120441. (I put the call there while trying to debug, but I was meaning to remove it since it's pretty much useless.)
Flags: needinfo?(ahunt)
Update patch for beta without the offending unnecessary isForegrounded() call.
No crashes were encountered in our 52.0b4 test session held today hence i'll leave this issue as is and have a look on the crash stats after the build is accessed by a larger number of users.
Small note: it looks like there's a crash with the same signature on the latest beta, however that crash is in a different location (and smaller volume - i.e. it's now the 34th top-crash). I've filed Bug 1338670 to look into that, however due to the small volume it's probably not an immediate priority to fix (i.e. we have higher-impact crashers that could be fixed before that).
See Also: → 1427076
You need to log in before you can comment on or make changes to this bug.