If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Move session history serialization to its own JSM

RESOLVED WONTFIX

Status

()

Firefox
Session Restore
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: ttaubert, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Move _serializeHistoryEntry() and _deserializeHistoryEntry() to SessionHistory.jsm. We should refactor the API itself as well.
(Reporter)

Comment 1

5 years ago
Created attachment 628353 [details] [diff] [review]
Part 1 - Create SessionHistory.jsm
Attachment #628353 - Flags: review?(paul)
(Reporter)

Comment 2

5 years ago
Created attachment 628354 [details] [diff] [review]
Part 2 - Let SessionStore use the new JSM
Attachment #628354 - Flags: review?(paul)
(Reporter)

Comment 3

5 years ago
Created attachment 628355 [details] [diff] [review]
Part 3 - Make SessionStorage.jsm use SessionHistory.jsm
Attachment #628355 - Flags: review?(paul)
(Reporter)

Comment 4

5 years ago
I removed the idMap and docIdMap arguments as they should be required to be identical only per-docShell. From https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsISHEntry

docShell.ID => "An ID to help identify this entry from others during subframe navigation."

docShell.docIdentifier => "An integer that should be the same for two entries attached to the same docshell if and only if the two entries are entries for the same document."

Those statements make me think that we don't need those maps to be per-window as they currently are. I couldn't find any further implementation details as that code exists since the CVS times.
(Reporter)

Comment 5

5 years ago
Looks good on try:

https://tbpl.mozilla.org/?tree=Try&rev=959c584b9768
(Reporter)

Comment 6

5 years ago
Created attachment 629580 [details] [diff] [review]
Part 3 - Make SessionStorage.jsm use SessionHistory.jsm

Re-based.
Attachment #628355 - Attachment is obsolete: true
Attachment #628355 - Flags: review?(paul)
Attachment #629580 - Flags: review?(paul)
Justin, you're more familiar with docshell ID/docIdentifier... Can you read through comment 4 and make sure that makes sense?
As I read this patch queue, the new code never calls nsISHEntry::adoptBFCacheEntry.  (The call is there in part 1, but then removed in part 2, afact.)  If so, that is surely incorrect.

As usual for internal APIs, the MDN documentation is out of date.  You'll have more success with the IDL.  nsISHEntry::docIdentifier doesn't exist anymore.  The "docIdentifier" you see in the current code lives only on the *serialized* shentries.  The serialized "docIdentifier" is a stand-in for the concept of two nsISHEntries sharing the same nsISHEntry::BFCacheEntry.

What the code is doing with the docIdMap is the following: If we have two nsISHEntries X and Y such that X.sharesDocumentWith(Y) (and, since it's reflexive, Y.sharesDocumentWith(X)), then when we serialize them, X and Y must have the same documentIdentifier.  Otherwise, X and Y must have different documentIdentifiers.

To your question about what's the relevant range of X and Y, that depends on how you write the de-serialization code.  X and Z with !X.sharesDocumentWith(Z) may share a serialized documentIdentifier if, during deserialization, you won't call X.adoptBFCacheEntry(Z) or Z.adoptBFCacheEntry(X).

Does that make any sense?
(Reporter)

Comment 9

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #8)
> As I read this patch queue, the new code never calls
> nsISHEntry::adoptBFCacheEntry.  (The call is there in part 1, but then
> removed in part 2, afact.)  If so, that is surely incorrect.

No, I didn't remove it. Part 1 creates the JSM that contains the adoptBFCacheEntry() call and Part 2 just removes the legacy code and makes everything use the new JSM. These patches don't change any behavior (except comment #4), they're just about moving code to a JSM.
> (The call is there in part 1, but then removed in part 2, afact.)

I see, we're moving code from one JSM to another, which is what confused me.

Is it possible for you to produce a diff of the changes you made?  Otherwise it's hard for me to say that this code is correct.
(Reporter)

Comment 11

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #8)
> As usual for internal APIs, the MDN documentation is out of date.  You'll
> have more success with the IDL.  nsISHEntry::docIdentifier doesn't exist
> anymore.  The "docIdentifier" you see in the current code lives only on the
> *serialized* shentries.  The serialized "docIdentifier" is a stand-in for
> the concept of two nsISHEntries sharing the same nsISHEntry::BFCacheEntry.

Ah... darn, I was misguided by the fact that the SHEntry *used to* have an attribute with the same name. Ok, so it's actually a custom attribute name we chose for serialization, gotcha.

(In reply to Justin Lebar [:jlebar] from comment #10)
> > (The call is there in part 1, but then removed in part 2, afact.)
> 
> I see, we're moving code from one JSM to another, which is what confused me.
> 
> Is it possible for you to produce a diff of the changes you made?  Otherwise
> it's hard for me to say that this code is correct.

I'll try to describe what I did. At the moment, when deserializing history entries, we pass a docIdentMap that is used for all tabs in the window that is restored.

This patch handles history (de)serialization only per docShell so we can't pass a window-scope docIdentMap. That essentially removes the possibility of SHEntries from different tabs sharing their documents.

Is this a use-case the we supported intentionally? Is it possible for SHEntries of different tabs/docShell to share their documents?
(Reporter)

Comment 12

5 years ago
The same goes for nsISHEntry.ID ("An ID to help identify this entry from others during subframe navigation."). We currently keep track of those to make sure we're not assigning duplicate IDs and change them before assignment if already taken.

The description sounds like this attribute's scope is per-docShell (subframe navigation). Do we even need to handle collisions? They shouldn't occur if we're assuming a valid sessionstore.js without modifications.
> Is this a use-case the we supported intentionally? Is it possible for SHEntries of different 
> tabs/docShell to share their documents?

No, so I think we're good, as you describe.

> The description [for nsISHEntry.ID] sounds this attribute's scope is per-docShell (subframe 
> navigation).

This I'm a bit less sure about.  But like you say, assuming a valid sessionstore.js file without modifications, we won't get any collisions anyway.  So that's fine.
Attachment #628353 - Flags: feedback+
(Reporter)

Comment 14

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #13)
> > The description [for nsISHEntry.ID] sounds this attribute's scope is per-docShell (subframe 
> > navigation).
> 
> This I'm a bit less sure about.  But like you say, assuming a valid
> sessionstore.js file without modifications, we won't get any collisions
> anyway.  So that's fine.

Even with a valid sessionstore.js we could get collisions if we for example duplicate a tab. That would be a collision only if IDs must be unique per-window or per-application. If SHEntry IDs are unique per-docShell only, then that's not a problem.

At least when created, SHEntry IDs are really unique:

http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHEntry.cpp#33

(That might have been easier to implement.)

Looking at

http://dxr.lanedo.com/mozilla-central/docshell/shistory/src/nsSHistory.cpp.html#l1238

I'd say that they're really only used per sessionHistory/docShell.
(Reporter)

Comment 15

5 years ago
(In reply to Paul O'Shannessy [:zpao] from comment #7)
> Justin, you're more familiar with docshell ID/docIdentifier... Can you read
> through comment 4 and make sure that makes sense?

I'd say we keep everything as is then. SHEntries share documents only if they're belonging to the same docShell, that's what the patch does.

SHEntry IDs are unique per docShell and it doesn't really hurt to keep the current collision checking here in case someone modifies sessionstore.js.
(Reporter)

Comment 16

5 years ago
Created attachment 662071 [details] [diff] [review]
Part 1 - Create SessionHistory.jsm
Attachment #628353 - Attachment is obsolete: true
Attachment #628353 - Flags: review?(paul)
Attachment #662071 - Flags: review?(felipc)
(Reporter)

Comment 17

5 years ago
Created attachment 662072 [details] [diff] [review]
Part 2 - Let SessionStore use the new JSM
Attachment #628354 - Attachment is obsolete: true
Attachment #628354 - Flags: review?(paul)
Attachment #662072 - Flags: review?(felipc)
(Reporter)

Comment 18

5 years ago
Created attachment 662073 [details] [diff] [review]
Part 3 - Make SessionStorage.jsm use SessionHistory.jsm
Attachment #629580 - Attachment is obsolete: true
Attachment #629580 - Flags: review?(paul)
Attachment #662073 - Flags: review?(felipc)
(Reporter)

Comment 19

5 years ago
Green on try:

https://tbpl.mozilla.org/?tree=Try&rev=ab6c939457ec
This bug is now blocking me. Marking as blocker.
Severity: normal → blocker
Blocks: 827852
Removing blocker status, I have found another angle for my current work that doesn't seem blocked by this bug.
Severity: blocker → normal
(Reporter)

Comment 22

5 years ago
Comment on attachment 662071 [details] [diff] [review]
Part 1 - Create SessionHistory.jsm

Let's cancel this for now. We first need to evaluate the basic direction we're going with sessionstore in the future.
Attachment #662071 - Flags: review?(felipc)
(Reporter)

Updated

5 years ago
Attachment #662072 - Flags: review?(felipc)
(Reporter)

Updated

5 years ago
Attachment #662073 - Flags: review?(felipc)
(Reporter)

Updated

5 years ago
Assignee: ttaubert → nobody
(Reporter)

Comment 23

4 years ago
The serialization part of this will land in bug 894595. The deserialization will follow when we're ready.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.