1134518 - Incorrect back-forward navigation in tab with lots of history and e10s enabled

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

STR:

1. Navigate around in a tab to accumulate history (10 pages is sufficient)
2. Navigate to "about:preferences"
3. Click the back button

ER:
Clicking back returns to the previous page, whatever it was

AR:
Clicking back reloads "about:preferences". Clicking back one more time goes to the previous page.

Comment 1

[David Parks [:handyman]]

I am able to see this in a non-e10s window on my Macbook Pro.

Comment 2

[Dave Townsend [:mossop]]

When doing a load from history that switches process we send a message to the main process telling it which history index to load. We rely on session store collecting the history but it actually throws away the history older than a certain point so the index is now incorrect and we just load the most recent document.

One fix for this would be to pass something other than the index, instead maybe pass the history entry's ID, docshellID or docIdentifier. I'm not sure what the difference is between those but presumably one of them is unique enough that we can then scan the history sessionstore gives us to find the page to load.

But that assumes the user doesn't want to go too far back in history, at some point they can request an old page that isn't in the session stored history. Also there is the problem that navigating between remote and non-remote is throwing away history.

Tim, what would you think about changing session store so it always collects the entire session history but then only persists the subset to disk that max_serialize_back and max_serialize_forward define?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dave Townsend [:mossop] from comment #2)
> Tim, what would you think about changing session store so it always collects
> the entire session history but then only persists the subset to disk that
> max_serialize_back and max_serialize_forward define?

Yeah, that sounds like the most reasonable thing to do.

Currently we collect data in SessionHistory.jsm and ignore entries if we hit the limits. We can't iterate all entries on the main thread before saving, that's bad enough for cookies already...

The only option I see here is that we do this kind of cleanup on the SessionWorker. Either always when saving, or only on the final write before shutdown. It might make sense to send the state as an object (instead as a string) to the worker so we don't JSON.stringify() on the main thread only to JSON.parse() + .stringify() on the worker again. I don't know a lot about the perf impact here though.

Another advantage for such a cleanup job on the worker would be that we could maybe use that to fix bug 912717 as well and filter cookies off the main thread. SessionStore.getCurrentState() would then always report a little more data than is saved to disk but I do think that's better than the other way around.

David, any thoughts?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

> Tim, what would you think about changing session store so it always collects the entire session history but then only persists the subset to disk that max_serialize_back and max_serialize_forward define?

Can you walk me through this? I'm not sure how this would fix the issue.

Comment 5

[Dave Townsend [:mossop]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #4)
> > Tim, what would you think about changing session store so it always collects the entire session history but then only persists the subset to disk that max_serialize_back and max_serialize_forward define?
> 
> Can you walk me through this? I'm not sure how this would fix the issue.

We keep session history for all tabs in memory but it is only a subset of the history based on the prefs. So when we recreate a tab either because it has crashed or because (as here) we are switching from a non-remote to a remote tab we end up throwing away some of the user's history which isn't ideal.

I had assumed that we were only collecting a subset based on not wanting the data written to disk to be large but sounds like the perf impact of the collection itself might be a bigger deal?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dave Townsend [:mossop] from comment #5)
> I had assumed that we were only collecting a subset based on not wanting the
> data written to disk to be large but sounds like the perf impact of the
> collection itself might be a bigger deal?

Yeah, bug 943339 reduced the amount of data written to disk. The perf impact of the collection itself wasn't a huge issue iirc. If it is, we always have the ability to make history collection a tad more intelligent and only collect the stuff that changed. This might not be super easy to do but also not super hard.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Taking.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1134518-Remove-shistory-capping-code-from-Sessio.patch

This patch removes the current code that caps the number of shistory entries, introduced in bug 943339.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1134518-Send-a-state-object-instead-of-a-string-.patch

This patch makes us send a state object to the worker, instead of a state string.

David, I remember you saying that in the future sending strings might be faster than sending objects. Not sure if we're there yet but I assume that sending an object to the worker and stringifying it there after cleaning up might make up for the small overhead? We probably don't want to JSON.parse(), cleanup, JSON.stringify() on the worker even though it's OMT.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1134518-Cap-shistory-entries-in-the-SessionWorke.patch

This patch moves the shistory capping code to the SessionWorker. I would like to do the same for cookies to fix bug 912717. It cleans up only once on clean shutdown, we should think about moving other cleanup routines here as well.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1134518-Add-xpcshell-test-for-shistory-caps-on-c.patch

This removes the old test and adds a new xpcshell one. It also fixes test_backup_once.js to work with proper state objects instead of mock strings.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8596463 [details] [diff] [review]
0001-Bug-1134518-Remove-shistory-capping-code-from-Sessio.patch

Review of attachment 8596463 [details] [diff] [review]:
-----------------------------------------------------------------

Fair enough. Performance data showed that this had no impact on loading time.

r=me with the remark below addressed.

::: browser/components/sessionstore/SessionHistory.jsm
@@ +78,3 @@
>          entry.persist = txn.persist;
>          data.entries.push(entry);
> +      } while (txn = txn.next);

I seem to remember the convention that we should double-parenthesize in such cases to show that yes, we really intend `=` and not `==`.

However, a `for` would be much nicer (and would work even if `rootTransaction` is empty, I don't know if this can happen).

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8596469 [details] [diff] [review]
0002-Bug-1134518-Send-a-state-object-instead-of-a-string-.patch

Review of attachment 8596469 [details] [diff] [review]:
-----------------------------------------------------------------

Well, I guess that bug 984886 and my hopes of shaving 10-20ms from the critical path are dead and gone.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo" - away until May 6th) from comment #12)
> >          entry.persist = txn.persist;
> >          data.entries.push(entry);
> > +      } while (txn = txn.next);
> 
> I seem to remember the convention that we should double-parenthesize in such
> cases to show that yes, we really intend `=` and not `==`.
> 
> However, a `for` would be much nicer (and would work even if
> `rootTransaction` is empty, I don't know if this can happen).

Yeah, fair point. Will address that.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8596475 [details] [diff] [review]
0003-Bug-1134518-Cap-shistory-entries-in-the-SessionWorke.patch

Review of attachment 8596475 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/SessionWorker.js
@@ +97,5 @@
>      this.state = origin;
>      this.Paths = paths;
> +    this.maxUpgradeBackups = maxUpgradeBackups;
> +    this.maxSerializeBack = maxSerializeBack;
> +    this.maxSerializeForward = maxSerializeForward;

Can you throw a `TypeError` if any of these fields is unset, to avoid bad surprises in the future?

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8596477 [details] [diff] [review]
0004-Bug-1134518-Add-xpcshell-test-for-shistory-caps-on-c.patch

Review of attachment 8596477 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/test/unit/test_backup_once.js
@@ +78,5 @@
>  function promise_check_contents(path, expect) {
>    return Task.spawn(function*() {
>      do_print("Checking whether " + path + " has the right contents");
>      let actual = yield OS.File.read(path, { encoding: "utf-8"});
> +    Assert.deepEqual(JSON.parse(actual), expect);

, `File ${path} contains the expected data.`

::: browser/components/sessionstore/test/unit/test_shutdown_cleanup.js
@@ +1,1 @@
> +"use strict";

A few words to explain what this test is all about?

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6fe254a6e35a
https://hg.mozilla.org/integration/fx-team/rev/9484987dbf3e
https://hg.mozilla.org/integration/fx-team/rev/a0b3a91f0960
https://hg.mozilla.org/integration/fx-team/rev/8a151d59bdec

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Oh, hm. I forgot to add a test for the original issue. Will attach one shortly.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-1134518-Add-test-to-ensure-we-don-t-lose-shistor.patch

Had to fix ContentTask.spawn(), the method it used to determine whether to load the helper frame script was a little flawed. browser.permanentKey is the same for the remote and the non-remote frameLoader. After switching a browser's remoteness ContentTask.spawn() thus seized working.

Comment 20

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8596463 [details] [diff] [review]
0001-Bug-1134518-Remove-shistory-capping-code-from-Sessio.patch

This removes more code than I remember adding - but then I think things have changed a fair bit since then (e10s wasn't being pushed yet) :)

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #20)
> This removes more code than I remember adding - but then I think things have
> changed a fair bit since then (e10s wasn't being pushed yet) :)

Yeah indeed, this wasn't a straight backout. Your code lives on in SessionWorker.js now!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fe254a6e35a
https://hg.mozilla.org/mozilla-central/rev/9484987dbf3e
https://hg.mozilla.org/mozilla-central/rev/a0b3a91f0960
https://hg.mozilla.org/mozilla-central/rev/8a151d59bdec

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/11c4c1801cbe

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4d9970eb3c80

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Needed to push a bustage fix. When swapping a docShell to another window it seems we just load all of the pending frame scripts again, no matter whether one or all of those were loaded by that frameLoader before.

Comment 26

[Dave Townsend [:mossop]]

(In reply to Pulsebot from comment #24)
> https://hg.mozilla.org/integration/fx-team/rev/4d9970eb3c80

Nit: Services.mm!

(In reply to Tim Taubert [:ttaubert] from comment #25)
> Needed to push a bustage fix. When swapping a docShell to another window it
> seems we just load all of the pending frame scripts again, no matter whether
> one or all of those were loaded by that frameLoader before.

That sounds like a bug to me, filed bug 1158198

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dave Townsend [:mossop] from comment #26)
> (In reply to Pulsebot from comment #24)
> > https://hg.mozilla.org/integration/fx-team/rev/4d9970eb3c80
> 
> Nit: Services.mm!

Oh, right. Forgot that this was added a while ago.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11c4c1801cbe
https://hg.mozilla.org/mozilla-central/rev/4d9970eb3c80