944694 - [Session Restore] Storing docshellID makes no sense

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

At the moment, whenever we store history entries, we add a docshellID field. I believe that this doesn't make sense, as the docshellID is an in-memory key referring to the (globally indexed, valid per session) docshell used on the system. When we restore sessionstore.js, we set the docshellID field of nsISHEntry to this now dangling value. For all we know, this may cause bugs.

Now, the only case in which I believe docshellID makes sense is if we are restoring without leaving Firefox, which is a rather unusual case. I suspect that we should get rid of this field.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Other option is to store also the gDocshellIDCounter and when starting FF next time, 
restore that, so that internal docshell ids don't overlap the ones sessionstore has.

Comment 2

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

Except if we restore the session at some point after startup, gDocshellIDCounter will have lived its life already and we can still end up with overlap between docshell ids, no?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

But the stuff in sessionstore would have always lower id values than the initial gDocshellIDCounter.
(and gDocshellIDCounter is increased whenever a new docshell is created.)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Or can we restore the sessionstore collected during current FF run?
I guess we can when restoring closed tabs.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

But even then it should be ok, since the closed tabs had all those docshell IDs which we're about to restore.

Comment 6

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

So, if I create a nsISHEntry and assign a docshellID for a docshell that doesn't exist yet, what's going to happen? Also, is there a quick way to find out which docshellIDs already exist?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 16th-23th) from comment #6)
> So, if I create a nsISHEntry and assign a docshellID for a docshell that
> doesn't exist yet, what's going to happen?
Session history may start to work unexpectedly at some point
http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp?mark=1734-1734#1692

> Also, is there a quick way to
> find out which docshellIDs already exist?
In the current setup anything < gDocshellIDCounter may exist.

Comment 8

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

For the moment, I can't see how we can expect to load a docshellID from sessionstore.js.
At the moment, we perform:
 http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionHistory.jsm?from=SessionHistory.jsm#357-358

But this doesn't have any sense.
We cannot assign docshellIDs by ourselves, because they will be created in some unpredictable order with their docshells, if I understand correctly.

We also cannot leave them blank, because all history entries would be associated with the same docshell.

Any suggestion?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=944694#c1 is not clear enough?

Comment 10

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

(In reply to Olli Pettay [:smaug] from comment #9)
> https://bugzilla.mozilla.org/show_bug.cgi?id=944694#c1 is not clear enough?

Not really.

I have a bunch of nodes with a bogus docshellID. If I further export gDcoshellIDCounter to JS, I can use these docshellIDs + gDocshellIDCounter to generate a bunch of non-colliding docshellIDs. However, 
these docshellIDs are dangling, so according to comment 7, this will cause history bugs.

Am I missing something?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

But if session restore uses only IDs which aren't being used for current docshells, what is the issue?
We'd store gDocshellIDCounter somewhere and restore it during startup. session restore could then
use IDs 0 - gDocshellIDCounter, since the new ones would be (gDocshellIDCounter + 1) ->

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(I'm possibly missing something here. I'm not too familiar with session restore.)

Comment 13

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

(In reply to Olli Pettay [:smaug] from comment #11)
> But if session restore uses only IDs which aren't being used for current
> docshells, what is the issue?

I understood comment 7 to mean that there would be an issue. Did I misunderstand?

> We'd store gDocshellIDCounter somewhere and restore it during startup.
> session restore could then
> use IDs 0 - gDocshellIDCounter, since the new ones would be
> (gDocshellIDCounter + 1) ->

Well, that wouldn't work since we can call session restore after opening tabs and documents, etc. On the other hand, we could restore as 

function getID(storedID /* ID stored in sessionstore.js */) {
  let result = memo.get(storedID);
  if (result) {
    return result;
  }
  result = ++gDocshellIDCounter;
  memo.set(storedID, result);
  return result;
}

But will these arbitrary docshellIDs that don't match any real docshell work?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

So from what I understand, we think that saving and restoring the .docshellID property for nsISHEntry may cause collisions due to gDocshellIDCounter being reset at every startup. This may be even worse due to the docShell adapting its history entry's docShell ID when loading a history entry.

What if we just don't store this value at all and use the current docShell's ID when creating history entries for it - that will never collide. We would just pass docShell.ID to SessionHistory.deserializeEntry() and that's it, no?

Comment 15

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

Tim: I don't think that will work if we have frames with history. What's the "current docShell" in such a context?

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

I'm not sure I understand what you mean by "frames with history". No matter how many frames and subframes a page has, its history entries should all belong to the same docShell, no? When navigating an iframe to a different URL we create a new top-level shentry that is a duplicate of the current hierarchy except the one entry that just changed.

Comment 17

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

Attachment: Sample of frame history

This is a subset of what test browser_sessionHistory.js produces. Here, windows[0].tabs[0].entries[0].docshellID == 15, while windows[0].tabs[0].entries[0].children[0].docshellID == 16. So we need the docshell for both the parent and the child. Do we have this?

Comment 18

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

Actually, it might not be as bad as I thought. Would nsIDocShell::getDocShellEnumerator() return the children in the same order as children[]?

Comment 19

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

I'll give it a try.

Comment 20

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

As I suspected, we seem to be restoring the history before we have loaded all the iframes, so a naive implementation of the approach suggested in comment 14 will not work. Will continue tomorrow.

Comment 21

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

Is there a simple way to make sure that gContentRestore.restoreHistory is only called after all iframes have been loaded?

Comment 22

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

Attachment: Experimenting with restoring without docshellID

I haven't been able to get much progress.
The attached patch fails with test browser_463206, apparently because the sub-docshells are not created.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8432551 [details] [diff] [review]
Experimenting with restoring without docshellID

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

I don't quite understand the docShell ID code and don't know how that relates to child entries and what we should do about it.

This code has been in Firefox for multiple years and doesn't seem to cause any serious trouble. We're currently just assuming that this could cause problems, right?

Can we shift the focus for now to try to come up with a test case that demonstrates this is really an issue? It seems like we spent quite some time here already without having proof that this should be prioritized or worked on.

Comment 24

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

Well, we have an internal data structure full of dangling references, which can't be good, and has good chances of leading to hard-to-trace bugs. Also, I believe that this contributed to bug 942601 and certainly other cases of sessionstore.js saturation.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Then let's rather invest time in confirming that assumption first.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

I don't really know what feedback is needed here.
https://bugzilla.mozilla.org/show_bug.cgi?id=944694#c11 is what I think should happen.

Comment 28

[Nika Layzell [:nika] (ni? for response)]

This is the way which I am approaching this right now - it isn't exactly what olli was talking about in comment 11, but it might be good enough.

Currently the only major use of the docshellID as far as I know is for cleaning up shistory entries for docshells which have been removed from their document via script. This means that docshellIDs don't need to be unique per process, but rather need to be unique per toplevel docshell. I don't believe that there are any requirements that these IDs are unique per process.

For consistency's sake we want to be sure that all toplevel history entries within the same toplevel docshell have the same docshellID, because doing otherwise, while it is unlikely to break anything, is a bit odd.

We also want to be confident that any new docshells which we create within a given toplevel docshell don't have ID conflicts with any of the documents within the current docshell.

I think we can accomplish all of those things through 2 small changes:
* When setting the docshellID of a nsSHEntry, set gDocshellIDCounter = max(gDocshellIDCounter, docshellID) - this means that no new docshell will ever be created with the same docshellID as an existing nsSHEntry.
* When adding the first history entry to a docshell's nsSHistory, update that docshell's mHistoryID to match it.

Olli, do you have any thoughts on this? I am planning to quickly throw together a patch which handles this change.

Comment 29

[Nika Layzell [:nika] (ni? for response)]

Attachment: Modify docshell id handling in session history to better support Session Restore

MozReview-Commit-ID: Idmz5qEoqu8

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

It is unclear to me what the scope of this bug is. ClampHistoryIDMonotonic doesn't seem to fix the possible issue of using docshellIDs from different partial histories. They may share the same docshellIDs for some SHEntries of iframes, and then when removing duplicates we end up removing too much, right?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8810958 [details] [diff] [review]
Modify docshell id handling in session history to better support Session Restore

But I guess this works for single process case.

"If we get e10s-multi in the future" 
AFAIK we're about to enable the pref to support multiple child processes (depending on some talos regressions), so we shouldn't land this kind of tests anymore.

Other than that this looks ok for the single-process in a tab case, but I'm not sure about the prerendering or large-allocation case.

Comment 32

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Use globally unique UUIDs for docshellID

The new strategy which I am using here is to change docshellID to be a nsID instead of a uint64_t, which means that we can generate it anywhere and assume that it is globally unique. This avoids the problems with docshell ID conflicts after restores.

Comment 33

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update SessionStore to use the new docshellID format

These changes required changes to the way we use docshellID inside of sessionstore.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8811501 [details] [diff] [review]
Part 1: Use globally unique UUIDs for docshellID

>+nsDocShell::HistoryID() {
{ goes to its own line
>@@ -12308,17 +12311,17 @@ nsDocShell::AddToSessionHistory(nsIURI* aURI, nsIChannel* aChannel,
>   entry->Create(aURI,                // uri
>                 EmptyString(),       // Title
>                 inputStream,         // Post data stream
>                 nullptr,             // LayoutHistory state
>                 cacheKey,            // CacheKey
>                 mContentTypeHint,    // Content-type
>                 triggeringPrincipal, // Channel or provided principal
>                 principalToInherit,
>-                mHistoryID,
>+                &mHistoryID,
I think passing const nsID& would be better, and then you wouldn't need to make this change

>   /**
>    * The ID of the docshell in the session history.
>    */
>-  readonly attribute unsigned long long historyID;
>+  readonly attribute nsIDPtr historyID;
>+
>+  /**
>+   * Helper method for accessing this value from C++
>+   */
>+  [noscript, notxpcom] nsID HistoryID();
It is unclear how this ends up returning a const value in C++.
Could you verify the generated .h actually returns const.
(though, if this compiles, I assume so)

>@@ -188,17 +188,17 @@ interface nsISHEntry : nsISupports
> 
>     /** Additional ways to create an entry */
>     [noscript] void create(in nsIURI URI, in AString title,
>                            in nsIInputStream inputStream,
>                            in nsILayoutHistoryState layoutHistoryState,
>                            in nsISupports cacheKey, in ACString contentType,
>                            in nsIPrincipal triggeringPrincipal,
>                            in nsIPrincipal principalToInherit,
>-                           in unsigned long long docshellID,
>+                           in nsIDPtr docshellID,
So I'd prefer if this was ref, not ptr

>+                  const nsID* aDocShellID,
>                   bool aDynamicCreation)
> {
>   mURI = aURI;
>   mTitle = aTitle;
>   mPostData = aInputStream;
> 
>   // Set the LoadType by default to loadHistory during creation
>   mLoadType = (uint32_t)nsIDocShellLoadInfo::loadHistory;
> 
>   mShared->mCacheKey = aCacheKey;
>   mShared->mContentType = aContentType;
>   mShared->mTriggeringPrincipal = aTriggeringPrincipal;
>   mShared->mPrincipalToInherit = aPrincipalToInherit;
>-  mShared->mDocShellID = aDocShellID;
>+  mShared->mDocShellID = *aDocShellID;
If aDocShellID was &, not *, you wouldn't need to make this change
>-nsSHEntry::SetDocshellID(uint64_t aID)
>+nsSHEntry::SetDocshellID(const nsID* aID)
Could also this perhaps take a ref and not ptr


Those fixed, r+

Comment 35

[Mike de Boer [:mikedeboer]]

Comment on attachment 8811502 [details] [diff] [review]
Part 2: Update SessionStore to use the new docshellID format

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

::: browser/components/sessionstore/SessionHistory.jsm
@@ +43,5 @@
>  var SessionHistoryInternal = {
>    /**
> +   * Mapping from legacy docshellIDs to docshellUUIDs
> +   */
> +  _docshellUUIDMap: {},

Please use `new Map(),`

@@ +356,5 @@
>  
> +    // If we have the legacy docshellID on our entry, upgrade it to a
> +    // docshellUUID by going through the mapping.
> +    if (entry.docshellID) {
> +      if (!(entry.docshellID in this._docshellUUIDMap)) {

...then this will become `if (!this._docshellUUIDMap.has(entry.docshellID)) {`

@@ +357,5 @@
> +    // If we have the legacy docshellID on our entry, upgrade it to a
> +    // docshellUUID by going through the mapping.
> +    if (entry.docshellID) {
> +      if (!(entry.docshellID in this._docshellUUIDMap)) {
> +        let gen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);

Can you move this to a `XPCOMUtils.defineLazyServiceGetter` almost at the top of this file?

@@ +358,5 @@
> +    // docshellUUID by going through the mapping.
> +    if (entry.docshellID) {
> +      if (!(entry.docshellID in this._docshellUUIDMap)) {
> +        let gen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +        this._docshellUUIDMap[entry.docshellID] = gen.generateUUID();

...and this `this._docshellUUIDMap.set(entry.docshellID, gen.generateUUID())`

@@ +365,5 @@
> +      delete entry.docshellID;
> +    }
> +
> +    if (entry.docshellUUID) {
> +      shEntry.docshellID = Components.ID(entry.docshellUUID);

According to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.ID it expects a string, so you need to store the `gen.generateUUID().toString()` in the map after all.

::: browser/components/sessionstore/test/browser_docshell_uuid_consistency.js
@@ +1,1 @@
> +// First test - open a tab and duplicate it, using session restore to restore the history into the new tab

nit: missing dot.

@@ +1,2 @@
> +// First test - open a tab and duplicate it, using session restore to restore the history into the new tab
> +add_task(function*() {

nit: `function* () {` - I also appreciate a function name, because it'll show up in the logs when chasing intermittents.

@@ +12,5 @@
> +    is(shEntry.docshellID.number, docshell.historyID.number);
> +  });
> +
> +  let tab2 = gBrowser.duplicateTab(tab);
> +  yield promiseTabRestored(tab2);

Please use `BrowserTestUtils.browserLoaded(tab2.linkedBrowser);`

@@ +23,5 @@
> +    is(shEntry.docshellID.number, docshell.historyID.number);
> +  });
> +
> +  gBrowser.removeTab(tab);
> +  gBrowser.removeTab(tab2);

Please use `yield BrowserTestUtils.removeTab(tab);`

@@ +26,5 @@
> +  gBrowser.removeTab(tab);
> +  gBrowser.removeTab(tab2);
> +});
> +
> +// Second test - open a tab and navigate across processes, which triggers sessionrestore to persist history

nit: missing dot.

@@ +30,5 @@
> +// Second test - open a tab and navigate across processes, which triggers sessionrestore to persist history
> +add_task(function*() {
> +  const TEST_URL = "data:text/html,foo";
> +  let tab = gBrowser.addTab(TEST_URL);
> +  yield promiseBrowserLoaded(tab.linkedBrowser);

Please use BrowserTestUtils here too.

@@ +39,5 @@
> +          .QueryInterface(Ci.nsIDocShell);
> +    let sh = docshell.sessionHistory;
> +    is(sh.count, 1);
> +    is(sh.getEntryAtIndex(0, false).docshellID.number, docshell.historyID.number);
> +    return docshell.historyID.number;

You can assert all  the things inside a content-task using `Assert.equal()`, instead of `is()`. You don't seem to do something with the return value?

@@ +42,5 @@
> +    is(sh.getEntryAtIndex(0, false).docshellID.number, docshell.historyID.number);
> +    return docshell.historyID.number;
> +  });
> +
> +  // Force the browser to navigate to the chrome process

nit: missing dot.

@@ +49,5 @@
> +    let webnav = content.window.QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIWebNavigation);
> +    webnav.loadURI(CHROME_URL, Ci.nsIWebNavigation.LOAD_FLAGS_NONE, null, null, null);
> +  });
> +  yield promiseBrowserLoaded(tab.linkedBrowser);

Please use BrowserTestUtils here too.

@@ +61,5 @@
> +  is(sh.count, 2);
> +  is(sh.getEntryAtIndex(0, false).docshellID.number, docShell.historyID.number);
> +  is(sh.getEntryAtIndex(1, false).docshellID.number, docShell.historyID.number);
> +
> +  gBrowser.removeTab(tab);

Same.

::: mobile/android/components/SessionStore.js
@@ +94,5 @@
>    // to bother reloading the newly selected tab if it is zombified.
>    // The Java UI will tell us which tab to watch out for.
>    _keepAsZombieTabId: -1,
>  
> +  // Mapping from legacy docshellIDs to docshellUUIDs

nit: missing dot.

@@ +1312,5 @@
> +      delete aEntry.docshellID;
> +    }
> +
> +    if (aEntry.docshellUUID) {
> +      shEntry.docshellID = Components.ID(aEntry.docshellUUID);

Same comments apply here as well.

Comment 36

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Use globally unique UUIDs for docshellID

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update SessionStore to use the new docshellID format

(In reply to Mike de Boer [:mikedeboer] from comment #35)
> According to
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/
> Language_Bindings/Components.ID it expects a string, so you need to store
> the `gen.generateUUID().toString()` in the map after all.

number returns a string (counter intuitively, I know), so I actually am handling the strings correctly here. :)

> You can assert all  the things inside a content-task using `Assert.equal()`,
> instead of `is()`. You don't seem to do something with the return value?

Are you asking for me to use Assert.equal() instead of is()? I didn't change that in this version of the patch because I like the consistency of using is() both inside and outside of the content task.

The return value is no longer used, so I removed it.

Comment 38

[Mike de Boer [:mikedeboer]]

Comment on attachment 8811844 [details] [diff] [review]
Part 2: Update SessionStore to use the new docshellID format

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

::: browser/components/sessionstore/SessionHistory.jsm
@@ +362,5 @@
> +      if (!this._docshellUUIDMap.has(entry.docshellID)) {
> +        // Get the `.number` property out of the nsID such that the docshellUUID
> +        // property is correctly stored as a string.
> +        this._docshellUUIDMap.set(entry.docshellID,
> +                                  uuidGenerator.generateUUID().number);

Well, other parts of the codebase - the parts that I know :) - use '.toString()' instead of '.number'. It does the same thing, apparently.

::: browser/components/sessionstore/test/browser_docshell_uuid_consistency.js
@@ +17,5 @@
> +
> +  yield ContentTask.spawn(tab2.linkedBrowser, null, function() {
> +    let docshell = content.window.QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIWebNavigation)
> +          .QueryInterface(Ci.nsIDocShell);

nit: hmm, odd indentation... also below in the other content-tasks. Can you fix that before landing?

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/048cb0a9f401
Part 1: Use globally unique UUIDs for docshellID, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/66686afdff45
Part 2: Update SessionStore to use the new docshellID format, r=mikedeboer

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/048cb0a9f401
https://hg.mozilla.org/mozilla-central/rev/66686afdff45

Comment 41

[areya]

https://hg.mozilla.org/mozilla-central/rev/048cb0a9f401
https://hg.mozilla.org/mozilla-central/rev/66686afdff45