1044556 - Tabs unloaded from memory occasionally forget their browsing history and current page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Jan Henning [:JanH]]

User Agent: Mozilla/5.0 (Android; Mobile; rv:31.0) Gecko/31.0 Firefox/31.0 (Nightly/Aurora)
Build ID: 20140717124120

Steps to reproduce:

1. Open a new tab and load an arbitrary page which we'll call page A. Then navigate around by clicking on links, so that e.g. your browsing history consists of pages A, B and C, and the page currently open in that tab is page D.
2. Open some other tabs and try to get the tab from step 1 unloaded from memory. I don't know whether getting Firefox as a whole evicted from memory (by switching to other apps) is also sufficient to trigger this behaviour.
3. Switch back to the original tab from step 1. If that tab has indeed been unloaded from memory, Firefox will now try reloading it.

My setup: FF31 on a Galaxy S3 Mini running stock Android 4.1.2


Actual results:

 Occasionally Firefox forgets the browsing history of that tab and will reload the page you initially opened in that tab, i.e. page A. Unfortunately this issue doesn't seem to be reproducible 100% of the time, however I've encountered it often enough during normal browsing.

If I remember correctly, I've encountered this issue since I started using Firefox for Android with FF29.


Expected results:

That tab should open at page D, where I last left it and with its browsing history intact.

Comment 1

[matcheydj]

Hi, this seems very similar to a basic Android memory issue which may affect individual apps, like browsers. If you go too far away from an unused activity, or extension of (like a new tab within the main browser activity), it will be unloaded from memory. Even with changing the number of background processes in Android's developer options, it is not possible to extend this limit beyond the standard, for the sake of memory performance.

Also, some web pages' HTML is coded to refresh upon access, so it also depends which site is being navigated.

There is some more explanation here: https://support.mozilla.org/en-US/questions/968188

Comment 2

[Jan Henning [:JanH]]

Thank you, however I'm not so much concerned about the fact that tabs get unloaded from memory - I've already realised why that happens - but more about the problem that upon reopening such a tab, sometimes, but not always, Firefox forgets the browsing history of that tab and resets the current page to the one I initially opened there.

Comment 3

[matcheydj]

Okay, I understand better now after re-reading. The only thing I can think of is to check the website you're accessing, since some might redirect to a home page regardless of which page is requested, for example to log in. Sorry I can't be of more help.

Comment 4

[Oliver Henshaw]

This can happen when page A and D aren't on the same site, so I doubt that's it. I'm sure I saw another bug report related to this some time ago, but can't find it now.

Comment 5

[Oliver Henshaw]

Another report at bug 1004964. I'm sure I've seen one with some analysis though.

Comment 6

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

Brian - Can you take a look at this (and bug 1004964)

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 8

[Jan Henning [:JanH]]

This still happens occasionally, although I've now found that sometimes Firefox manages to restore a later page than the initially opened one - i.e. after browsing through pages A, B, C, D and E, when reloading that tab after it getting unloaded from memory, Firefox:
- most of the time opens page E (expected behaviour)
- sometimes opens page A (as described above)
- and, unless I'm mistaken, sometimes opens one out of pages B through D.

I don't know whether this should be a different bug or not, but I've also found that if I close a tab shortly before exiting/switching away from Firefox, after reopening Firefox, occasionally that tab reappears in the tab list as if I had never closed it. I can't remember at the moment though whether that tab appears at its original position in the tab list, or whether it gets inserted at the end of the list.

Comment 9

[Oliver Henshaw]

This has annoyed me enough that I've been digging into it, and have found a couple of potential problems.

* SessionStore.js does a sync write on getting a onPause call from android (when the browser is hidden by e.g. a pulldown menu or the app switcher button). If there's already an async write in flight then the older write could complete before the newer one. If android kills firefox before the next async write then you'll lose some (a few seconds of) history.

I've built fennec with logging and running it in the android emulator to try and confirm this, but the emulator's pretty temperamental and I haven't confirmed it yet.

But I've definitely seen this happen just from switching tabs and back, never leaving firefox or having anything else in the foreground. At least some of the time that an old page is reloaded there's a forward button in the firefox menu, but it doesn't work. So maybe the SS_data and tab history get out of sync?

I did notice something fishy when tabs are unloaded ("zombified") on low memory events, but I'm not sure how that could lead to this bug:

* when least-recently-used tabs are zombified, the code checks whether the tab is already a zombie; but when all unloaded tabs are zombified (on a low-memory event) this check is not done. Then when zombifying a zombie, the tab is briefly recreated with the oldest url from session data (rather than the current url for the tab). The correct session data is reattached to the tab right away, so it's difficult to see how this can cause the bug, but maybe some event is fired off with the wrong data from when the tab was created?

The "Tabs:Added" message handler in mobile/android/base/Tabs.java looks suspicious. I don't think any events subsequently fired from addTab() are harmful, but much more happens as a result of selectTab(). Someone more familiar with fennec internals might have a better idea what happens here, but in any case I don't think it's correct to try to select tabs in mid-zombification is it?

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Oliver Henshaw from comment #9)
> If there's already an async write in flight then the older write
> could complete before the newer one.

I assume you mean *after* the newer one. Given the tiny window (likely just a few milliseconds, not seconds), I'd be surprised if this is something you're hitting regularly, but I agree this is worth fixing.

Comment 11

[Oliver Henshaw]

(In reply to Brian Nicholson (:bnicholson) from comment #10)
> (In reply to Oliver Henshaw from comment #9)
> > If there's already an async write in flight then the older write
> > could complete before the newer one.
> 
> I assume you mean *after* the newer one. Given the tiny window (likely just
> a few milliseconds, not seconds), I'd be surprised if this is something
> you're hitting regularly, but I agree this is worth fixing.

Yes, after. I have seen the "atomic" async write take three seconds though, not sure why. I think maybe it can take seconds if there are (long) enough OS.File operation queued before the atomic write.

Comment 13

[Oliver Henshaw]

This still happens in current beta (will be firefox 35). I will test with current aurora (will be firefox 36).

Comment 14

[Oliver Henshaw]

Still happens with aurora 36.0a2

Not sure whether this is new behaviour, but I've noticed that the zombified tab is reloaded from the cache - e.g. I was reading a 4 page forum thread that had expanded to 9 pages while I was reading it. After loading page 7 I switched away and when I switched back some time later I was on page 1 but the thread navigation only showed 4 pages. A page reload fixed that.

Still can't get a reliable reproducer, but I suspect it happens when switching to a different tab while the new page is loading. And I think the bug is more likely to be in the tab history than the session store data (or maybe some interaction between the two).

Comment 15

[Oliver Henshaw]

Still happens with 37.0a2 (2015-01-23)

I had hoped that the Dec 5 commit for bug #1097098 (i only have the gitbub commit hash, not hg) might fix it, but no.

Comment 17

[Nicolae Albu]

Still happens on 38.0.5...

Comment 19

[Jan Henning [:JanH]]

My debugging means are rather limited, but I've tried monitoring changes to <profile folder>/sessionstore.js to see whether that might lead to something.
For this test, so far I've only managed to recreate the case where after reloading, the forward arrow has become greyed out.
What I've found is that once a tab has been zombified at least once, any further navigation actions within that tab, whether by clicking on links, pressing the forward/backward/reload buttons or manually entering a new URL don't cause writes to sessionstore.js any more. Therefore from that point onward, its session store history becomes effectively frozen.

This behaviour would also fit the observation reported in bug 1160850 comment #2.

It also explains why it's not always the first page you visited that gets erroneously reloaded: It's the page that was loaded while the tab was first zombified within the current browsing session that is getting retained instead.

A complete restart (e.g. by exiting Firefox and swiping it away in the task manager) fixes this behaviour for all tabs, but of course the next low-memory-event triggers it again.

Comment 20

[Jan Henning [:JanH]]

I've done some further research and I think I've found the cause. I had to use an older nightly version (20150306030229) because debugging the main process is currently broken (see bug 1170495), but I think my results are still valid.

What I've found is that after a tab has been zombified, the session restore event handler (http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#202) doesn't get called any more for any "DOMTitleChanged" events. As zombifying a tab involves destroying a tab and then recreating it in a zombified state (http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/MemoryObserver.js#52), I assume that during the process the DOMTitleChanged event listener created by the session store gets destroyed. As tab.create (in browser.js) by itself doesn't fire any "TabOpen" events (that only happens when addTab is called), the session store has no way of learning about the freshly recreated tab and reattaching its relevant listeners.

Comment 21

[Richard Newman [:rnewman]]

I figure Brian's not working on this, so let's get this back into triage and get an assignee.

Comment 22

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

Sebastian - Can you take a look at this sometime and see if you can work with JanH to nail down a possible root cause?

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mark Finkle (:mfinkle) from comment #22)
> Sebastian - Can you take a look at this sometime and see if you can work
> with JanH to nail down a possible root cause?

Sure! I will need some time to dig into this area of the code and understand what's going on but this is probably a good exercise anyways.

Comment 25

[donrhummy]

(In reply to Sebastian Kaspari (:sebastian) from comment #23)
> (In reply to Mark Finkle (:mfinkle) from comment #22)
> > Sebastian - Can you take a look at this sometime and see if you can work
> > with JanH to nail down a possible root cause?
> 
> Sure! I will need some time to dig into this area of the code and understand
> what's going on but this is probably a good exercise anyways.

Is there anything we users can do to help you narrow down the root cause?

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to donrhummy from comment #25)
> Is there anything we users can do to help you narrow down the root cause?

I'm having a hard time reproducing this issue. I tried on my Nexus 5/9 and on a HTC Desire HD running Gingerbread (I assume a memory constraint device will trigger this more easily?) but with no luck so far.

I wish I could find STR that trigger this at least > 50% of the time - to automate this for local debugging.

Comment 27

[Jan Henning [:JanH]]

If you might be able to manually trigger this:

https://dxr.mozilla.org/mozilla-central/rev/cdf53a4dee219aabec7462888875bac08b093edc/mobile/android/chrome/content/MemoryObserver.js?offset=0#21  [1]

This piece of code runs when a memory-pressure notification is received and then proceeds to zombify all tabs except the currently active one.
As I've written above, zombifying works by throwing away the old tab object and creating a new tab object in the "pending" state, the same which is also used for recreating the tabs when restoring a session, i.e. they contain only a minimal amount of data and start fully loading only after the user has selected them.
The problem is that throwing away the original tab object also destroys/invalidates (hopefully not leaks?) all event listeners that are attached to it.
If I remember correctly, browser.js recreates its event listeners within the tab object constructor, and therefore isn't affected by zombification, but the session store relies on the "domwindowopened" (during start up) and "TabOpen" (the rest of the time) events to attach it's event listeners to the tab object.
The "TabOpen" event however is only triggered when the full tabAdd is called (https://dxr.mozilla.org/mozilla-central/rev/cdf53a4dee219aabec7462888875bac08b093edc/mobile/android/chrome/content/browser.js#1197), so after the a zombifcation, the session store has effectively lost its event listeners on that tab object.

[1] There's also the tab expiration code here: https://dxr.mozilla.org/mozilla-central/rev/cdf53a4dee219aabec7462888875bac08b093edc/mobile/android/chrome/content/browser.js#8031
but during active browsing it is less relevant, and in the end it triggers the same zombification process anyway.

Comment 28

[Jan Henning [:JanH]]

Just to record this for posterity, this is how you can reproduce the issue on a device with ample memory:

> 10:24	JanH	If you run |adb logcat -s "GeckoMemoryMonitor"| in one shell
> 10:25	JanH	and |adb shell am broadcast -a org.mozilla.gecko.FORCE_MEMORY_PRESSURE| in another one
> 10:25	JanH	this will increase Firefox memory pressure to 4 (= HIGH), which will trigger zombification of all tabs except for the currently open one
> 10:26	JanH	you then have to wait a bit for memory pressure to decay again to 3 or lower before sending that broadcast again has any effect
> 10:40	JanH	sebastian: For testing you might want to adjust this value here, https://dxr.mozilla.org/mozilla-c...oid/base/MemoryMonitor.java#210 so you don't have to wait so long for memory pressure to decay again

Then it's basically
1. Watch the logcat in one shell as detailed above and launch a fresh Firefox session with one tab (tab A)
2. Do some browsing in that tab. When you stop, remember the current page.
3. Open a new tab (tab B) and switch to it.
4. Trigger a zombification by running the "adb shell am broadcast ..." command from another shell. Your current memory pressure level needs to be <= 3 for this to take any effect, but if you device has enough memory, this shouldn't be a problem. After this zombification, tab A's session history has become effectively frozen (see comment 20 and comment 27).
5. Switch back to tab A, observe it reloading the page from the end of step 2 and then continue browsing a bit.
6. Switch to tab B, wait for the memory pressure to decay to 3 or lower, then trigger another zombification.
7. Switch to tab A, observe it reloading and notice that it reloads the page from the end of step *2*, not step 5 as might be expected.

Comment 29

[Jan Henning [:JanH]]

Just noticed that the link from the IRC transcript didn't copy properly. Here's a working one:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/mobile/android/base/MemoryMonitor.java?offset=100#210

Comment 31

[donrhummy]

Is there any progress on this bug? It's a critical bug.

Comment 32

[Jan Henning [:JanH]]

Attachment: Bug 1044556 - Notify the session store about tab zombifications.patch

I still haven't got a working Fennec build environment set up (so sorry for slightly misusing the try server), but I think I might have come up with a working patch.

I've tested it locally along the lines of comment 28 on my device and it seems to be working - despite multiple forced zombifications, session history continues getting recorded for each tab.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8732208 [details] [diff] [review]
Bug 1044556 - Notify the session store about tab zombifications.patch

I'm still not super familiar with that area. Maybe margaret can give a quick feedback.

Comment 34

[matcheydj]

tl;dr - when is the tab state saved

What's doing the checkpointing? browser.js has checks for verification on load/sleep of the app, but if this is for android firefox it should be verified whether the checkpointing of the page/tab status is done through/by the kernel and when checkpointing for a tab is being done.

Comment 36

[Jan Henning [:JanH]]

Sebastian, I've finally got a VM with a working build environment going - can I officially take over?

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jan Henning [:JanH] from comment #36)
> Sebastian, I've finally got a VM with a working build environment going -
> can I officially take over?

Yes! I've been playing with Web IDE debugging over the weekend and have been poking this part of the code. After some more testing/understanding I should be able to give you feedback and/or review the code. :)

Comment 38

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1044556 - Part 1 - Notify the session store about tab zombifications. r=sebastian

The session store relies on a few event listeners to track the history and life cycle of a tab. Under memory pressure, background tabs are zombified in order to reduce our memory usage. This involves destroying the original tab object and then recreating it as a delay loaded tab.

As the session store is never told about this, it will keep the event listeners for the old tab objects - which have now been destroyed - alive and won't receive any future events for the new tab objects. This means that once a zombification has been triggered, the session history for those tabs will become effectively frozen, so after the next zombification or a session restore, the tab will reload the wrong page.

Therefore this patch introduces two new events which are sent during the tab zombification process and allow the session store to detach its event listeners from the old tab object before it is going to be destroyed and subsequently reattach its listeners to the new tab object.

Review commit: https://reviewboard.mozilla.org/r/43457/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43457/

Comment 39

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1044556 - Part 2 - Move promiseTabEvent to head.js. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/43559/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43559/

Comment 40

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1044556 - Part 3 - Add a test for verifying that the session store successfully continues recording tab history even after zombification. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/43459/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43459/

Comment 41

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1044556 - Part 4 - Unify audio playback indication handling. r=sebastian

Since we're now triggering zombification events, we can easily handle resetting the audio playback indicator upon zombifciation by having the tab object listen for the appropriate TabPreZombify event and handle it alongside the normal DOMAudioPlayback events instead of duplicating that functionality within the memory observer.

Review commit: https://reviewboard.mozilla.org/r/43461/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43461/

Comment 42

[donrhummy]

(In reply to Jan Henning [:JanH] from comment #41)
> Created attachment 8736814 [details]
> MozReview Request: Bug 1044556 - Part 4 - Unify audio playback indication
> handling. r=sebastian
> 
> Since we're now triggering zombification events, we can easily handle
> resetting the audio playback indicator upon zombifciation by having the tab
> object listen for the appropriate TabPreZombify event and handle it
> alongside the normal DOMAudioPlayback events instead of duplicating that
> functionality within the memory observer.
> 
> Review commit: https://reviewboard.mozilla.org/r/43461/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/43461/

Will these changes be testable in Firefox Beta on the Play store?

Comment 43

[Jan Henning [:JanH]]

They will, but they have to land first :-) It's probably too late to get them into the current 46 beta, but an uplift to 47 might be possible.

Comment 44

[Jan Henning [:JanH]]

Comment on attachment 8736813 [details]
MozReview Request: Bug 1044556 - Part 3 - Add a test for verifying that the session store successfully continues recording tab history even after zombification. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43459/diff/1-2/

Comment 45

[Jan Henning [:JanH]]

Comment on attachment 8736814 [details]
MozReview Request: Bug 1044556 - Part 4 - Unify audio playback indication handling. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43461/diff/1-2/

Comment 46

[Jan Henning [:JanH]]

I've noticed we already had one session store test (dealing with form data), so I've renamed my new test file to follow the same name pattern.

Comment 47

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736811 [details]
MozReview Request: Bug 1044556 - Part 1 - Notify the session store about tab zombifications. r=sebastian

https://reviewboard.mozilla.org/r/43457/#review40767

Comment 48

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736812 [details]
MozReview Request: Bug 1044556 - Part 2 - Move promiseTabEvent to head.js. r=sebastian

https://reviewboard.mozilla.org/r/43559/#review40769

Comment 49

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736813 [details]
MozReview Request: Bug 1044556 - Part 3 - Add a test for verifying that the session store successfully continues recording tab history even after zombification. r=sebastian

https://reviewboard.mozilla.org/r/43459/#review40771

Great test!

Comment 50

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736814 [details]
MozReview Request: Bug 1044556 - Part 4 - Unify audio playback indication handling. r=sebastian

https://reviewboard.mozilla.org/r/43461/#review40773

Comment 51

[Sebastian Kaspari (:sebastian; :pocmo)]

Great patches, Jan. I played this through with the steps from comment 28 and observed the bug and how it's fixed with your patches. :)

Comment 52

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d77d2428844

Comment 53

[Jan Henning [:JanH]]

Comment on attachment 8736811 [details]
MozReview Request: Bug 1044556 - Part 1 - Notify the session store about tab zombifications. r=sebastian

Note: The uplift request is only for part 1 - 3. Part 4 depends on 1260113 and can ride the trains.
Approval Request Comment
[Feature/regressing bug #]: Session store on Android
[User impact if declined]: Under memory-pressure, the session-store stops keeping track of a tab's history, so if a tab needs to be restored (when reloading Firefox from disk or when reloading a closed or zombified tab) Firefox will often restore the wrong URL.
[Describe test coverage new/current, TreeHerder]: New Mochitest covering this case + manual testing
[Risks and why]: Very low. Only very small, localised changes have been necessary to inform the session store when a tab has been zombified, so it can switch its event listeners to the new tab object created after zombification. 
[String/UUID change made/needed]: None.

Comment 54

[Jan Henning [:JanH]]

Comment on attachment 8736812 [details]
MozReview Request: Bug 1044556 - Part 2 - Move promiseTabEvent to head.js. r=sebastian

see above

Comment 55

[Jan Henning [:JanH]]

Comment on attachment 8736813 [details]
MozReview Request: Bug 1044556 - Part 3 - Add a test for verifying that the session store successfully continues recording tab history even after zombification. r=sebastian

see above

Comment 56

[Jan Henning [:JanH]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Long standing user-visible bug with relatively many duplicates.
[Suggested wording]: No longer loose track of tab history under memory pressure
[Links (documentation, blog post, etc)]:

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a305a109fa76
https://hg.mozilla.org/integration/fx-team/rev/fe87ac9dc1b4
https://hg.mozilla.org/integration/fx-team/rev/ca275706ed42
https://hg.mozilla.org/integration/fx-team/rev/3a3e79f7724a

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a305a109fa76
https://hg.mozilla.org/mozilla-central/rev/fe87ac9dc1b4
https://hg.mozilla.org/mozilla-central/rev/ca275706ed42
https://hg.mozilla.org/mozilla-central/rev/3a3e79f7724a

Comment 59

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8736811 [details]
MozReview Request: Bug 1044556 - Part 1 - Notify the session store about tab zombifications. r=sebastian

Fix for session store for android, adds new tests, OK to uplift for aurora and beta 10.

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to tweak part 1 to get it to apply to aurora. Hopefully the tweak is sufficient.

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d3700b39df4
https://hg.mozilla.org/releases/mozilla-aurora/rev/9dbb445db299
https://hg.mozilla.org/releases/mozilla-aurora/rev/e9eadb37c23e

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2869bb3483b1
https://hg.mozilla.org/releases/mozilla-beta/rev/2de7cd4da877
https://hg.mozilla.org/releases/mozilla-beta/rev/cef84689923a

Comment 63

[Jan Henning [:JanH]]

(In reply to Wes Kocher (:KWierso) from comment #60)
> I had to tweak part 1 to get it to apply to aurora. Hopefully the tweak is
> sufficient.

LGTM. This probably must have been the audio playback indicator reset code in MemoryObserver.js from bug 1260113, which is only on Nightly.

Comment 64

[Liz Henry (:lizzard) (relman/hg->git project)]

Noted as FIXED: Keep track of track of tab history under memory pressure