Update SessionStore for GroupedSHistory.

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: freesamael, Assigned: Nika)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(5 attachments, 6 obsolete attachments)

22.72 KB, patch
Details | Diff | Splinter Review
11.85 KB, patch
Details | Diff | Splinter Review
8.72 KB, patch
Details | Diff | Splinter Review
5.72 KB, patch
Details | Diff | Splinter Review
2.84 KB, patch
mconley
: review+
Details | Diff | Splinter Review
Not sure how this should be implemented yet, but SessionStore should be modified so that

1. When long pressing "back" button, it shows the complete history list in the GroupedSHistory.
2. When restoring tabs on browser crash / restarting, it should restore only one tab with complete history existed in the GroupedSHistory before.

Updated

2 years ago
Priority: -- → P3
(Assignee)

Comment 1

2 years ago
I'm helping Samael out with prerendering, so I am going to tentatively take over this bug.
Assignee: nobody → michael
(Assignee)

Comment 2

2 years ago
Created attachment 8809910 [details] [diff] [review]
Part 1: Add an aFlags argument to nsIBrowser::SwapBrowsers

NOTE: I'm still working out some test failures from these patches, but they have grown fairly large and unweildy, so I would like to start getting review feedback on them at the same time as I am figuring out the tail end of the test failures. 

This adds a single flag, SWAP_KEEP_PERMANENT_KEY, which tells the
browser that when it performs the swap, the permanent key should stick
with the browser, rather than following the frameLoader.

This patch also adds the implementation of tabbrowser.swapBrowsers,
which was previously absent, despite being referenced.

MozReview-Commit-ID: CLwJYzpY8Pp
Attachment #8809910 - Flags: review?(bugs)
(Assignee)

Comment 3

2 years ago
Created attachment 8809911 [details] [diff] [review]
Part 2: Emit a BrowserChangedProcess event whenever the browser changes process due to a grouped history navigation

MozReview-Commit-ID: 7uY0WQhVxDS
Attachment #8809911 - Flags: review?(bugs)
(Assignee)

Comment 4

2 years ago
Created attachment 8809912 [details] [diff] [review]
Part 3: Add support to SessionStore for recording history for GroupedSHistories

MozReview-Commit-ID: Ffq7h3zRUm3
Attachment #8809912 - Flags: review?(mdeboer)
(Assignee)

Comment 5

2 years ago
Created attachment 8809913 [details] [diff] [review]
Part 4: Add a test for SessionStore support for cross process navigations

MozReview-Commit-ID: 288dTbrSjOE
Attachment #8809913 - Flags: review?(mdeboer)

Comment 6

2 years ago
Comment on attachment 8809910 [details] [diff] [review]
Part 1: Add an aFlags argument to nsIBrowser::SwapBrowsers

This needs a review from someone else. I have no idea what permanentKey is.
Attachment #8809910 - Flags: review?(bugs)

Comment 7

2 years ago
Comment on attachment 8809911 [details] [diff] [review]
Part 2: Emit a BrowserChangedProcess event whenever the browser changes process due to a grouped history navigation

Tiny bit hard to review without any explanation what this actually is supposed to do
>+    mThis->mGroupedSessionHistory.swap(other->mGroupedSessionHistory);
>+
>+    // Dispatch the BrowserChangedProcess event to tell JS that the process swap
>+    // has occurred.
>+    GroupedHistoryEventInit eventInit;
>+    eventInit.mBubbles = true;
>+    eventInit.mCancelable = false;
>+    eventInit.mOtherBrowser = secondaryContent;
>+    RefPtr<GroupedHistoryEvent> event =
>+      GroupedHistoryEvent::Constructor(primaryContent,
>+                                       NS_LITERAL_STRING("BrowserChangedProcess"),
>+                                       eventInit);
>+    event->SetTrusted(true);
>+    primaryContent->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
Please use the normal DispatchEvent() method.


Why async dispatching is used one with BrowserChangedProcess but the other uses sync dispatching? Feels odd inconsistency.

Could you possibly refactor BrowserWillChangeProcess dispatching to some helper method. Now you have almost the same code twice.

>   /**
>    * Append partial session history from another frame loader.
>+   *
>+   * @return A promise which will be resolved when the navigation is complete.
>    */
>-  void appendPartialSessionHistoryAndSwap(in nsIFrameLoader aOther);
>+  nsISupports appendPartialSessionHistoryAndSwap(in nsIFrameLoader aOther);
What navigation? Method called appendPartialSessionHistoryAndSwap doesn't sound like anything doing a navigation.



> 
>   /**
>    * If grouped session history is applied, use this function to navigate to
>    * an entry of session history object of another frameloader.
>+   *
>+   * @return A promise which will be resolved when the navigation is complete.
>    */
>-  void requestGroupedHistoryNavigation(in unsigned long aGlobalIndex);
>+  nsISupports requestGroupedHistoryNavigation(in unsigned long aGlobalIndex);
What does "navigation complete" mean in this case?
Based on the code the promise is resolved when all the BrowserChangingProcessBlockers
have been resolved. Nothing to do with navigation. Or am I missing something here.

>+
>+  /**
>+   * Adds a blocking promise for the current cross process navigation.
>+   * This method can only be called while the "BrowserWillChangeProcess" event
>+   * is being fired.
>+   */
>+  [implicit_jscontext]
>+  void addProcessChangeBlockingPromise(in jsval aPromise);
What does blocking promise mean?


>+ * For more information on this interface, please see
>+ * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html
>+ */
This link sure is wrong
Attachment #8809911 - Flags: review?(bugs) → review-
(Assignee)

Updated

2 years ago
Attachment #8809910 - Flags: review?(dao+bmo)
(Assignee)

Comment 8

2 years ago
Created attachment 8810484 [details] [diff] [review]
Part 2: Emit BrowserWillChangeProcess and BrowserChangedProcess when doing cross-frameloader navigations

With GroupedSHistory, history navigations may now require the browser to
change which frameloader is stored internally from within Core. This
patch adds a mechanism to allow for chrome code to respond to these
changes and both delay the change, or respond once the change is
performed.

Delaying the change is accomplished through the BrowserWillChangeProcess
event, which is fired when it is determined that a process change will
happen for the given browser, but the change has not occured yet. During
this time the nsIFrameLoader::AddProcessChangeBlockingPromise method may
be called on the target browser's frameloader. Any promises passed to
this method will be waited on, and the process change will not occur
until they have all been fulfiled.

Once that has occured, the process change occurs, and the
BrowserChangedProcess event is fired.

This is useful for chrome code which needs to flush state from the
original process before the change, and then which needs to connect
state in the new process with state in the chrome process.

MozReview-Commit-ID: C0Xn6pfruB2

----

I hope that above description is useful for figuring out what exactly I'm trying to accomplish in this patch. I tried not to change too many names but I'm open to suggestions about what better names to use.

For example, AppendPartialSessionHistoryAndSwap does perform a frameloader swap, and is used as part of prerendering navigation, which is why I use the term "A promise which is resolved when the navigation is complete". I am open to coming up with a better term which makes the function of the method more clear.
Attachment #8810484 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8809911 - Attachment is obsolete: true

Comment 9

2 years ago
Comment on attachment 8810484 [details] [diff] [review]
Part 2: Emit BrowserWillChangeProcess and BrowserChangedProcess when doing cross-frameloader navigations


>+  // Dispatch the BrowserChangedProcess event to tell JS that the process swap
>+  // has occurred.
>+  GroupedHistoryEventInit eventInit;
>+  eventInit.mBubbles = true;
>+  eventInit.mCancelable= false;
>+  eventInit.mOtherBrowser = secondaryContent;
>+  RefPtr<GroupedHistoryEvent> event =
>+    GroupedHistoryEvent::Constructor(primaryContent,
>+                                     NS_LITERAL_STRING("BrowserChangedProcess"),
>+                                     eventInit);
>+  event->SetTrusted(true);
>+  bool _dummy;
Nit, dummy

>+  RefPtr<GroupedHistoryEvent> event =
>+    GroupedHistoryEvent::Constructor(mOwnerContent,
>+                                     NS_LITERAL_STRING("BrowserWillChangeProcess"),
>+                                     eventInit);
>+  event->SetTrusted(true);
>+  bool _dummy;
dummy



>+  // The ready promise will be resolved once all of the blockers have resolved
>+  // which were added during the BrowserWillChangeProcess event.
>+  nsresult FireWillChangeProcessEvent(mozilla::dom::Promise** aReady,
>+                                      mozilla::dom::Promise** aComplete);
As discussed on IRC, aComplete should be created by the caller. It has nothing to do with FireWillChangeProcessEvent


>+  // A stack-maintained reference to an array of promises which are blocking
>+  // grouped history navigation
>+  nsTArray<RefPtr<mozilla::dom::Promise>>* mBrowserChangingProcessBlockers;
So this points to a random memory after nsFrameLoader has been created. Please initialize explicitly to nullptr;
Attachment #8810484 - Flags: review?(bugs) → review+
Hi Michael! After cursory reading of the patch and reading the explanation you provided for Olli I must confess that I would like some more of that to chew on, please :)
I've read a bit of the background by following the meta bugs and such, but is there a wiki page that explains this project in a bit more detail?

Don't worry, the cursory glance upon the Sessionstore changes didn't yield any red flags, so I think it'll be fine. I'd just appreciate a bit more context, that's all. Thanks!
Flags: needinfo?(michael)
Comment on attachment 8809910 [details] [diff] [review]
Part 1: Add an aFlags argument to nsIBrowser::SwapBrowsers

I seem to recall that Mike has been dealing quite a bit with docShell swapping (well me too I suppose, but who hasn't at this point?) and introduced the permanentKey concept. I think you'll want him to check out this patch too.
Dão, do you agree?
Attachment #8809910 - Flags: review?(mconley)
(Assignee)

Comment 12

2 years ago
Of course. We are horribly disorganized, and don't have a wiki page which explains everything right now, but I'll throw something together to try to explain it to you (and perhaps it will evolve into a wiki page).

The end goal here is prerendering. We would like to be able to prerender documents, and then switch to the already prerendered document when the document is navigated to. As we need to keep the original document active, and we would like to be able to run the prerendering in a new process if possible, this prerendered document is rendered in a new docshell, potentially in a different process. When the link is clicked on to switch to the prerendered document, we will send an event to the parent process, which causes the frameloaders of the prerendered document and the currently active document to swap, and for their histories to be stitched together.

(NOTE: The patches to actually implement prerendering aren't in m-c yet, my wip ones can be found on bug 1315105)

As you can imagine, having multiple docshells which each doesn't have a complete session history isn't supported by Core right now, so in bug 1276553, samael added the concept of a GroupedSessionHistory which spans across multiple frameloaders. Whenever a history navigation occurs which extends out of the range which is supported by the currently active frameloader, a frameloader swap occurs to bring the frameloader which can handle the load to active status.

The above stuff is all very buggy and behind a pref, and there are a bunch of known issues with it which I am trying to work out. 

The goal with this patch in particular is to ensure that the sessionstore code can handle these grouped session histories. My plan for this was to handle that by having a single sessionstore session history for the lifetime of the tab, and to have each of the hunks of history in the content process only update the section of sessionhistory in the parent TabStateCache which is relevant to them. This was done by trying to use partial updates more often.

I also added some new events which are fired before and after the frameloader swap occurs. The one which occurs before is used by SessionStore to flush the currently active frameloader's TabState before the navigation occurs, just in case it isn't up to date, and the second one is used by sessionstore to determine a new epoch for the newly active process, which is at least 1 higher than the current epoch in the previous frameloader, and the current frameloader.

If you have any other questions or that isn't clear, ni? me or ping me on irc and I'll do my best to explain it better :).

Thanks!
(Assignee)

Comment 13

2 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Comment on attachment 8809910 [details] [diff] [review]
> Part 1: Add an aFlags argument to nsIBrowser::SwapBrowsers
> 
> I seem to recall that Mike has been dealing quite a bit with docShell
> swapping (well me too I suppose, but who hasn't at this point?) and
> introduced the permanentKey concept. I think you'll want him to check out
> this patch too.
> Dão, do you agree?

I work in the same office as mike and asked him for ideas of how I should handle the permanentKey situation. He seemed to think that this approach would work, though of course he hasn't seen the particular code which I have written here.

I didn't r? him on the patch originally because I knew he was on pto. I'm fine with this patch waiting for a bit if you think it's important that he takes a look at it.
Flags: needinfo?(michael)
Thanks much, Michael! This makes a lot more sense to me now and I'm excited about the end goal!
Comment on attachment 8809912 [details] [diff] [review]
Part 3: Add support to SessionStore for recording history for GroupedSHistories

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

Alright, I agree with the changes here - but I'm expecting test coverage for the 'BrowserWillChangeProcess' and 'BrowserProcessChanged' event handling in sessionstore and the 'SessionStore:becomeActiveProcess' message handling.
And all the current tests have to still pass, of course ;-) Or adjusted to be correct for the new situation when they fail currently.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +301,5 @@
>        if (this._fromIdx === kNoIndex) {
>          return null;
>        }
>  
> +      let history = SessionHistory.collect(docShell, this._fromIdx);

I'm actually quite happy that you moved this logic to `collect` instead.
Attachment #8809912 - Flags: review?(mdeboer) → review+
Comment on attachment 8809913 [details] [diff] [review]
Part 4: Add a test for SessionStore support for cross process navigations

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

Now that I'm familiar with the material, please expect faster turn-around times.

::: docshell/test/browser/browser_bug1310771.js
@@ +35,5 @@
> +      // XXX: We currently need to close this tab manually. Ideally in the
> +      // future it will be automatically closed when tab1 is closed.
> +      gBrowser.removeTab(tab2);
> +    }
> +  });

Like I mentioned in the review before this one; I'd like this to cover a few more scenarios to really see a grouping of tab history across processes.
In other words; navigate multiple times and switch processes multiple times. Does that make sense?
Attachment #8809913 - Flags: review?(mdeboer) → review+
Attachment #8809913 - Flags: review+ → review-
(Assignee)

Comment 17

2 years ago
Created attachment 8810960 [details] [diff] [review]
Part 2: Emit BrowserWillChangeProcess and BrowserChangedProcess when doing cross-frameloader navigations

With GroupedSHistory, history navigations may now require the browser to
change which frameloader is stored internally from within Core. This
patch adds a mechanism to allow for chrome code to respond to these
changes and both delay the change, or respond once the change is
performed.

Delaying the change is accomplished through the BrowserWillChangeProcess
event, which is fired when it is determined that a process change will
happen for the given browser, but the change has not occured yet. During
this time the nsIFrameLoader::AddProcessChangeBlockingPromise method may
be called on the target browser's frameloader. Any promises passed to
this method will be waited on, and the process change will not occur
until they have all been fulfiled.

Once that has occured, the process change occurs, and the
BrowserChangedProcess event is fired.

This is useful for chrome code which needs to flush state from the
original process before the change, and then which needs to connect
state in the new process with state in the chrome process.

MozReview-Commit-ID: C0Xn6pfruB2
(Assignee)

Updated

2 years ago
Attachment #8810484 - Attachment is obsolete: true
Comment on attachment 8809910 [details] [diff] [review]
Part 1: Add an aFlags argument to nsIBrowser::SwapBrowsers

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

Looks okay to me. Thanks!

::: dom/interfaces/base/nsIBrowser.idl
@@ +27,5 @@
>  
>    /**
> +   * Flags for controlling the behavior of swapBrowsers
> +   */
> +  /**

Nit: let's put some newlines between these blocks.
Attachment #8809910 - Flags: review?(mconley) → review+
(Assignee)

Comment 19

2 years ago
Created attachment 8811028 [details] [diff] [review]
Part 3: Add support to SessionStore for recording history for GroupedSHistories

MozReview-Commit-ID: Ffq7h3zRUm3
(Assignee)

Updated

2 years ago
Attachment #8809912 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8811029 [details] [diff] [review]
Part 4: Add a test for SessionStore support for cross process navigations

This has many more tests, and I caught a small off-by-one error because of it.
Attachment #8811029 - Flags: review?(mdeboer)
(Assignee)

Updated

2 years ago
Attachment #8809913 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
Created attachment 8811041 [details] [diff] [review]
Part 1: Add an aFlags argument to nsIBrowser::SwapBrowsers

This adds a single flag, SWAP_KEEP_PERMANENT_KEY, which tells the
browser that when it performs the swap, the permanent key should stick
with the browser, rather than following the frameLoader.

This patch also adds the implementation of tabbrowser.swapBrowsers,
which was previously absent, despite being referenced.

MozReview-Commit-ID: CLwJYzpY8Pp
(Assignee)

Updated

2 years ago
Attachment #8809910 - Attachment is obsolete: true
Attachment #8809910 - Flags: review?(dao+bmo)
Comment on attachment 8811029 [details] [diff] [review]
Part 4: Add a test for SessionStore support for cross process navigations

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

Two general comments:
1. Please change the name of the file to something a bit more descriptive.
2. You might want to consider moving this test to 'browser/'. Only location I can think of is 'browser/components/sessionstore/', even though it's crowded!

Thanks for making this change and I'm happy you caught an error with it!

r=me with comments addressed.

::: docshell/test/browser/browser_bug1310771.js
@@ +1,1 @@
> +add_task(function*() {

nit: `function* () {`

@@ +7,5 @@
> +    "data:text/html,5",
> +  ];
> +
> +  let TabStateCache = Cu.import("resource:///modules/sessionstore/TabStateCache.jsm", {}).TabStateCache;
> +  let TabStateFlusher = Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm", {}).TabStateFlusher;

nit: `const {TabStateCache} = Cu.import("...", {});`

@@ +13,5 @@
> +  yield SpecialPowers.pushPrefEnv({
> +    set: [["browser.groupedhistory.enabled", true]]
> +  });
> +
> +  // Check that the data stored in the TabStateCache is correct for the current state

nit: can you end your sentences inside comments with a dot throughout this file?

@@ +41,5 @@
> +  // Order of events:
> +  // Load [0], load [1], prerender [2], load [2], load [3]
> +  // Back [2], Back [1], Forward [2], Back [0], Forward [3]
> +  // Prerender [4], Back [0], Forward [2], Load [3'], Back [0]
> +  yield BrowserTestUtils.withNewTab({ gBrowser, url: URIs[0] }, function*(browser1) {

nit: `function* (browser1) {`

@@ +42,5 @@
> +  // Load [0], load [1], prerender [2], load [2], load [3]
> +  // Back [2], Back [1], Forward [2], Back [0], Forward [3]
> +  // Prerender [4], Back [0], Forward [2], Load [3'], Back [0]
> +  yield BrowserTestUtils.withNewTab({ gBrowser, url: URIs[0] }, function*(browser1) {
> +    yield* validate(browser1, 1, 1);

I don't think you need `yield*`, because I don't see a nested iterator in there.

@@ +67,5 @@
> +    yield* validate(browser1, 4, 4);
> +
> +    {
> +      // In process navigate back
> +      let p = BrowserTestUtils.waitForContentEvent(browser1, "pageshow");

nit: `let promise = ...`

@@ +70,5 @@
> +      // In process navigate back
> +      let p = BrowserTestUtils.waitForContentEvent(browser1, "pageshow");
> +      browser1.goBack();
> +      yield p;
> +    }

Why the separate block? I think you don't need it.
Attachment #8811029 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 23

2 years ago
Created attachment 8811503 [details] [diff] [review]
Part 4: Add a test for SessionStore support for cross process navigations

Updated.

I believe I do need the yield* as validate is a function*, and yields to TabStateFlusher.flush(). I decided to make validate a generator because it meant that the backtraces in the case of the is() statements within validate failing were much nicer.
(Assignee)

Updated

2 years ago
Attachment #8811029 - Attachment is obsolete: true
(Assignee)

Comment 24

2 years ago
Created attachment 8811882 [details] [diff] [review]
Part 5: Move the logic for unregistering the opened page from _swapRegisteredOpenURIs into swapBrowsersAndCloseOther

After rebasing on master I was having local test failures which required me to look into the unified complete engine, and I realized that we weren't handling it correctly (as we were assuming that after a browser swap one of the browsers would be closed, which was wrong).

This should work better for now, though we will need to file another bug at some point to ensure that tabs which are hidden are not listed as pages to open with the awesomebar.
Attachment #8811882 - Flags: review?(mconley)
(Assignee)

Comment 25

2 years ago
(In reply to Michael Layzell [:mystor] [:mrl] from comment #24)
> Created attachment 8811882 [details] [diff] [review]
> Part 5: Move the logic for unregistering the opened page from
> _swapRegisteredOpenURIs into swapBrowsersAndCloseOther

Please pretend like the attribute typo is not there - I noticed it immediately after posting this but didn't want to spam you with clearing the r?
Comment on attachment 8811882 [details] [diff] [review]
Part 5: Move the logic for unregistering the opened page from _swapRegisteredOpenURIs into swapBrowsersAndCloseOther

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

r=me assuming a green try run with that typo fixed.

::: browser/base/content/tabbrowser.xml
@@ +2865,5 @@
>  
> +            // Unregister the previously opened URI
> +            if (otherBrowser.registeredOpenURI) {
> +              this._unifiedComplete.unregisterOpenPage(otherBrowser.registeredOpenURI,
> +                                                      otherBrowser.getAttribute("usercontextid") || 0);

Nit - alignment

@@ +2988,5 @@
>            <![CDATA[
> +            // Swap the registeredOpenURI properties of the two browsers
> +            let tmp = aOurBrowser.registeredOpenURI;
> +            delete aOurBrowser.registeredOpenURI;
> +            if (aOtherBrowser.registredOpenURI) {

Whoopsy - typo: "registredOpenURI" -> "registeredOpenURI".
Attachment #8811882 - Flags: review?(mconley) → review+

Comment 27

2 years ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ec9cb5fc23
Part 1: Add an aFlags argument to nsIBrowser::SwapBrowsers, r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/04f861d4b2fe
Part 2: Emit BrowserWillChangeProcess and BrowserChangedProcess when doing cross-frameloader navigations, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/03a293583473
Part 3: Add support to SessionStore for recording history for GroupedSHistories, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa643642a67
Part 4: Add a test for SessionStore support for cross process navigations, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/84e635f18d70
Part 5: Move the logic for unregistering the opened page from _swapRegisteredOpenURIs into swapBrowsersAndCloseOther, r=mconley
Depends on: 1319596

Updated

2 years ago
Depends on: 1340872

Updated

2 years ago
Depends on: 1345857

Updated

2 years ago
Depends on: 1350567
You need to log in before you can comment on or make changes to this bug.