Closed Bug 1134518 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
e10s m7+ ---
firefox40 --- fixed

People

(Reporter: cmanchester, Assigned: ttaubert)

References

Details

Attachments

(5 files)

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.
Flags: needinfo?(dtownsend)
I am able to see this in a non-e10s window on my Macbook Pro.
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?
Flags: needinfo?(ttaubert)
Flags: needinfo?(dtownsend)
Blocks: 943339
(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?
Flags: needinfo?(ttaubert) → needinfo?(dteller)
> 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.
Flags: needinfo?(dteller) → needinfo?(dtownsend)
(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?
Flags: needinfo?(dtownsend)
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
(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.
Taking.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Points: 3 → 5
OS: Mac OS X → All
Hardware: x86 → All
This patch removes the current code that caps the number of shistory entries, introduced in bug 943339.
Attachment #8596463 - Flags: review?(dteller)
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.
Attachment #8596469 - Flags: review?(dteller)
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.
Attachment #8596475 - Flags: review?(dteller)
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.
Attachment #8596477 - Flags: review?(dteller)
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).
Attachment #8596463 - Flags: review?(dteller) → review+
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.
Attachment #8596469 - Flags: review?(dteller) → review+
(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 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?
Attachment #8596475 - Flags: review?(dteller) → review+
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?
Attachment #8596477 - Flags: review?(dteller) → review+
Oh, hm. I forgot to add a test for the original issue. Will attach one shortly.
Keywords: leave-open
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.
Attachment #8596566 - Flags: review?(dtownsend)
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) :)
(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!
Attachment #8596566 - Flags: review?(dtownsend) → review+
Keywords: leave-open
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.
(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
(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.
https://hg.mozilla.org/mozilla-central/rev/11c4c1801cbe
https://hg.mozilla.org/mozilla-central/rev/4d9970eb3c80
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Duplicate of this bug: 1154552
You need to log in before you can comment on or make changes to this bug.