Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

History State API #15261

Closed
wants to merge 7 commits into from
Closed

History State API #15261

wants to merge 7 commits into from

Conversation

@cbrewster
Copy link
Member

cbrewster commented Jan 27, 2017

This PR implements the state-related portions of the History API. We need to think out how exactly to handle the history state in the case of 1. Reloads and 2. Pruning/Reloading Distant 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 _____

This change is Reviewable

@highfive
Copy link

highfive commented Jan 27, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/frame.rs, components/constellation/constellation.rs
  • @fitzgen: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/popstateevent.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/history.rs, components/script/script_thread.rs, components/script/dom/webidls/History.webidl, components/script/dom/bindings/trace.rs
  • @KiChjang: components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/popstateevent.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/history.rs, components/script/script_thread.rs, components/script/dom/webidls/History.webidl, components/script/dom/bindings/trace.rs
@highfive
Copy link

highfive commented Jan 27, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@cbrewster
Copy link
Member Author

cbrewster commented Jan 27, 2017

@highfive highfive assigned asajeffrey and unassigned wafflespeanut Jan 27, 2017
@cbrewster cbrewster mentioned this pull request Jan 27, 2017
3 of 5 tasks complete
@cbrewster
Copy link
Member Author

cbrewster commented Jan 27, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2017

Trying commit c9582e7 with merge 4b08440...

bors-servo added a commit that referenced this pull request Jan 27, 2017
History State API

<!-- Please describe your changes on the following line: -->
This PR implements the state-related portions of the History API. We need to think out how exactly to handle the history state in the case of 1. Reloads and 2. Pruning/Reloading Distant History.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15261)
<!-- Reviewable:end -->
[Manual scroll restoration should take precedent over scrolling to fragment in cross doc navigation]
expected: FAIL
expected: TIMEOUT

This comment has been minimized.

@cbrewster

cbrewster Jan 27, 2017

Author Member

This new TIMEOUT is due to hashchange events not being implemented yet.

[history.{push,replace}State retain scroll restoration mode and navigation in the same document respects it]
expected: FAIL
expected: TIMEOUT

This comment has been minimized.

@cbrewster

cbrewster Jan 27, 2017

Author Member

Same as above

@cbrewster cbrewster mentioned this pull request Jan 27, 2017
18 of 24 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2017

💔 Test failed - linux-rel-wpt

@cbrewster
Copy link
Member Author

cbrewster commented Jan 27, 2017

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: attribute state

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: operation pushState(any,DOMString,DOMString)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: operation replaceState(any,DOMString,DOMString)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property "state" with the proper type (2)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property "pushState" with the proper type (6)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: calling pushState(any,DOMString,DOMString) on window.history with too few arguments must throw TypeError

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: window.history must inherit property "replaceState" with the proper type (7)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] History interface: calling replaceState(any,DOMString,DOMString) on window.history with too few arguments must throw TypeError
@cbrewster cbrewster force-pushed the cbrewster:finish_the_history branch from c9582e7 to ff16b67 Jan 27, 2017
@cbrewster cbrewster force-pushed the cbrewster:finish_the_history branch 3 times, most recently from bb10d3d to 979e719 Jan 27, 2017
@cbrewster
Copy link
Member Author

cbrewster commented Jan 27, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2017

Trying commit 979e719 with merge 886c7a7...

bors-servo added a commit that referenced this pull request Jan 27, 2017
History State API

<!-- Please describe your changes on the following line: -->
This PR implements the state-related portions of the History API. We need to think out how exactly to handle the history state in the case of 1. Reloads and 2. Pruning/Reloading Distant History.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15261)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2017

💔 Test failed - linux-rel-wpt

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2017

The latest upstream changes (presumably #14971) made this pull request unmergeable. Please resolve the merge conflicts.

@cbrewster
Copy link
Member Author

cbrewster commented Jan 29, 2017

@jdm I have been looking at how to handle the StructuredCloneData. I have tried using an enum that stores either StructuredCloneData or a cached Heap<JSVal>. This enum is stored in a HistoryEntry which is stored in a HashMap on History. The issue is that every time I read from StructuredCloneData, it always gives me a garbage value. I noticed that Gecko is using a StructuredCloneContainer, I am not sure if that is just a wrapper over StructuredCloneData or if its doing something more complicated.

@cbrewster cbrewster force-pushed the cbrewster:finish_the_history branch from 979e719 to 30a1d59 Feb 1, 2017
@cbrewster
Copy link
Member Author

cbrewster commented Feb 10, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

Trying commit 677f30c with merge 8695eb1...

bors-servo added a commit that referenced this pull request Feb 10, 2017
History State API

<!-- Please describe your changes on the following line: -->
This PR implements the state-related portions of the History API. We need to think out how exactly to handle the history state in the case of 1. Reloads and 2. Pruning/Reloading Distant History.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15261)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

💔 Test failed - linux-rel-wpt

@cbrewster cbrewster force-pushed the cbrewster:finish_the_history branch from 677f30c to 4e2f6af Feb 10, 2017
@cbrewster
Copy link
Member Author

cbrewster commented Feb 13, 2017

Reusing the same pipeline id does cause some other unintended issues as Pipelines are not closed right away. They are closed after the constellation received the pipeline exited message from the script thread. Having an entry keeping the pipeline id causes some unintended messages to be send. There are also cases where we may discard a pipeline and traverse back to that entry that had that pipeline before the pipeline was completely closed.

After thinking about it, I wonder if we could something along these lines:

  • Use a Rc<Cell<Option<PipelineId>>> to group entries that share pipelines. This way when we discard the pipeline of an entry, we can set the Option to None and that will in turn update all the entries that share the same document. The only issue with this is we currently store a PipelineId on Frame which would break the reference.
  • This will be a bit invasive when dealing with updating the pipeline with the newly created pipeline. We currently use the same logic when navigating with replacement enabled and reloading the document that multiple entries share. Navigating with replacement should create a new pipeline and only affect the single reloaded entry. Reloading a discarded Pipeline should update all entries that previously shared the pipeline.
@cbrewster
Copy link
Member Author

cbrewster commented Feb 15, 2017

Blocked on #15566

@asajeffrey
Copy link
Member

asajeffrey commented Feb 16, 2017

Sounds like recycling pipeline IDs is having unexpected consequences.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

The latest upstream changes (presumably #15537) made this pull request unmergeable. Please resolve the merge conflicts.

cbrewster added 7 commits Feb 15, 2017
This change implements handling history state entries and hash change
history entries. The constellation is in charge of managing session
history and traversals. The History object is in charge of updating all
state on the script/dom side of things.

The resource thread stores the serialized history state data. This data
must be stored outside the script thread as the script thread may be
shut down if its document is purged during history pruning. Storing the
state data allows for retrieval of the state data at a later time even
if the document had to be recreated.

Each state is tracked using a state ID which is a UUID.

Traversal tasks are now queued on the history traversal task source. The
runnable waits synchronously on the constellation to finish handling the
traversal. This allows for tests to use setTimeout to wait for a
traversal to be handled before the callback has a chance of being
called.
update test expectations
@cbrewster cbrewster force-pushed the cbrewster:finish_the_history branch from 4e2f6af to 13a9509 Feb 17, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Feb 18, 2017

The latest upstream changes (presumably #15588) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey
Copy link
Member

asajeffrey commented Feb 24, 2017

IRC conversation about session histories with multiple instances of the same pipeline id: http://logs.glob.uno/?c=mozilla%23servo&s=24+Feb+2017&e=24+Feb+2017#c619161

TL;DR: If you have document A which pushStates to B then again to C, then their shared Document is discarded, then the history is traversed to B, what happens? Our read of the spec is that A, B and C should all be reloaded to the same fresh document. This means that any session history entries with the same pipeline id must be contiguous.

(Duplicating #10992 (comment) just to make sure I don't forget.)

@nox
Copy link
Member

nox commented Apr 4, 2017

What is the status on this? If this is stalled, could we close the PR and file an issue referencing it?

@cbrewster
Copy link
Member Author

cbrewster commented Apr 4, 2017

@nox right now we need to find a better way of handling distant history pipeline discard that allows the history to contain multiple entries with the same pipeline id. I'm finishing up school at the moment so I don't have time right now, but I plan on finishing this up here in a couple weeks.

I'll close for now until I have time to resume work.

@cbrewster cbrewster closed this Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.