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

Implement document load cancellation #21111

Merged
merged 6 commits into from Jul 29, 2018

Conversation

Projects
None yet
6 participants
@gterzian
Collaborator

gterzian commented Jul 1, 2018


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #19309 fix #21114 fix #21113 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Jul 1, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/eventsource.rs, components/script/dom/document.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script/document_loader.rs
  • @KiChjang: components/script/dom/eventsource.rs, components/script/dom/document.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script/document_loader.rs
@highfive

This comment has been minimized.

highfive commented Jul 1, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian

This comment has been minimized.

Collaborator

gterzian commented Jul 1, 2018

Not ready for review yet...

@gterzian gterzian force-pushed the gterzian:implement_document_load_cancellation branch from 962e8e8 to 925b929 Jul 1, 2018

@KiChjang

This comment has been minimized.

Member

KiChjang commented Jul 2, 2018

We should probably also use this opportunity to rename cancel to terminate, since that's the exact same term used by the spec.

// the user agent must abort any instance of the fetch algorithm opened by this EventSource.
self.canceller.borrow_mut().cancel();
}
}

This comment has been minimized.

@gterzian

gterzian Jul 2, 2018

Collaborator

Question on this: is the Drop of a Rust DomRoot going to be called when the associated JsObject is garbage collected in the JS engine?

(The above might be un-necessary since I've also added a change that make the globalscope keep a reference to all EventSource, and cancel them manually when needed, could still be a good backup ensuring we indeed always abort the fetch if the JSObject is garbarge collected)

This comment has been minimized.

@jdm

jdm Jul 4, 2018

Member

Yes.

@gterzian gterzian referenced this pull request Jul 13, 2018

Merged

Use remote-event task source in EventSource #21170

0 of 5 tasks complete

bors-servo added a commit that referenced this pull request Jul 13, 2018

Auto merge of #21170 - gterzian:use_remote_event_task_source_for_even…
…tsource, r=jdm

Use remote-event task source in EventSource

<!-- Please describe your changes on the following line: -->
Follow up on #21126, this again 'changes nothing', but will be useful for #21111 in the context of #21114

Trying to break up work into smaller PR's ;)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #21112 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/21170)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Jul 13, 2018

Auto merge of #21170 - gterzian:use_remote_event_task_source_for_even…
…tsource, r=jdm

Use remote-event task source in EventSource

<!-- Please describe your changes on the following line: -->
Follow up on #21126, this again 'changes nothing', but will be useful for #21114 in the context of #21111.

Trying to break up work into smaller PR's ;)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #21112 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/21170)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Jul 14, 2018

Auto merge of #21170 - gterzian:use_remote_event_task_source_for_even…
…tsource, r=jdm

Use remote-event task source in EventSource

<!-- Please describe your changes on the following line: -->
Follow up on #21126, this again 'changes nothing', but will be useful for #21114 in the context of #21111.

Trying to break up work into smaller PR's ;)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #21112 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/21170)
<!-- Reviewable:end -->

@gterzian gterzian force-pushed the gterzian:implement_document_load_cancellation branch from 0dd5c53 to dcdebd8 Jul 21, 2018

// TODO: https://github.com/servo/servo/issues/15236
self.window.cancel_all_tasks();
self.window.cancel_all_tasks_from_source(TaskSourceName::Networking);

This comment has been minimized.

@KiChjang

KiChjang Jul 21, 2018

Member

This carries an assumption that all the tasks queued on the networking task source are fetches. I think it is ok for now, but we'd definitely want to leave a comment here clarifying the assumption.

This comment has been minimized.

@gterzian

gterzian Jul 22, 2018

Collaborator

Good catch, I'll add a note.

This comment has been minimized.

@gterzian

gterzian Jul 22, 2018

Collaborator

I've also filed whatwg/html#3837

fn process_response_done(&mut self, aborted:bool) {
if aborted {
// https://html.spec.whatwg.org/multipage/
// server-sent-events.html#sse-processing-model:fail-the-connection

This comment has been minimized.

@KiChjang

This comment has been minimized.

@gterzian

gterzian Jul 22, 2018

Collaborator

Yep, thanks!

@gterzian gterzian force-pushed the gterzian:implement_document_load_cancellation branch from 690dd2e to 99b7f30 Jul 22, 2018

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Jul 22, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 22, 2018

⌛️ Trying commit 99b7f30 with merge fa6711c...

bors-servo added a commit that referenced this pull request Jul 22, 2018

Auto merge of #21111 - gterzian:implement_document_load_cancellation,…
… r=<try>

[WIP] Implement document load cancellation

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19309 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/21111)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 22, 2018

💔 Test failed - linux-dev

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Jul 22, 2018

Error:
/home/servo/buildbot/slave/linux-dev/build/python/_virtualenv/local/lib/python2.7/site-packages/cryptography/hazmat/primitives/constant_time.py:26: CryptographyDeprecationWarning: Support for your Python version is deprecated. The next version of cryptography will remove support. Please upgrade to a 2.7.x release that supports hmac.compare_digest as soon as possible.
  utils.DeprecatedIn23,
awscli 1.15.56 has requirement botocore==1.10.55, but you'll have botocore 1.5.95 which is incompatible.

servo/saltfs#862

@gterzian gterzian force-pushed the gterzian:implement_document_load_cancellation branch from 99b7f30 to d91ce07 Jul 22, 2018

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Jul 22, 2018

@bors-servo try (some servers still work, and I'd like to know where I stand in terms of the overall test suite)

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 22, 2018

⌛️ Trying commit d91ce07 with merge 267c721...

bors-servo added a commit that referenced this pull request Jul 22, 2018

Auto merge of #21111 - gterzian:implement_document_load_cancellation,…
… r=<try>

[WIP] Implement document load cancellation

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19309 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/21111)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 22, 2018

💔 Test failed - linux-rel-css

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Jul 29, 2018

This one has come back, and this time it's OK...

{"status": "OK", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/semantics/scripting-1/the-script-element/script-onerror-insertion-point-2.html", "line": 94095, "action": "test_result", "expected": "TIMEOUT"}
@gterzian

This comment has been minimized.

Collaborator

gterzian commented Jul 29, 2018

@bors-servo retry

bors-servo added a commit that referenced this pull request Jul 29, 2018

Auto merge of #21111 - gterzian:implement_document_load_cancellation,…
… r=<try>

Implement document load cancellation

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19309 fix #21114 fix #21113 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/21111)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2018

⌛️ Trying commit dbdfda8 with merge e543cba...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2018

💔 Test failed - linux-rel-wpt

@gterzian gterzian force-pushed the gterzian:implement_document_load_cancellation branch from dbdfda8 to e67adfc Jul 29, 2018

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Jul 29, 2018

Ok, removed the TIMEOUT for {"status": "OK", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/semantics/scripting-1/the-script-element/script-onerror-insertion-point-2.html", "line": 94095, "action": "test_result", "expected": "TIMEOUT"

@jdm

This comment has been minimized.

Member

jdm commented Jul 29, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2018

📌 Commit e67adfc has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2018

⌛️ Testing commit e67adfc with merge 076198f...

bors-servo added a commit that referenced this pull request Jul 29, 2018

Auto merge of #21111 - gterzian:implement_document_load_cancellation,…
… r=jdm

Implement document load cancellation

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19309 fix #21114 fix #21113 (github issue number if applicable).

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/21111)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2018

@bors-servo bors-servo merged commit e67adfc into servo:master Jul 29, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@gterzian gterzian deleted the gterzian:implement_document_load_cancellation branch Jul 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment