Closed
Bug 865005
Opened 9 years ago
Closed 9 years ago
OOM restore with private tabs open results in multiple about:home tabs, and the wrong tab is selected
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox21 affected, firefox22+ verified, firefox23+ verified, firefox24 verified, fennec+)
VERIFIED
FIXED
Firefox 24
People
(Reporter: bnicholson, Assigned: bnicholson)
References
Details
Attachments
(4 files, 1 obsolete file)
5.80 KB,
patch
|
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.51 KB,
patch
|
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.11 KB,
patch
|
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.34 KB,
patch
|
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Updated•9 years ago
|
Comment 1•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0c6b9bedfa65
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Assignee | ||
Comment 2•9 years ago
|
||
That changeset landed with the wrong bug #.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•9 years ago
|
Status: REOPENED → NEW
![]() |
||
Comment 3•9 years ago
|
||
Sorry -- Comment 1 belongs to bug 865006!
Updated•9 years ago
|
tracking-fennec: ? → 21+
Comment 4•9 years ago
|
||
Brian - Based on the flags you set, this probably is not a regression from bug 838793?
Assignee | ||
Comment 5•9 years ago
|
||
(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
Updated•9 years ago
|
tracking-fennec: 21+ → +
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Updated•9 years ago
|
Target Milestone: Firefox 23 → ---
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Some cleanup to move session restore code out of initialize() and make things a little more readable.
Attachment #749569 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #749569 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 13•9 years ago
|
||
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 14•9 years ago
|
||
Comment on attachment 750594 [details] [diff] [review] Part 4: Merge multiple session strings before restoring \o/
Attachment #750594 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 15•9 years ago
|
||
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•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Assignee | ||
Comment 17•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Attachment #749567 -
Flags: approval-mozilla-beta?
Attachment #749567 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #749569 -
Flags: approval-mozilla-beta?
Attachment #749569 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #750594 -
Flags: approval-mozilla-beta?
Attachment #750594 -
Flags: approval-mozilla-aurora?
Comment 19•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #749567 -
Flags: approval-mozilla-beta?
Attachment #749567 -
Flags: approval-mozilla-beta+
Attachment #749567 -
Flags: approval-mozilla-aurora?
Attachment #749567 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #749569 -
Flags: approval-mozilla-beta?
Attachment #749569 -
Flags: approval-mozilla-beta+
Attachment #749569 -
Flags: approval-mozilla-aurora?
Attachment #749569 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #750594 -
Flags: approval-mozilla-beta?
Attachment #750594 -
Flags: approval-mozilla-beta+
Attachment #750594 -
Flags: approval-mozilla-aurora?
Attachment #750594 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
tracking-firefox22:
--- → +
tracking-firefox23:
--- → +
Comment 20•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Assignee | ||
Comment 23•9 years ago
|
||
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
Assignee | ||
Comment 24•9 years ago
|
||
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•9 years ago
|
||
Mihai, can you verify this on Beta on the next build this week?
Flags: needinfo?(mihai.g.pop)
Comment 26•9 years ago
|
||
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)
Comment 27•9 years ago
|
||
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•9 years ago
|
||
Sorry, I meant: Firefox for Android 22 Beta 3 (2013-05-29)
Comment 29•9 years ago
|
||
I will set this Verified fixed as it was verified for all channels individually.
Status: RESOLVED → VERIFIED
Comment 30•9 years ago
|
||
Since the bug is verified fixed, I am removing the keyword:"verifyme"
Keywords: verifyme
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•