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

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

Trunk
Firefox 40
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → ttaubert
Iteration: --- → 40.3 - 11 May
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.
Attachment #8602073 - Flags: review?(wmccloskey)
Status: NEW → ASSIGNED
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.
Attachment #8602074 - Flags: review?(wmccloskey)
Small addition.
Attachment #8602074 - Attachment is obsolete: true
Attachment #8602074 - Flags: review?(wmccloskey)
Attachment #8602087 - Flags: review?(wmccloskey)
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.
Attachment #8602095 - Flags: review?(wmccloskey)
(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".
Same patch as 0003, but without whitespace changes.
Attachment #8602121 - Flags: feedback?(wmccloskey)
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?
Attachment #8602073 - Flags: review?(wmccloskey) → review+
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?
Attachment #8602087 - Flags: review?(wmccloskey) → review+
Attachment #8602095 - Flags: review?(wmccloskey) → review+
(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.
You need to log in before you can comment on or make changes to this bug.