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

Clear refcounted promise before dropping JSRuntime #21988

Merged
merged 2 commits into from Oct 29, 2018

Conversation

Projects
None yet
6 participants
@CYBAI
Collaborator

CYBAI commented Oct 21, 2018

Not sure if this is the right solution?

I also tried to impl Drop for LiveDOMReferences but it's still executed after dropping JSRuntime.
So, maybe we should clear it before dropping the JSRuntime?

cc @jdm @asajeffrey


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21331 .
  • There are tests for these changes; the status of fetch/cross-origin-resource-policy/fetch-in-iframe.html will be OK

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Oct 21, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/refcounted.rs, components/script/script_runtime.rs
  • @KiChjang: components/script/dom/bindings/refcounted.rs, components/script/script_runtime.rs
@CYBAI

This comment has been minimized.

Collaborator

CYBAI commented Oct 21, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 21, 2018

⌛️ Trying commit 2a7db89 with merge e25f305...

bors-servo added a commit that referenced this pull request Oct 21, 2018

Auto merge of #21988 - CYBAI:drop-promises, r=<try>
Clear refcounted promise before dropping JSRuntime

Not sure if this is the right solution?

I also tried to `impl Drop for LiveDOMReferences` but it's still executed after dropping JSRuntime.
So, maybe we should clear it before dropping the JSRuntime?

cc @jdm @asajeffrey

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21331 .
- [x] There are tests for these changes; the status of `fetch/cross-origin-resource-policy/fetch-in-iframe.html` will be `OK`

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

This comment has been minimized.

@emilio

This comment has been minimized.

Member

emilio commented Oct 29, 2018

r? @jdm

@highfive highfive assigned jdm and unassigned avadacatavra Oct 29, 2018

@jdm

This comment has been minimized.

Member

jdm commented Oct 29, 2018

@bors-servo r+
Yes, this looks appropriate. Thanks for fixing it!

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

📌 Commit 2a7db89 has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

⌛️ Testing commit 2a7db89 with merge e580250...

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

Auto merge of #21988 - CYBAI:drop-promises, r=jdm
Clear refcounted promise before dropping JSRuntime

Not sure if this is the right solution?

I also tried to `impl Drop for LiveDOMReferences` but it's still executed after dropping JSRuntime.
So, maybe we should clear it before dropping the JSRuntime?

cc @jdm @asajeffrey

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21331 .
- [x] There are tests for these changes; the status of `fetch/cross-origin-resource-policy/fetch-in-iframe.html` will be `OK`

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

This comment has been minimized.

@bors-servo bors-servo merged commit 2a7db89 into servo:master Oct 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

@CYBAI CYBAI deleted the CYBAI:drop-promises branch Oct 29, 2018

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