Skip to content

/ servo

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

Use dom and new_inherited in (extendable)messageevent #25467

Merged
merged 1 commit into from Jan 9, 2020
Merged

Use dom and new_inherited in (extendable)messageevent #25467

merged 1 commit into from Jan 9, 2020

Conversation

@gterzian
Copy link
Member

gterzian commented Jan 9, 2020 •
edited

Also FIX #24707

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #25464 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@gterzian
use dom and new_inherited in (extendable)messageevent
Loading status checks…
a9fcd73
@highfive
Copy link

highfive commented Jan 9, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/messageevent.rs, components/script/dom/extendablemessageevent.rs
  • @KiChjang: components/script/dom/messageevent.rs, components/script/dom/extendablemessageevent.rs
@highfive
Copy link

highfive commented Jan 9, 2020

warning Warning warning

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

gterzian commented Jan 9, 2020

@Manishearth r?
@gterzian gterzian assigned Manishearth and unassigned nox Jan 9, 2020
@Manishearth
Manishearth reviewed Jan 9, 2020
View changes
components/script/dom/messageevent.rs
.iter()
.map(|port| DomRoot::from_ref(&**port))
.collect();
message_ports_to_frozen_array(ports.as_slice(), cx)

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Jan 9, 2020

Member

We should update message_ports_to_frozen_array to work on &[T] where T: ToJSValConvertible instead, and once we do that we should make it generic. But perhaps not in this PR.
@Manishearth
Copy link
Member

Manishearth commented Jan 9, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2020

📌 Commit a9fcd73 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2020

⌛ Testing commit a9fcd73 with merge fc910d3...
bors-servo added a commit that referenced this pull request Jan 9, 2020
@bors-servo
Auto merge of #25467 - gterzian:fix_message_event, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
fc910d3 
Use dom and new_inherited in (extendable)messageevent

<!-- 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 #25464 (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. -->
@Manishearth
Copy link
Member

Manishearth commented Jan 9, 2020

@bors-servo p=-1 force retry
@Manishearth Manishearth closed this Jan 9, 2020
@Manishearth Manishearth reopened this Jan 9, 2020
@Manishearth
Copy link
Member

Manishearth commented Jan 9, 2020

This is r=me, but I'm going to wait for #25463 to land because Taskcluster has that build cached
@Manishearth Manishearth closed this Jan 9, 2020
@Manishearth Manishearth reopened this Jan 9, 2020
@Manishearth
Copy link
Member

Manishearth commented Jan 9, 2020

@bors-servo r- retry

forgot that bors remembers results
@gterzian
Copy link
Member Author

gterzian commented Jan 9, 2020 •
edited

This seems to have also positively affected #24707 (which doesn't crash anymore), (and #24810 (which now has a different error) EDIT: incorrect, it's the same error on the second issue, however the crash on the first seems to have dissapeared).

I guess storing a DomRoot on a dom struct prevents proper tracing, which could have caused the GC?

cc @jdm
@Manishearth
Copy link
Member

Manishearth commented Jan 9, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2020

📌 Commit a9fcd73 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2020

⌛ Testing commit a9fcd73 with merge f8c957d...
bors-servo added a commit that referenced this pull request Jan 9, 2020
@bors-servo
Auto merge of #25467 - gterzian:fix_message_event, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f8c957d 
Use dom and new_inherited in (extendable)messageevent

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

Also FIX #24707

---
<!-- 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 #25464 (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. -->
@gterzian gterzian mentioned this pull request Jan 9, 2020
Merged
0 of 5 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2020

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing f8c957d to master...
@bors-servo bors-servo merged commit a9fcd73 into servo:master Jan 9, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:fix_message_event branch Jan 9, 2020
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1eeecf0 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- 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 #___ (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. -->
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
2a36c00 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- 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 #___ (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. -->
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a9b4e31 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- 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 #___ (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. -->
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
dbba9ea 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- 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 #___ (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. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth

Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

MessageEvent and ExtendableMessageEvent have some issues
"Messageport Gc'ed too early" panic in /fetch/security/embedded-credentials.tentative.sub.html
5 participants
@gterzian @highfive @Manishearth @bors-servo @nox
You can’t perform that action at this time.