Skip to content
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

Refactor promise compartment #23253

Merged
merged 4 commits into from Apr 29, 2019

Conversation

Projects
None yet
5 participants
@AZWN
Copy link
Contributor

commented Apr 24, 2019

This PR adds a mechanism to verify that certain code is executed inside a JSAutoCompartment, and applies this to the Promise::new_in_current_compartment constructor.

r? @jdm


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23167
  • These changes do not require tests because they do not change existing functionality.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 24, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/navigator.rs, components/script/dom/rtcpeerconnection.rs, components/script/dom/vrdisplay.rs, components/script/dom/customelementregistry.rs, components/script/dom/baseaudiocontext.rs and 22 more
  • @KiChjang: components/script/dom/navigator.rs, components/script/dom/rtcpeerconnection.rs, components/script/dom/vrdisplay.rs, components/script/dom/customelementregistry.rs, components/script/dom/baseaudiocontext.rs and 22 more
@highfive

This comment has been minimized.

Copy link

commented Apr 24, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@AZWN

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

I have one open question/issue. The available Promise constructors now look like this:

impl Promise {
    pub fn new(global: &GlobalScope, comp: &InCompartment) -> Rc<Promise> {
        Promise::new_in_current_compartment(global, comp)
    }

    #[allow(unsafe_code)]
    pub fn new_in_current_compartment(global: &GlobalScope, _comp: &InCompartment) -> Rc<Promise> {
        // ...
    }
    // ...
}

To, me, having both Promise::new and Promise::new_in_current_compartment looks a bit redundant, since the first is just delegating to the second. Would it be better to remove Promise::new_in_current_compartment?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Promise::new should not accept an InCompartment argument. Instead it should enter the compartment for global before calling new_in_current_compartment.

@AZWN

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Ok, so If I understand you well, it should use InCompartment::Entered like his:

impl Promise {
    pub fn new(global: &GlobalScope) -> Rc<Promise> {
        let comp = JSAutoCompartment::with_obj(&global.get_cx(), &global);
        Promise::new_in_current_compartment(global, &InCompartment::Entered(&comp))
    }

    #[allow(unsafe_code)]
    pub fn new_in_current_compartment(global: &GlobalScope, _comp: &InCompartment) -> Rc<Promise> {
        // ...
    }
    // ...
}

?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

That's the idea! I think we can pass InCompartment by value, rather than by reference, however.

@AZWN

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Ok, give me a moment, and I will implement it.

I am not sure about the fact InCompartment should be passed by value though. The advantage of references in my opinion is that it is possible to reuse InCompartment values for multiple calls. For now, that is not that relevant yet, but if you are planning to extend this pattern to more use cases it might become an advantage.

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

If you add #[derive(Copy, Clone)] to the InCompartment struct definition, it should be possible to reuse them.

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Fantastic! I can already see places where we can improve our safety guarantees based on these changes. Thanks for doing this work!
@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

📌 Commit eb73cce has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

⌛️ Testing commit eb73cce with merge 486f235...

bors-servo added a commit that referenced this pull request Apr 24, 2019

Auto merge of #23253 - BartGitHub:refactor-promise-compartment, r=jdm
Refactor promise compartment

<!-- Please describe your changes on the following line: -->
This PR adds a mechanism to verify that certain code is executed inside a ```JSAutoCompartment```, and applies this to the ```Promise::new_in_current_compartment``` constructor.

r? @jdm

---
<!-- 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 #23167

<!-- Either: -->
- [x] These changes do not require tests because they do not change existing functionality.

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

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

error[E0308]: mismatched types
  --> components/script/body.rs:57:9
   |
57 |         &InCompartment::Already(&in_compartment_proof),
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |         |
   |         expected enum `compartments::InCompartment`, found reference
   |         help: consider removing the borrow: `InCompartment::Already(&in_compartment_proof)`
   |
   = note: expected type `compartments::InCompartment<'_>`
              found type `&compartments::InCompartment<'_>`

error[E0599]: no function or associated item named `with_obj` found for type `js::jsapi::JSAutoCompartment` in the current scope
  --> components/script/dom/promise.rs:84:39
   |
84 |         let comp = JSAutoCompartment::with_obj(&global.get_cx(), &global);
   |                                       ^^^^^^^^ function or associated item not found in `js::jsapi::JSAutoCompartment`

error: aborting due to 2 previous errors

@AZWN AZWN force-pushed the BartGitHub:refactor-promise-compartment branch from eb73cce to e2e6e2a Apr 25, 2019

Show resolved Hide resolved components/script/dom/promise.rs Outdated
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

📌 Commit 3229af5 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

⌛️ Testing commit 3229af5 with merge 852223b...

bors-servo added a commit that referenced this pull request Apr 29, 2019

Auto merge of #23253 - BartGitHub:refactor-promise-compartment, r=jdm
Refactor promise compartment

<!-- Please describe your changes on the following line: -->
This PR adds a mechanism to verify that certain code is executed inside a ```JSAutoCompartment```, and applies this to the ```Promise::new_in_current_compartment``` constructor.

r? @jdm

---
<!-- 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 #23167

<!-- Either: -->
- [x] These changes do not require tests because they do not change existing functionality.

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

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@bors-servo bors-servo merged commit 3229af5 into servo:master Apr 29, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@AZWN AZWN deleted the BartGitHub:refactor-promise-compartment branch Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.