Closed Bug 865005 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox21 --- affected
firefox22 + verified
firefox23 + verified
firefox24 --- verified
fennec + ---

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
tracking-fennec: --- → ?
https://hg.mozilla.org/mozilla-central/rev/0c6b9bedfa65
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
That changeset landed with the wrong bug #.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
Sorry -- Comment 1 belongs to bug 865006!
tracking-fennec: ? → 21+
Brian - Based on the flags you set, this probably is not a regression from bug 838793?
(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.
No longer blocks: 838793
tracking-fennec: 21+ → +
Status: NEW → ASSIGNED
Summary: OOM restore with private tabs open results in multiple about:home tabs → OOM restore with private tabs open results in multiple about:home tabs, and the wrong tab is selected
Duplicate of this bug: 871528
Target Milestone: Firefox 23 → ---
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.
Attachment #749558 - Flags: review?(mark.finkle)
Comment on attachment 749558 [details] [diff] [review]
Part 1: Refactor initialization and create loadStartupTab() for initial tab

Seems sane and covers the same cases
Attachment #749558 - Flags: review?(mark.finkle) → review+
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.
Attachment #749567 - Flags: review?(mark.finkle)
Some cleanup to move session restore code out of initialize() and make things a little more readable.
Attachment #749569 - Flags: review?(mark.finkle)
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).
Attachment #749576 - Flags: review?(mark.finkle)
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
Attachment #749567 - Flags: review?(mark.finkle) → review+
Attachment #749569 - Flags: review?(mark.finkle) → review+
Drops the forceSelected argument and merges the restore data into a single list before clamping the index and executing the onTabRead() callbacks.
Attachment #749576 - Attachment is obsolete: true
Attachment #749576 - Flags: review?(mark.finkle)
Attachment #750594 - Flags: review?(mark.finkle)
Comment on attachment 750594 [details] [diff] [review]
Part 4: Merge multiple session strings before restoring

\o/
Attachment #750594 - Flags: review?(mark.finkle) → review+
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
Attachment #749558 - Flags: approval-mozilla-beta?
Attachment #749558 - Flags: approval-mozilla-aurora?
Attachment #749567 - Flags: approval-mozilla-beta?
Attachment #749567 - Flags: approval-mozilla-aurora?
Attachment #749569 - Flags: approval-mozilla-beta?
Attachment #749569 - Flags: approval-mozilla-aurora?
Attachment #750594 - Flags: approval-mozilla-beta?
Attachment #750594 - Flags: approval-mozilla-aurora?
Landed in 24, not 23.
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.
Attachment #749558 - Flags: approval-mozilla-beta?
Attachment #749558 - Flags: approval-mozilla-beta+
Attachment #749558 - Flags: approval-mozilla-aurora?
Attachment #749558 - Flags: approval-mozilla-aurora+
Attachment #749567 - Flags: approval-mozilla-beta?
Attachment #749567 - Flags: approval-mozilla-beta+
Attachment #749567 - Flags: approval-mozilla-aurora?
Attachment #749567 - Flags: approval-mozilla-aurora+
Attachment #749569 - Flags: approval-mozilla-beta?
Attachment #749569 - Flags: approval-mozilla-beta+
Attachment #749569 - Flags: approval-mozilla-aurora?
Attachment #749569 - Flags: approval-mozilla-aurora+
Attachment #750594 - Flags: approval-mozilla-beta?
Attachment #750594 - Flags: approval-mozilla-beta+
Attachment #750594 - Flags: approval-mozilla-aurora?
Attachment #750594 - Flags: approval-mozilla-aurora+
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
Keywords: verifyme
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.
Keywords: verifyme
(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.
Mihai, can you verify this on Beta on the next build this week?
Flags: needinfo?(mihai.g.pop)
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.
Flags: needinfo?(mihai.g.pop)
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
Sorry, I meant: Firefox for Android 22 Beta 3 (2013-05-29)
I will set this Verified fixed as it was verified for all channels individually.
Status: RESOLVED → VERIFIED
Since the bug is verified fixed, I am removing the keyword:"verifyme"
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.