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

Calling matchMedia during a MQL change event will not panic #15495

Merged
merged 1 commit into from Feb 19, 2017
Merged

Calling matchMedia during a MQL change event will not panic #15495

merged 1 commit into from Feb 19, 2017
+51 −6

Conversation

@prampey
Copy link
Contributor

prampey commented Feb 10, 2017 •
edited

Calling matchMedia now leads to a new copy of MQL objects to prevent errors when borrowing references from MQL during an MQL change event.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14967 (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 Feb 10, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/mediaquerylist.rs
  • @KiChjang: components/script/dom/mediaquerylist.rs
@jdm jdm added the S-needs-rebase label Feb 10, 2017
@jdm jdm assigned jdm and unassigned pcwalton Feb 10, 2017
@jdm
jdm requested changes Feb 10, 2017
View changes
Copy link
Member

jdm left a comment

Great work! Just a couple stylistic comments, and the changes will need to be rebased against master.
tests/wpt/mozilla/tests/mozilla/mql_borrow.html Outdated
<script>
// Test to make sure matchMedia can be suitably used inside MediaQueryList listeners without any errors in borrowing
// mql references
var test = async_test("No clashes found while trying to get a refernce to mql");

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 10, 2017

Member

"Using matchMedia inside of a MediaQueryList callback does not panic"
tests/wpt/mozilla/tests/mozilla/mql_borrow.html Outdated
assert_not_equals(match3, undefined);
}));
window.resizeTo(600, 400);
}));

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 10, 2017

Member

Let's unindent this code to match var test.
components/script/dom/mediaquerylist.rs Outdated
}
// Sending change events for all changed Media Queries
for mql in mql_list.iter()
{

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 10, 2017

Member

nit: for mql in &mql_list {

This comment has been minimized.

Sign in to view
Copy link
@prampey

prampey Feb 10, 2017

Author Contributor

@jdm For the sake of theory, there is no performance difference between the two right?

This comment has been minimized.

Sign in to view
Copy link
@prampey

prampey Feb 10, 2017

Author Contributor

Also, I get an error saying that RootedVec doesn't implement the Iterator trait.

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 10, 2017

Member

Correct.

This comment has been minimized.

Sign in to view
Copy link
@prampey

prampey Feb 10, 2017

Author Contributor

I get that error for &mql_list. Should I revert back?

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 10, 2017

Member

Yes, my mistake. Please keep the { on the same line, though.
components/script/dom/mediaquerylist.rs Outdated
@@ -138,16 +138,24 @@ impl WeakMediaQueryListVec {
/// Evaluate media query lists and report changes
/// https://drafts.csswg.org/cssom-view/#evaluate-media-queries-and-report-changes
pub fn evaluate_and_report_changes(&self) {
for mql in self.cell.borrow().iter() {
rooted_vec!(let mut mql_list);
for mql in self.cell.borrow_mut().iter() {

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 10, 2017

Member

Why do we need borrow_mut here?

This comment has been minimized.

Sign in to view
Copy link
@prampey

prampey Feb 10, 2017

Author Contributor

Yeah it works without.
@prampey
Copy link
Contributor Author

prampey commented Feb 13, 2017

@jdm Cool. I'm done with the stylistic changes but I seem to be a bit confused when resolving MANIFEST.json.
@jdm
Copy link
Member

jdm commented Feb 13, 2017

@prampey You should be able to revert all changes to MANIFEST.json and run ./mach manifest-update to get a version which is correct.
@prampey
Copy link
Contributor Author

prampey commented Feb 15, 2017

@jdm Yes, I already used update-manifest. However while rebasing, the latest commit and my commit seem to have huge differences in code. How do I go about this?
@jdm
Copy link
Member

jdm commented Feb 15, 2017

@prampey Do you mean in MANIFEST.json? My point is that while rebasing you can just resolve merge conflicts in that file by reverting all changes and running update-manifest again, then adding the modified file.
@prampey prampey force-pushed the prampey:mql-borrow-error branch from 1f0d4bc to f846508 Feb 16, 2017
@prampey prampey force-pushed the prampey:mql-borrow-error branch from f846508 to 2369de7 Feb 16, 2017
@prampey
Copy link
Contributor Author

prampey commented Feb 16, 2017

@jdm Oh okay, my bad. I pushed in the new changes.
@jdm jdm removed the S-needs-rebase label Feb 16, 2017
@jdm
Copy link
Member

jdm commented Feb 16, 2017

@bors-servo: r+
Thanks a lot for fixing this!
@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

📌 Commit 2369de7 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

⌛ Testing commit 2369de7 with merge 89b2275...
bors-servo added a commit that referenced this pull request Feb 16, 2017
@bors-servo
Auto merge of #15495 - prampey:mql-borrow-error, r=jdm
Loading status checks…
89b2275 
Calling matchMedia during a MQL change event will not panic

<!-- Please describe your changes on the following line: -->
Calling matchMedia now leads to a new copy of MQL objects to prevent errors when borrowing references from MQL during an MQL change event.

---
<!-- 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
- [X] These changes fix #14967  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [x] 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/15495)
<!-- Reviewable:end -->
@prampey
Copy link
Contributor Author

prampey commented Feb 16, 2017 •
edited

@jdm Thanks a lot for bearing with me :) Also, I think it'd be really convenient for developers if we added the documentation link (http://testthewebforward.org/docs/testharness-library.html) in the wpt Readme file. Should I open an issue and send in a PR?
@jdm
Copy link
Member

jdm commented Feb 16, 2017

Please do!
@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2017

💔 Test failed - mac-rel-wpt2
@jdm
Copy link
Member

jdm commented Feb 17, 2017

I suspect the problem is that resizeTo does not trigger a resize event when running Servo in headless mode. If you resize an iframe instead then I expect it will work. Please file an issue with this test case so we can track the problem with headless mode!
@prampey prampey force-pushed the prampey:mql-borrow-error branch from 2369de7 to 60f5155 Feb 17, 2017
@prampey
Copy link
Contributor Author

prampey commented Feb 17, 2017

@jdm Yes, iframes seemed to work. Just pushed in the update.
@jdm
Copy link
Member

jdm commented Feb 17, 2017

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

bors-servo commented Feb 17, 2017

📌 Commit 60f5155 has been approved by jdm
@jdm
Copy link
Member

jdm commented Feb 17, 2017

I filed #15621 about the headless resize issue.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 18, 2017

⌛ Testing commit 60f5155 with merge e91f316...
bors-servo added a commit that referenced this pull request Feb 18, 2017
@bors-servo
Auto merge of #15495 - prampey:mql-borrow-error, r=jdm
Loading status checks…
e91f316 
Calling matchMedia during a MQL change event will not panic

<!-- Please describe your changes on the following line: -->
Calling matchMedia now leads to a new copy of MQL objects to prevent errors when borrowing references from MQL during an MQL change event.

---
<!-- 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
- [X] These changes fix #14967  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [x] 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/15495)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 18, 2017

💔 Test failed - mac-dev-unit
@jdm
Copy link
Member

jdm commented Feb 18, 2017

Bah, another manifest-update is necessary :<
@prampey
Calling matchMedia during a MQL change event will not panic
Loading status checks…
0c21333
@prampey prampey force-pushed the prampey:mql-borrow-error branch from 60f5155 to 0c21333 Feb 18, 2017
@nox
Copy link
Member

nox commented Feb 19, 2017

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

📌 Commit 0c21333 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

⌛ Testing commit 0c21333 with merge debdebe...
bors-servo added a commit that referenced this pull request Feb 19, 2017
@bors-servo
Auto merge of #15495 - prampey:mql-borrow-error, r=jdm
Loading status checks…
debdebe 
Calling matchMedia during a MQL change event will not panic

<!-- Please describe your changes on the following line: -->
Calling matchMedia now leads to a new copy of MQL objects to prevent errors when borrowing references from MQL during an MQL change event.

---
<!-- 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
- [X] These changes fix #14967  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [x] 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/15495)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 19, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: jdm
Pushing debdebe to master...
@bors-servo bors-servo merged commit 0c21333 into servo:master Feb 19, 2017
3 checks passed
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
@prampey prampey deleted the prampey:mql-borrow-error branch Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Calling matchMedia during a MQL change event will panic
6 participants
@prampey @highfive @jdm @bors-servo @nox @pcwalton
You can’t perform that action at this time.