New Session History #20507

cbrewster · 2018-04-03T01:34:09Z

Remaining Work:

Move LoadData from BrowsingContext to Pipeline
Cleanup *Diff and *Changeset types
Implement pipeline discarding and reloading
Document new code

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-04-03T01:34:13Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/session_history.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs, components/constellation/browsingcontext.rs, components/constellation/lib.rs
@paulrouget: ports/servo/browser.rs, components/constellation/session_history.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs, components/constellation/browsingcontext.rs and 1 more

cbrewster · 2018-04-03T01:34:18Z

@bors-servo try

bors-servo · 2018-04-03T01:34:23Z

⌛ Trying commit 90635d3 with merge af1e7ea...

[WIP] New Session History  ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____

bors-servo · 2018-04-03T01:54:37Z

💔 Test failed - linux-dev

gterzian · 2018-04-03T13:48:05Z

Hi @cbrewster

I have a question:

In the context of #20329, (before)unload, I have to find out from within History if the document of the entry to be traversed to, is or isn't the 'active' one of the browsing context of the entry. (see https://html.spec.whatwg.org/multipage/history.html#traverse-the-history-by-a-delta Step 5.2)

I'm currently doing this there: https://github.com/servo/servo/pull/20329/files#diff-55c92a6a5ba7654ce45fe6fc6c63740fR2041, which happens by way of a message sent from https://github.com/servo/servo/pull/20329/files#diff-7ea9bf6c0d2a7a541eef145db84a6937R56

The reason for this extra step involving sending/receiving a message to/from the constellation, is that before deciding to unload and go ahead with the traversal, we first need to prompt the user(via a to be implemented blocking message send/receive to the embedder, and I believe we can't block from the constellation so it has to be done from script before sending the ScriptMsg::TraverseHistory). Only after the prompt can we unload, and traverse.

It's a bit of a hack, mostly replicating logic already found in Constellation.traverse_to_entry, and I'm wondering if it's the best way. Perhaps you have a better idea?

Also, am I correct to think that the concept of the 'current entry' of a browsing context, is same as the 'active document' that the spec refers to? The spec refers to "A browsing context's active document is its WindowProxy object's [[Window]] internal slot value's associated Document.", which in Servo is a pipeline id, I assumed it's the same pipeline as the 'current entry', but wasn't sure.

Thanks.

asajeffrey · 2018-04-03T14:39:15Z

OK, read through the current WIP, it looks much much nicer! Yay for not having to keep those timestamps around!

A couple of questions...

a) Way back when, we were keeping the session history as a list of pipeline ids, then we ripped it out and replaced it with the current solution of timestamps and explicit iterators. I'm not sure why we didn't go for this implementation then? Did we just not think of it?

b) In session_history.rs there's SessionHistoryChange, BrowsingContextDiff and SessionHistoryDiff, all of which do very similar jobs. I wonder if there's a way to unify all of these into one structure?

I also need to think about how we're handling unloading documents, and when the session history gets updated. Also, it might be worth writing an addendum to the navigation history paper talking about this implementation strategy.

cbrewster · 2018-04-03T15:31:58Z

@asajeffrey

a) The initial intention was to keep as close to the spec as possible while still having correct behavior. Unfortunately, that made it very difficult and complex to handle history state and discards. Since other browsers don't follow the spec either, I think its okay to do an implementation like this as long as the behavior matches.

b) Yeah there are a lot of new structs that have slightly different purposes.

SessionHistoryChange is a pending change (caused by a load)
SessionHistoryDiff represents a single diff between an entry and its adjacent entry. This is currently a single variant enum, but other variants will be added to handle History state and hash change.
BrowsingContextDiff is a case of SessionHistoryDiff where a browsing context node is changed.

In the future I would plan to add a PipelineDiff which is a variant of SessionHistoryDiff which handles history state and hash change.

I'm open to any ideas on how to reduce the number of structs here.

I would like to add this implementation to the paper and make sure it still meets all of the properties defined in the paper.

@gterzian I'll have to think about unloading a bit more before I can give any concrete ideas. The active document of a browsing context is the same thing as the document of the current entry. So yes, the pipeline of the current entry is the "active document".

asajeffrey · 2018-04-03T16:13:15Z

Perhaps we can unify all the *Diff types into one, and maybe handle the history state changes by allowing a diff where the old and new pipeline ids are the same?

I think SessionHistoryChange will have to stay, because its old pipeline id is optional, and is None in the case of the first session history entry.

cbrewster · 2018-04-03T19:16:21Z

My concern with combining the *Diff types into 1 is that the BrowsingContextDiff and PipelineDiff hold very different data. The former holds info about pipeline ids and load data for a specific browsing context and the latter holds info about hash changes/state changes for a specific pipeline.

As for SessionHistoryChange, I am wondering if we could get rid of old_pipeline_id. It is only used here:

servo/components/constellation/constellation.rs

Lines 1684 to 1688 in 122bfa0

    
           fn handle_script_loaded_url_in_iframe_msg(&mut self, load_info: IFrameLoadInfoWithData) { 
        
               let (load_data, window_size, is_private) = { 
        
                   let old_pipeline = load_info.old_pipeline_id 
        
                       .and_then(|old_pipeline_id| self.pipelines.get(&old_pipeline_id));

We could get the same pipeline by getting the active pipeline_id from the browsing context.

asajeffrey · 2018-04-03T22:32:17Z

Hmm, I see what you mean about the *Diffs. Perhaps a SessionHistoryDiff with something like:

enum SessionHistoryDiff {
  Initial {
    pipeline_id: PipelineId, 
    browsing_context_id: BrowsingContextId,
    load_data: LoadData,
  },
  Navigate {
    old_pipeline_id: PipelineId, 
    new_pipeline_id: PipelineId, 
    browsing_context_id: BrowsingContextId,
    old_load_data: LoadData,
    new_load_data: LoadData,
  },
  PushState {
    pipeline_id: PipelineId,
    browsing_context_id: BrowsingContextId,
    old_history_state: HistoryStateId,
    new_history_state: HistoryStateId,
  },
}

I think the reason why we're keeping the old pipeline id in the change is so we can discard the change if the browsing context has already changed? E.g. a race between two navigations?

cbrewster · 2018-04-04T23:19:08Z

IRC Chat: https://mozilla.logbot.info/servo/20180404#c14561798
TL;DR We can probably reduce the number of structs here. History discarding is a pain and we are unsure what to do in some of the edge cases.

cbrewster · 2018-04-05T05:16:53Z

@bors-servo try

bors-servo · 2018-04-05T05:16:57Z

⌛ Trying commit 35c1ade with merge e23dccd...

[WIP] New Session History  Remaining Work: - [x] Move `LoadData` from `BrowsingContext` to `Pipeline` - [x] Cleanup `*Diff` and `*Changeset` types - [ ] Implement pipeline discarding and reloading - [ ] Document new code ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20507)

bors-servo · 2018-04-05T05:43:53Z

💔 Test failed - linux-rel-css

cbrewster · 2018-04-05T19:13:36Z

@bors-servo try

bors-servo · 2018-04-05T19:13:41Z

⌛ Trying commit aa8e1b3 with merge 22bd4ed...

[WIP] New Session History  Remaining Work: - [x] Move `LoadData` from `BrowsingContext` to `Pipeline` - [x] Cleanup `*Diff` and `*Changeset` types - [ ] Implement pipeline discarding and reloading - [ ] Document new code ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20507)

bors-servo · 2018-04-05T20:08:39Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

asajeffrey · 2018-04-05T21:10:53Z

components/constellation/constellation.rs

@@ -2470,7 +2464,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
                .filter_map(|maybe_pipeline| maybe_pipeline)
                .collect::<Vec<_>>();

-            pipelines_to_evict.extend(session_history.future.iter()
+            pipelines_to_evict.extend(session_history.future.iter().rev()


Really? The session history future is in increasing chronological order isn't it? We should be evicting ones far off in the future, not the near future.

Yes we want to reverse here because this will put the diffs in the near future first, which we then skip by history_length.

asajeffrey

Yay, this is much nicer!

asajeffrey · 2018-04-05T21:16:46Z

components/constellation/browsingcontext.rs

@@ -29,72 +25,33 @@ pub struct BrowsingContext {
    /// The size of the frame.
    pub size: Option<TypedSize2D<f32, CSSPixel>>,

-    /// The timestamp for the current session history entry.
-    pub instant: Instant,


Yay! No more timestamps!

asajeffrey · 2018-04-05T21:19:19Z

components/constellation/constellation.rs

@@ -277,6 +274,8 @@ pub struct Constellation<Message, LTF, STF> {
    /// to become same-origin, at which point they can share DOM objects.
    event_loops: HashMap<TopLevelBrowsingContextId, HashMap<Host, Weak<EventLoop>>>,

+    joint_session_histories: HashMap<TopLevelBrowsingContextId, SessionHistory>,


Bikeshed: should we rename SessionHistory to TopLevelBrowsingContext?

IRC: JointSessionHistory

asajeffrey · 2018-04-05T21:21:26Z

components/constellation/constellation.rs

@@ -1732,14 +1661,17 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        let IFrameLoadInfo {
            parent_pipeline_id,
            new_pipeline_id,
-            replace,
+            replace: _replace,


Use a ... pattern for this?

whoops, meant to go and fix this...

asajeffrey · 2018-04-05T21:24:30Z

components/constellation/constellation.rs

+                        match diff {
+                            SessionHistoryDiff::BrowsingContextDiff {
+                                browsing_context_id,
+                                ref new_pipeline_id,


Do we need this ref? Does PipelineId not impl Copy?

This is an AliveOrDeadPipeline which can't have Copy since it may contain LoadData. Probably could do better names here.

Ah I see, yes a different name would help!

asajeffrey · 2018-04-05T21:25:41Z

components/constellation/constellation.rs

+                    }
+
+                    for _ in 0..forward {
+                        let diff = session_history.future.pop().expect("Infallible");


Can we remove this source of panic?

asajeffrey · 2018-04-05T21:40:16Z

components/constellation/session_history.rs

+            },
+            AliveOrDeadPipeline::Dead(pipeline_id, _) => {
+                match *other {
+                    AliveOrDeadPipeline::Dead(other_pipeline_id, _) if pipeline_id == other_pipeline_id => true,


asajeffrey · 2018-04-05T21:40:51Z

components/constellation/session_history.rs

+}
+
+impl PartialEq for AliveOrDeadPipeline {
+    fn eq(&self, other: &AliveOrDeadPipeline) -> bool {


Add a comment about why we're not just using the derived version.

asajeffrey · 2018-04-05T21:42:26Z

components/constellation/session_history.rs

+        match *self {
+            SessionHistoryDiff::BrowsingContextDiff { ref mut old_pipeline_id, ref mut new_pipeline_id, .. } => {
+                if *old_pipeline_id == *replaced_pipeline_id {
+                    *old_pipeline_id = pipeline_id.clone();


can we not copy the pipeline ids?

Same this, this is an AliveOrDeadPipeline which may contain LoadData

Variable naming?

asajeffrey · 2018-04-05T21:43:17Z

ports/servo/browser.rs

@@ -122,14 +122,18 @@ impl Browser {
            }
            (CMD_OR_ALT, None, Key::Right) | (KeyModifiers::NONE, None, Key::NavigateForward) => {
                if let Some(id) = self.browser_id {
-                    let event = WindowEvent::Navigation(id, TraversalDirection::Forward(1));
-                    self.event_queue.push(event);
+                    if state == KeyState::Pressed {


This looks like it should be in a separate PR.

asajeffrey · 2018-04-05T21:43:34Z

ports/servo/browser.rs

-                    let event = WindowEvent::Navigation(id, TraversalDirection::Back(1));
-                    self.event_queue.push(event);
+                    if state == KeyState::Pressed {
+                        let event = WindowEvent::Navigation(id, TraversalDirection::Back(1));


asajeffrey · 2018-04-05T21:45:18Z

Remove the WIP tag?

asajeffrey · 2018-04-05T22:54:38Z

OK, just some minor nits left, then we can squash and merge! I will trust you to do that, and I'm on PTO tomorrow, so you can r=me.

This new implementation of the session history keeps track of a single tree of browsing contexts and pipelines which represents the active entry of the session history and it keeps track of diffs between adjacent entries. This allows use to traverse across the joint session history by applying diffs to the active tree.

cbrewster · 2018-04-05T23:26:34Z

@bors-servo r=asajeffrey

bors-servo · 2018-04-05T23:26:34Z

📌 Commit f3d068f has been approved by asajeffrey

bors-servo · 2018-04-05T23:26:39Z

⌛ Testing commit f3d068f with merge 7f3b9ca...

New Session History  Remaining Work: - [x] Move `LoadData` from `BrowsingContext` to `Pipeline` - [x] Cleanup `*Diff` and `*Changeset` types - [x] Implement pipeline discarding and reloading - [x] Document new code ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20507)

bors-servo · 2018-04-06T00:29:26Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: asajeffrey
Pushing 7f3b9ca to master...

highfive assigned Manishearth Apr 3, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 3, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 3, 2018

asajeffrey assigned asajeffrey and unassigned Manishearth Apr 3, 2018

cbrewster added S-work-in-progress and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 5, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 5, 2018

cbrewster added the S-needs-squash Some (or all) of the commits in the PR should be combined. label Apr 5, 2018

cbrewster force-pushed the history_transactions branch 2 times, most recently from 6e433b1 to 35c1ade Compare April 5, 2018 05:16

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 5, 2018

asajeffrey reviewed Apr 5, 2018

View reviewed changes

asajeffrey removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 5, 2018

cbrewster changed the title ~~[WIP] New Session History~~ New Session History Apr 5, 2018

cbrewster removed the S-work-in-progress label Apr 5, 2018

cbrewster force-pushed the history_transactions branch from 29b84e3 to 369b8e2 Compare April 5, 2018 22:40

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 5, 2018

cbrewster force-pushed the history_transactions branch from 369b8e2 to f3d068f Compare April 5, 2018 23:26

cbrewster removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Apr 5, 2018

highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 5, 2018

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 5, 2018

bors-servo merged commit f3d068f into servo:master Apr 6, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 6, 2018

bors-servo mentioned this pull request Apr 6, 2018

Implement beforeunload event and infrastructure #20329

Merged

5 tasks

cbrewster deleted the history_transactions branch April 13, 2018 15:29

New Session History #20507

New Session History #20507

Conversation

cbrewster commented Apr 3, 2018 • edited

highfive commented Apr 3, 2018

cbrewster commented Apr 3, 2018

bors-servo commented Apr 3, 2018

bors-servo commented Apr 3, 2018

gterzian commented Apr 3, 2018 • edited

asajeffrey commented Apr 3, 2018

cbrewster commented Apr 3, 2018

asajeffrey commented Apr 3, 2018

cbrewster commented Apr 3, 2018

asajeffrey commented Apr 3, 2018

cbrewster commented Apr 4, 2018

cbrewster commented Apr 5, 2018

bors-servo commented Apr 5, 2018

bors-servo commented Apr 5, 2018

cbrewster commented Apr 5, 2018

bors-servo commented Apr 5, 2018

bors-servo commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey commented Apr 5, 2018

asajeffrey commented Apr 5, 2018

cbrewster commented Apr 5, 2018

bors-servo commented Apr 5, 2018

bors-servo commented Apr 5, 2018

bors-servo commented Apr 6, 2018

cbrewster commented Apr 3, 2018 •

edited

gterzian commented Apr 3, 2018 •

edited