865005 - OOM restore with private tabs open results in multiple about:home tabs, and the wrong tab is selected

Status: VERIFIED FIXED

(Firefox for Android :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open at least one private tab
2) Put Fennec in background and cause it to OOM
3) Reopen Fennec

After this, an extra about:home tab appears, and it's selected at startup instead of the private tab. Repeating these steps results in an extra about:home tab every time.

Likely a regression from bug 838793.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c6b9bedfa65

Comment 2

[Brian Nicholson (:bnicholson)]

That changeset landed with the wrong bug #.

Comment 3

[Geoff Brown [:gbrown]]

Sorry -- Comment 1 belongs to bug 865006!

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Brian - Based on the flags you set, this probably is not a regression from bug 838793?

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Brian - Based on the flags you set, this probably is not a regression from
> bug 838793?

Yeah, this bug has been around longer than that.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Refactor initialization and create loadStartupTab() for initial tab

There were a couple causes of this bug. The first is that we have a race condition in initializeChrome() and the session restore logic. The session restore logic creates tabs, firing a chain of events that eventually ends up calling GeckoApp#requestRender(). requestRender() uses mLayerView, but mLayerView isn't initialized until initializeChrome(), which is called *after* the session restore logic. We don't normally hit the NPE here because the tab callbacks are fired in runnables, but it's obviously fragile code that needs fixing.

This patch moves the tab creation parts of initializeChrome() into a separate method, loadStartupTab(). This allows us to call initializeChrome() before the session restore, and loadStartupTab() after.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 749558 [details] [diff] [review]
Part 1: Refactor initialization and create loadStartupTab() for initial tab

Seems sane and covers the same cases

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Use SessionRestoreException to avoid swallowing unrelated exceptions

The NPE from comment 7 shouldn't have been swallowed by the try/catch block in the session restore code. Since onTabRead() synchronously calls lots of other code through loadUrl(), the try/catch block in the session restore ends up catching unrelated exceptions. To prevent that, we can create and catch our own SessionRestoreException instead of using a generic catch-all Exception, which is generally considered bad practice.

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Move session restore code into restoreSessionTabs()

Some cleanup to move session restore code out of initialize() and make things a little more readable.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Only allow one tab to be selected during restore

The second cause of this bug, in addition to the race condition in comment 7, is that we select multiple tabs at startup. SessionParser has code that makes sure a tab is selected in case the selected index is out of bounds or missing. Problem is, the index *will* be missing if we're restoring a private tab that was previously selected. We save/restore private and non-private tabs as two completely separate bits of JSON, and each bit has its own window object, collection of tabs, and selected index. So when we restore non-private tabs, and the last selected tab was a private one, the first non-private tab will end up being selected. Then, we restore the private tabs, and the correct private tab is selected afterwards.

It's a bit tricky since we handle the private/non-private data as separate entities, yet we need to make sure that between them, only one tab gets selected. The approach I took in this patch was to add a forceSelected boolean to optionally prevent the selected index clamping behavior we have now. Now, if we restore private tabs first and set forceSelected to false, the first round of tab restores won't necessarily result in a selected tab. We then restore the non-private tabs, and by setting forceSelected to true, we'll make sure that at least one of them is marked as selected (but if a private tab was already selected, we can just ignore it).

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 749567 [details] [diff] [review]
Part 2: Use SessionRestoreException to avoid swallowing unrelated exceptions

Log.e is not my favorite, but we should examine those in a different bug

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Merge multiple session strings before restoring

Drops the forceSelected argument and merges the restore data into a single list before clamping the index and executing the onTabRead() callbacks.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 750594 [details] [diff] [review]
Part 4: Merge multiple session strings before restoring

\o/

Comment 15

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3700ac5488a
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f4d65e3bf0
https://hg.mozilla.org/integration/mozilla-inbound/rev/52406c8a57bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a78a455ebf8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3700ac5488a
https://hg.mozilla.org/mozilla-central/rev/42f4d65e3bf0
https://hg.mozilla.org/mozilla-central/rev/52406c8a57bc
https://hg.mozilla.org/mozilla-central/rev/5a78a455ebf8

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 749558 [details] [diff] [review]
Part 1: Refactor initialization and create loadStartupTab() for initial tab

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: Broken session restore for any private browsing tabs. The wrong tab can be selected after the restore, and extra about:home tabs are added at startup.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk -- mostly cleanup and simple refactoring.
String or IDL/UUID changes made by this patch: none

Comment 18

[Brian Nicholson (:bnicholson)]

Landed in 24, not 23.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 749558 [details] [diff] [review]
Part 1: Refactor initialization and create loadStartupTab() for initial tab

Since this is still early in Beta cycle and this is low risk/cleanup that brings a solid win to releasing pb on android, approving for uplift.

Comment 20

[u421692]

Verified fixed on:
Build: Firefox for Android 24.0a1 (2013-05-23)/Firefox for Android 22.0b2(2013-05-22)/Firefox for Android 23.0a2(2013-05-24)
Device: Samsung Galaxy Tab
OS: Android 4.0.4

Comment 21

[u421692]

Since I was unable to reproduce this issue on firefox 22 and firefox 23, killing the app with a large image(http://bit.ly/Z51mKb), I marked the issues as verified. After modifying the flags I observed that the patch was not integrated in firefox 22 and firefox 23. Please ignore comment 20, since the correct way to reproduce this issue is using Advanced task killer app. Changing back flags.

Comment 22

[Brian Nicholson (:bnicholson)]

(In reply to Pop Mihai from comment #21)
> Since I was unable to reproduce this issue on firefox 22 and firefox 23,
> killing the app with a large image(http://bit.ly/Z51mKb), I marked the
> issues as verified. After modifying the flags I observed that the patch was
> not integrated in firefox 22 and firefox 23. Please ignore comment 20, since
> the correct way to reproduce this issue is using Advanced task killer app.
> Changing back flags.

The easiest way to reproduce the OOM part of this bug is to put Fennec in the background and open several other apps. Fennec has to be in the background for Android's state save/restore logic to kick in, so causing an OOM crash within Fennec itself won't work. Using task killers or force quitting also won't work. If Fennec is killed with any of these methods, your private session won't even be restored in the first place.

Comment 23

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/954bbaf1fcd8
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc069968237b
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8d0a361f320
https://hg.mozilla.org/releases/mozilla-aurora/rev/6664ebea6eae

Comment 24

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/156f0ea23214
https://hg.mozilla.org/releases/mozilla-beta/rev/6a8bef2510ef
https://hg.mozilla.org/releases/mozilla-beta/rev/899cb38d7fe4
https://hg.mozilla.org/releases/mozilla-beta/rev/7fe17ec4ea29

Comment 25

[Aaron Train [:aaronmt]]

Mihai, can you verify this on Beta on the next build this week?

Comment 26

[u421692]

Verified fixed on:
Build: Firefox for Android 23.0a2 (2013-05-27)
Device: Samsung Galaxy Tab
OS: Android 4.0.4
The issue does not reproduce on latest Aurora build, focus is kept on the correct tab, no extra about:home tab is opened. Waiting for the next beta build to verify.

Comment 27

[Teodora Vermesan (:TeoVermesan)]

At startup, no extra about:home tab appears and the private tab is selected. So, verified fixed on:
Build: Firefox for Android 22 Beta 2 
Device: Samsung Galaxy Nexus
OS: Android 4.1.1

Comment 28

[Teodora Vermesan (:TeoVermesan)]

Sorry, I meant: Firefox for Android 22 Beta 3 (2013-05-29)

Comment 29

[Ioana Chiorean]

I will set this Verified fixed as it was verified for all channels individually.

Comment 30

[Teodora Vermesan (:TeoVermesan)]

Since the bug is verified fixed, I am removing the keyword:"verifyme"