Closed Bug 716681 Opened 8 years ago Closed 8 years ago

Tabs are being duplicated when going back into Fennec

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
blocking-fennec1.0 --- +
fennec 11+ ---

People

(Reporter: pwd.mozilla, Assigned: bnicholson)

References

Details

(Whiteboard: tabs-ux)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0a1) Gecko/20120109 Firefox/12.0a1
Build ID: 20120109085652

Steps to reproduce:

If you have tabs open in Fennec, go to another application and then reopen Fennec either via a link or via the apps shortcut. Existing tabs are duplicated in the tab list causing unnecessary overhead.
OS: Windows 7 → Android
Hardware: x86 → ARM
Paul, do you have any other steps you did to reproduce this? I'm on Nightly (01/13), tried this on the Galaxy SII/Nexus S/Nexus One/Droid Pro and couldn't reproduce.
Keywords: qawanted
I'm still seeing this using Nightly 01-14 but am unable to provide STR right now. I should have some time this evening to try and get clear STR.
The easiest way for me to rack up duplicate tabs is to browse my twitter stream, and click on links. I read the article, then press the Android back button, which takes me back to Twitter. Then I click on a new link, and then suddenly I have 3 tabs (2 dupes, and the new one). Or 5, if I'd left 2 tabs open before.
Duplicate of this bug: 719082
Glad it's not just me. I'm finding I can basically replicate Sean's STR using Tweetdeck and Fennec.
I'm not seeing this when the browser is already open. I only see this when the browser is cold-started.

Given that it's reproduce-able from a cold-start state, seems like it could be related to session restore code.
Status: UNCONFIRMED → NEW
Ever confirmed: true
sounds like session store fail.
Assignee: nobody → bnicholson
tracking-fennec: --- → 11+
Priority: -- → P1
I can't repro this. Based Sean and Paul's comments, I'm trying the following:
- Open Fennec from cold start (using force quit).
- Create a new tab.
- Press Home, open Tweetdeck.
- Find a tweet with a link; click it. A 3rd tab appears in Fennec with the link URL.
- Click back.
- Find another tweet with a link; click it. A 3rd tab appears again with the new URL.

Anything you guys are doing differently?
My steps are in bug 719082.
My device is a Galaxy S Vibrant.

I'm using the official Twitter client, if that matters. I imagine the RAM is filling and dumping Fennec, so opening a new link makes it restore.

The behavior is extremely consistent. I just popped open Fennec again, while writing this to double check. After having played Battleheart. It duplicated the tab I was on last night.
I can repro by going to Ars Technica on Galaxy Tab 10.1, opening 10 or 11 articles in new tabs so that the system OOM reaper kills the app and then reopening Nightly.
Whiteboard: tabs-ux
I can reproduce this on HTC Sensation, even without opening links from apps.
The trick seems to be to open a tab, then switch to another app using up a lot of RAM (e.g. the built-in browser loading 4 heavy websites) and finally switch back to Fennec again.
I see this happen when I update the nightly too. Updating will close Fennec and after the update, I end up with more tabs than needed.

I am sure if you use adb to kill Fennec it would do the same.
Here's the STR I've been using that reproduce this bug every time, on all devices (thanks to Josh the intern):
1) open any URL in Fennec
2) click Home to put Fennec in the background
3) execute oom-fennec (https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/)
4) open Fennec

This will duplicate the last open URL in another tab. The last tab is being restored by both GeckoApp's restore bundle and the session restore.
Attached patch patchSplinter Review
The session restore already knows the last open URI (along with all other tabs), so there's no need for GeckoApp to remember/send it.
Attachment #592016 - Flags: review?(mark.finkle)
Comment on attachment 592016 [details] [diff] [review]
patch

I love that this patch is removing mLastUri. I hope we can remove more cruft.

However, I am worried about one piece:

>-                if (lastHistoryEntry.mUri.equals(mLastUri))
>-                    return;
>-
>                 ViewportMetrics viewportMetrics = mSoftwareLayerClient.getGeckoViewportMetrics();
>                 if (viewportMetrics != null)
>                     mLastViewport = viewportMetrics.toJSON();
> 
>-                mLastUri = lastHistoryEntry.mUri;
>                 mLastTitle = lastHistoryEntry.mTitle;
>                 getAndProcessThumbnailForTab(tab);

This little check might be helping our performance by not saving a screenshot for a URI we already saved. I don't know for certain that this check is _really_ helping at all, but I would feel a lot better if you could maybe put some logging in and check opening pages and going into the background. See if the log output is the same with and without. If it is, great!

I also think Brad should take a look and check for reason we shouldn't remove the variable.

Even if we need to keep the variable for some reason, we still need a patch that does not send the URI to Gecko on startup.

r-, just until you do some tests
Attachment #592016 - Flags: review?(mark.finkle) → review-
Comment on attachment 592016 [details] [diff] [review]
patch

Forgot to add Brad
Attachment #592016 - Flags: review+
Comment on attachment 592016 [details] [diff] [review]
patch

I need to stop drinking so much...
Attachment #592016 - Flags: review+ → review?(blassey.bugs)
Attachment #592016 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/109c05e0b3a3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Brian, please request approval for aurora

Finkle, this landed with your r-, is that what you wanted?
I would like to know if the performance issue I warned about in comment 17 is real or not. Brian, what say you? Are we saving the damn screenshot bitmap when we don't need to?
Attachment #592016 - Flags: approval-mozilla-aurora?
As discussed in IRC, the if statement from comment 17 should only help for 1) page reloads, and 2) pausing/saving the state. We should be fine without it.
Comment on attachment 592016 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Beta 11.
Attachment #592016 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Verified fixed on:

Firefox 12.0a2 (2012-02-07)
20120207042017
http://hg.mozilla.org/releases/mozilla-aurora/rev/cd3f31d128b8
Device: HTC Desire Z
OS: Android 2.3.3

Firefox 11.0
20120206202409
http://hg.mozilla.org/releases/mozilla-beta/rev/1c0aba74d116
Device: HTC Desire Z
OS: Android 2.3.3
How can Fennec 13.0a1 be affected as the patch landed in 12.0a1?
(In reply to Scoobidiver from comment #28)
> How can Fennec 13.0a1 be affected as the patch landed in 12.0a1?

I tried this on Fennec 13.0a1 and it was reproducible. Indeed, the patch landed in 12.0a1, but it happened before the merge. I guess that the fix has merged from Fx12 to Fx13.
(In reply to Cristian Nicolae (:xti) from comment #29)
> I tried this on Fennec 13.0a1 and it was reproducible.
I am reopening it.

> Indeed, the patch landed in 12.0a1, but it happened before the merge.
There's no merge in mozilla-central, only in mozilla-aurora and mozilla-beta.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm unable to reproduce this in Fennec 13.0a1 using the steps from comment 15.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
I got this yesterday. While it's quite possible a fix has worked itself into the tree. I did end up with ten tabs.
(In reply to Paul [sabret00the] from comment #32)
> I got this yesterday. While it's quite possible a fix has worked itself into
> the tree. I did end up with ten tabs.

Using which STR? Have you tried the steps from comment 15? How many tabs did you have open before you had ten tabs? This may be another issue outside of the given bug fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Brian Nicholson (:bnicholson) from comment #33)
How do I go about doing step 3?
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → INVALID
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
What's the real status here? We are only concerned with 13.0a1
I've recently started seeing this again, only on links opened from other applications. On re-open of Fennec it seems to load the last link opened from another app *instead of* one of the tabs I previously had open.
(In reply to Sam Tobin-Hochstadt [:samth] from comment #36)
> I've recently started seeing this again, only on links opened from other
> applications. On re-open of Fennec it seems to load the last link opened
> from another app *instead of* one of the tabs I previously had open.

Tried on  Nightly 13.0a1 (2012-02-24)
 - Motorola Droid - Android 2.3.3
 - Samsung Nexus S -  Android 2.3.6
and was not able to reproduce issue. 

Marking bug as Resolved WORKSFORME. 
Sam, if you can still reproduce it, please reopen and provide more details: steps, device, Android version?
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Keywords: qawanted
Resolution: --- → WORKSFORME
blocking-fennec1.0: --- → +
I'm seeing this intermittently too. It's nowhere near as reproducible as it once was, but there is still an issue here.
(In reply to Paul [sabret00the] from comment #38)
> I'm seeing this intermittently too. It's nowhere near as reproducible as it
> once was, but there is still an issue here.

How often are you seeing this issue? What device are you using? Can you please provide some steps.
I'm seeing this often, but it's hard to pin down exactly what makes it happen. You will definitely see it if you use Nightly as your default browser though.

Opening links from Twitter, then running other applications, and later opening more links, I often find that it's duplicated my first tab. This happens over a long time period though, and I've not easily been able to reproduce it in a short time-span.

I also sometimes get it not opening the link I clicked on, but the last link that was being displayed. Sometimes pressing back after this situation crashes the browser. This is likely a separate issue, but may be relevant/related.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
I have a HTC Incredible S (Vivo) running CM7 nightly (Android 2.3.7) as for STRs, I haven't a clue, seems a lot more random now and less frequent. I've seen it one or twice last week at the most.
Hi Paul, can you try to keep track of how you are switching apps (ie are you hitting the home button and then the app, or are you hitting the back button after finishing reading the page or something else?) and which apps you are using in order to open the link please?  

The two things I am trying to understand is the code path of switching apps, and also the code path/intent of how fennec is being opened from the other app.  Maybe getting the data for those two situations may help resolve this issue?
There were two separate bugs that were causing this issue:
1) We were restoring the last URL in two places (GeckoApp and the SessionStore component). This has been fixed already in this bug.
2) When coming back from OOM, we were calling onResume() with the same intent we used to launch previously, rather than the new one being given to us in onNewIntent(). This will be addressed in bug 731333.

Marking as fixed since this bug fixed case #1. See bug 731333 for the second case.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
BTW, steps to reproduce case #2:
1) From a cold clean state (i.e., Fennec was previously quit using menu > Quit), launch Fennec from an external link, such as from a home screen shortcut
2) Send Fennec to background and run oom-fennec (https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/summary)
3) Again, launch Fennec from an external link
so if I understand things correctly there are now 4 states in which Fennec can exit:
a) quit through menu - which does not restore
b) kill through command - which does not restore
c) oom killed - which will restore
d) some other crashing... which should restore?

Is that correct?
(In reply to Naoki Hirata :nhirata from comment #45)
> so if I understand things correctly there are now 4 states in which Fennec
> can exit:
> a) quit through menu - which does not restore
> b) kill through command - which does not restore
> c) oom killed - which will restore
> d) some other crashing... which should restore?
> 
> Is that correct?

Currently, every kill/crash will do a restore - that is, anything other than menu > Quit. But this will change with bug 718240; once that lands, OOM kills will be the only case that will restore. Everything else will just go to about:home.
(In reply to Brian Nicholson (:bnicholson) from comment #46)
> (In reply to Naoki Hirata :nhirata from comment #45)
> > so if I understand things correctly there are now 4 states in which Fennec
> > can exit:
> > a) quit through menu - which does not restore
> > b) kill through command - which does not restore
> > c) oom killed - which will restore
> > d) some other crashing... which should restore?
> > 
> > Is that correct?
> 
> Currently, every kill/crash will do a restore - that is, anything other than
> menu > Quit. But this will change with bug 718240; once that lands, OOM
> kills will be the only case that will restore. Everything else will just go
> to about:home.

Will there be a pref for that? Such an implementation would kill all work-flow from within Fennec. Users should at least get a preference.
You need to log in before you can comment on or make changes to this bug.