759782 - Move session history serialization to its own JSM

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 1 - Create SessionHistory.jsm

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 2 - Let SessionStore use the new JSM

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - Make SessionStorage.jsm use SessionHistory.jsm

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Looks good on try:

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - Make SessionStorage.jsm use SessionHistory.jsm

Re-based.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Justin, you're more familiar with docshell ID/docIdentifier... Can you read through comment 4 and make sure that makes sense?

Comment 8

[Justin Lebar (not reading bugmail)]

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?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 10

[Justin Lebar (not reading bugmail)]

> (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.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 13

[Justin Lebar (not reading bugmail)]

> 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.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 1 - Create SessionHistory.jsm

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 2 - Let SessionStore use the new JSM

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - Make SessionStorage.jsm use SessionHistory.jsm

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Green on try:

https://tbpl.mozilla.org/?tree=Try&rev=ab6c939457ec

Comment 20

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

This bug is now blocking me. Marking as blocker.

Comment 21

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

Removing blocker status, I have found another angle for my current work that doesn't seem blocked by this bug.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

The serialization part of this will land in bug 894595. The deserialization will follow when we're ready.