1161928 - Remove TabState.flush() call from SessionStore.restoreTab()

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

In SessionStore.restoreTab() we use a TabState.flush() call to ensure we properly ignore stale messages. We should be able to use browser epochs here to achieve the same.

I started looking at this because I hoped it would have a big impact on e10s startup performance but the sync handler is actually only called for tabs that are ready and have a docShell. At startup those are only the selected tab in each window and removing the flush() call didn't make any difference with even a big session.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1161928-Move-epoch-handling-from-ContentRestore..patch

This patch mostly moves keeping track of the current epoch from ContentRestore.jsm to content-sessionStore.js as we want to use not only when restoring state into a tab. Unless the previous implementation it doesn't just forget the epoch after restoring. A fresh tab starts with epoch=0 and might be told by the parent that a new one has started.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1161928-Add-assertions-to-ensure-tab-restoration.patch

This is prep work for patch #3 and introduces a few assertions. I used that to ensure the existing functionality is only used as intentioned. It doesn't hurt to leave them in, I would ideally like to introduce a few more fatal assertions in places where it makes sense to confirm a few invariants.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1161928-Add-assertions-to-ensure-tab-restoration.patch, v2

Small addition.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1161928-Require-an-epoch-managed-in-the-parent-i.patch

This test uses epochs to split a tab's life into sections and enable us to ignore updates from previous epochs whenever we throw away a tab's state.

It adds NOEPOCH_MESSAGES, a set that contains all messages that are okay to be sent without specifying an epoch as they're not sending data or changing state.

_browserEpochs is now a map that holds the current epoch for all browsers, if it doesn't have a key yet then we'll default to zero - the initial epoch of any tab.

receiveMessage() checks the tab's epoch now at the top when receiving a message. If the message is not in NOEPOCH_MESSAGES and has an epoch other than the current one we will discard it.

It removes the TabState.flush() call from SessionStore.restoreTab() and replaces that with starting a new epoch for the given tab.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> This test uses epochs to split a tab's life into sections and enable us to

Meant to write "this patch".

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1161928-Require-an-epoch... (without white space changes)

Same patch as 0003, but without whitespace changes.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8602073 [details] [diff] [review]
0001-Bug-1161928-Move-epoch-handling-from-ContentRestore..patch

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

Can you fix the comment at the top of the file that refers to epoch?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8602087 [details] [diff] [review]
0002-Bug-1161928-Add-assertions-to-ensure-tab-restoration.patch, v2

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

I love the use of assertions! Do you know for sure that NS_ASSERT will cause tests to turn orange?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #7)
> Can you fix the comment at the top of the file that refers to epoch?

Will do.

(In reply to Bill McCloskey (:billm) from comment #8)
> I love the use of assertions! Do you know for sure that NS_ASSERT will cause
> tests to turn orange?

Yes! Tried with NS_ASSERT(false) and that indeed makes the test fail.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e2b6db739d7

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e9e3afc65b9a
https://hg.mozilla.org/integration/fx-team/rev/360d53ac1556
https://hg.mozilla.org/integration/fx-team/rev/133c21f8ab74

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/e9e3afc65b9a
https://hg.mozilla.org/mozilla-central/rev/360d53ac1556
https://hg.mozilla.org/mozilla-central/rev/133c21f8ab74