Skip to content

Clear refcounted promise before dropping JSRuntime#21988

Merged
bors-servo merged 2 commits intoservo:masterfrom
CYBAI:drop-promises
Oct 29, 2018
Merged

Clear refcounted promise before dropping JSRuntime#21988
bors-servo merged 2 commits intoservo:masterfrom
CYBAI:drop-promises

Conversation

@CYBAI
Copy link
Copy Markdown
Member

@CYBAI 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



This change is Reviewable

@highfive
Copy link
Copy Markdown

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 21, 2018
@CYBAI
Copy link
Copy Markdown
Member Author

CYBAI commented Oct 21, 2018

@bors-servo try

@bors-servo
Copy link
Copy Markdown
Contributor

⌛ Trying commit 2a7db89 with merge e25f305...

bors-servo pushed a commit that referenced this pull request Oct 21, 2018
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
Copy link
Copy Markdown
Contributor

@emilio
Copy link
Copy Markdown
Member

emilio commented Oct 29, 2018

r? @jdm

@highfive highfive assigned jdm and unassigned avadacatavra Oct 29, 2018
@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 29, 2018

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

@bors-servo
Copy link
Copy Markdown
Contributor

📌 Commit 2a7db89 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 29, 2018
@bors-servo
Copy link
Copy Markdown
Contributor

⌛ Testing commit 2a7db89 with merge e580250...

bors-servo pushed a commit that referenced this pull request Oct 29, 2018
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
Copy link
Copy Markdown
Contributor

@bors-servo bors-servo merged commit 2a7db89 into servo:master Oct 29, 2018
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 29, 2018
@CYBAI CYBAI deleted the drop-promises branch October 29, 2018 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Promise destructor can crash when clearing TLS list of rooted promises

6 participants