script creates methods taking '*mut JSContext' unsafe #11595

ab22 · 2016-06-03T20:32:10Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11485

There are tests for these changes OR
These changes do not require tests because: no code logic was changed

Just as additional information, in case I missed any, these are the places where I found *mut JSContext on components/script/dom/bindings/codegen/CodegenRust.py:

JSContext params are stripped here:
https://github.com/servo/servo/blob/master/components/script/dom/bindings/codegen/CodegenRust.py#L5713

Can't find code where this is called:
https://github.com/servo/servo/blob/master/components/script/dom/bindings/codegen/CodegenRust.py#L6003

This change is

highfive · 2016-06-03T20:32:15Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/textdecoder.rs, components/script/dom/textencoder.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/crypto.rs, components/script/dom/testbinding.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/window.rs, components/script/dom/document.rs, components/script/dom/eventtarget.rs, components/script/dom/workerglobalscope.rs, components/script/dom/imagedata.rs, components/script/dom/popstateevent.rs, components/script/dom/customevent.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/errorevent.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/messageevent.rs, components/script/dom/worker.rs, components/script/dom/node.rs, components/script/dom/xmldocument.rs, components/script/dom/bindings/reflector.rs, components/script/dom/webglrenderingcontext.rs
@emilio: components/script/dom/webglrenderingcontext.rs

highfive · 2016-06-03T20:32:16Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

ab22 · 2016-06-03T20:54:42Z

Travis build is failing due to components/script/dom/serviceworkerglobalscope.rs. It got added 2 days ago. I will rebase as soon as I get back.


        script creates methods taking '*mut JSContext' unsafe

rebase + marked the necessary new code as unsafe

highfive · 2016-06-04T00:20:10Z

New code was committed to pull request.

jdm · 2016-06-07T15:49:20Z

Sorry for the delay here; someone else should take this over from me.
r? @nox

nox · 2016-06-08T10:34:39Z

-S-awaiting-review +S-needs-code-changes

Previously, jdm (Josh Matthews) wrote…

Sorry for the delay here; someone else should take this over from me.

r? @nox

Reviewed 22 of 22 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 21 unresolved discussions.

a discussion (no related file):
The code that you can't find executed anywhere is used for optional arguments without a default value. Don't we cover these in TestBinding?

components/script/dom/crypto.rs, line 46 [r1] (raw file):

                       _cx: *mut JSContext,
                       input: *mut JSObject)
                       -> Fallible<*mut JSObject> {

Nit: reindent.

components/script/dom/customevent.rs, line 91 [r1] (raw file):

                       can_bubble: bool,
                       cancelable: bool,
                       detail: HandleValue) {

Nit: reindent.

components/script/dom/eventtarget.rs, line 163 [r1] (raw file):

                            let error = unsafe {
                                RootedValue::new(cx, event.Error(cx))
                            };

RootedValue::new is not really unsafe yet unfortunately, right? Revert that.

components/script/dom/htmlcanvaselement.rs, line 239 [r1] (raw file):

                  id: DOMString,
                  attributes: Vec<HandleValue>)
        -> Option<CanvasRenderingContext2DOrWebGLRenderingContext> {

Nit: reindent.

components/script/dom/htmlcanvaselement.rs, line 258 [r1] (raw file):

                 _context: *mut JSContext,
                 _mime_type: Option<DOMString>,
                 _arguments: Vec<HandleValue>) -> Fallible<DOMString> {

Nit: reindent.

components/script/dom/textdecoder.rs, line 82 [r1] (raw file):

    // https://encoding.spec.whatwg.org/#dom-textdecoder-decode
    unsafe fn Decode(&self, _cx: *mut JSContext, input: Option<*mut JSObject>)
              -> Fallible<USVString> {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 833 [r1] (raw file):

    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.8
    unsafe fn CompressedTexImage2D(&self, _cx: *mut JSContext, _target: u32, _level: i32, _internal_format: u32,
                            _width: i32, _height: i32, _border: i32, _pixels: *mut JSObject) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 843 [r1] (raw file):

    unsafe fn CompressedTexSubImage2D(&self, _cx: *mut JSContext, _target: u32, _level: i32,
                               _xoffset: i32, _yoffset: i32, _width: i32, _height: i32,
                               _format: u32, _pixels: *mut JSObject) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1432 [r1] (raw file):

    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.12
    unsafe fn ReadPixels(&self, _cx: *mut JSContext, x: i32, y: i32, width: i32, height: i32,
                  format: u32, pixel_type: u32, pixels: *mut JSObject) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1593 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1607 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1632 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1661 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1690 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1719 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1749 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1778 [r1] (raw file):

                  _cx: *mut JSContext,
                  uniform: Option<&WebGLUniformLocation>,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/webglrenderingcontext.rs, line 1906 [r1] (raw file):

                  format: u32,
                  data_type: u32,
                  data: Option<*mut JSObject>) {

Nit: reindent.

components/script/dom/bindings/codegen/CodegenRust.py, line 2145 [r1] (raw file):

    an 'unsafe fn()' declaration.

    unsafe is used to wrap the body of a function in an unsafe block.

What if we just marked functions with unsafe=True as unsafe themselves, instead of just wrapping their body?

components/script/dom/bindings/codegen/CodegenRust.py, line 5037 [r1] (raw file):

            if not arguments or len(arguments) == 0:
                return False
            return reduce((lambda x, y: x or y[1] == '*mut JSContext'), arguments, False)

Could you instead find where that *mut JSContext argument is added and set unsafe_fn there?

Comments from Reviewable

ab22 · 2016-06-09T01:43:02Z

Review status: all files reviewed at latest revision, 22 unresolved discussions.

a discussion (no related file):

Previously, nox (Anthony Ramine) wrote…

The code that you can't find executed anywhere is used for optional arguments without a default value. Don't we cover these in TestBinding?

I will take a look at `TestBinding`!

components/script/dom/bindings/codegen/CodegenRust.py, line 2145 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

What if we just marked functions with unsafe=True as unsafe themselves, instead of just wrapping their body?

Agree. The 'unsafe' param was in the script before this change, so I added an 'unsafe_fn' parameter and the missing doc for the 'unsafe' param. I thought both could be needed in the future. Should I just leave one of those?

components/script/dom/bindings/codegen/CodegenRust.py, line 4976 [r1] (raw file):

        def attribute_arguments(needCx, argument=None):
            if needCx:
                yield "cx", "*mut JSContext"

@nox This is one of the two generator functions used, and this is one of the places where the extra 'unsafe' string could be returned.

components/script/dom/bindings/codegen/CodegenRust.py, line 5037 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Could you instead find where that *mut JSContext argument is added and set unsafe_fn there?

Well.. the function is added right below this code:

methods.append(CGGeneric("%sfn %s(&self%s) -> %s;\n" % (
                'unsafe ' if contains_unsafe_arg(arguments) else '',
                name, fmt(arguments), rettype))
)

These all use a generator to return the parameters. I could always return an extra unsafe string from the generators. Thoughts?

Comments from Reviewable

bors-servo · 2016-06-10T01:37:48Z

☔ The latest upstream changes (presumably #11168) made this pull request unmergeable. Please resolve the merge conflicts.

ab22 · 2016-06-17T18:22:14Z

Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

a discussion (no related file):

Previously, ab22 (Abelardo Mendoza) wrote…

I will take a look at TestBinding!

@nox Sorry for the delay. I have the changes locally at the moment but I have a few comments on what you asked for:

RootedValue::new is not really unsafe yet unfortunately, right? Revert that.

let error = unsafe {
    RootedValue::new(cx, event.Error(cx))
};

The RootedValue::new is not unsafe but event.Error is, so it now looks like:

let event_error = unsafe {
    event.Error(cx)
};
let error = RootedValue::new(cx, event_error);

What if we just marked functions with unsafe=True as unsafe themselves, instead of just wrapping their body?
This can be done but some other code makes use of the unsafe parameter to declare it's body as unsafe instead of declaring the function as unsafe such as https://github.com/servo/servo/blob/master/components/script/lib.rs#L162-L164

If you still would prefer it to just have 1 unsafe parameter I would just have to insert an unsafe block on to RegisterBindings::RegisterProxyHandlers(); and that should be it.

*Could you instead find where that mut JSContext argument is added and set unsafe_fn there?
These functions are just generators that return the name and the type of the variable, then they are added like:

methods.append(CGGeneric("%sfn %s(&self%s) -> %s;\n" % (
                'unsafe ' if contains_unsafe_arg(arguments) else '',
                name, fmt(arguments), rettype))
)

I'm still not sure if what you want is for me to change the CGGeneric to a CGAbstractMethod and make use of the unsafe_fn parameter, and even so, I would have to create a new class to define definition_body right?

As a side note, I have reindented and rebased the branch. Let me know if I'm just completely lost with what I've just said.

Comments from Reviewable

ab22 · 2016-07-01T01:49:07Z

@jdm @nox Could I get some feedback here guys, please?

nox · 2016-07-02T12:33:26Z

Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

a discussion (no related file):

Previously, ab22 (Abelardo Mendoza) wrote…

@nox Sorry for the delay. I have the changes locally at the moment but I have a few comments on what you asked for:

RootedValue::new is not really unsafe yet unfortunately, right? Revert that.
let error = unsafe {
    RootedValue::new(cx, event.Error(cx))
};
The RootedValue::new is not unsafe but event.Error is, so it now looks like:
let event_error = unsafe {
    event.Error(cx)
};
let error = RootedValue::new(cx, event_error);
What if we just marked functions with unsafe=True as unsafe themselves, instead of just wrapping their body?
This can be done but some other code makes use of the unsafe parameter to declare it's body as unsafe instead of declaring the function as unsafe such as https://github.com/servo/servo/blob/master/components/script/lib.rs#L162-L164

If you still would prefer it to just have 1 unsafe parameter I would just have to insert an unsafe block on to RegisterBindings::RegisterProxyHandlers(); and that should be it.

*Could you instead find where that mut JSContext argument is added and set unsafe_fn there?
These functions are just generators that return the name and the type of the variable, then they are added like:
methods.append(CGGeneric("%sfn %s(&self%s) -> %s;\n" % (
                'unsafe ' if contains_unsafe_arg(arguments) else '',
                name, fmt(arguments), rettype))
)
I'm still not sure if what you want is for me to change the CGGeneric to a CGAbstractMethod and make use of the unsafe_fn parameter, and even so, I would have to create a new class to define definition_body right?

As a side note, I have reindented and rebased the branch. Let me know if I'm just completely lost with what I've just said.

AFAIK, any function which uses the `unsafe` flag to just put the whole body in an unsafe block should actually be unsafe themselves, that's why I asked "What if we just marked functions with unsafe=True as unsafe themselves, instead of just wrapping their body?".

Comments from Reviewable

KiChjang · 2016-09-14T09:40:37Z

@ab22 Are you still working on this?

ab22 · 2016-09-15T22:51:58Z

@KiChjang go ahead and assign someone else. I'm having a hard time finding spare time to work on this.

fflorent · 2016-10-02T08:21:48Z

components/script/dom/imagedata.rs

    // https://html.spec.whatwg.org/multipage/#dom-imagedata-data
-    fn Data(&self, _: *mut JSContext) -> *mut JSObject {
+    unsafe fn Data(&self, _: *mut JSContext) -> *mut JSObject {


The context looks to be ignored here (and also in other places). Should we make this constructor unsafe anyway?

This is a method generated by codegen, so yes it needs to be unsafe, because codegen cannot know that the implementation may be safe.


        Auto merge of #14096 - fflorent:11485-make-dom-methods-taking-mut-JSC…

…ontent-unsafe, r=nox 11485 make dom methods taking mut js content unsafe This is a rebased version of PR #11595 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11485  - [ ] There are tests for these changes OR - [x] These changes do not require tests because: no code logic was changed   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14096)

highfive assigned jdm Jun 3, 2016

highfive added the S-awaiting-review label Jun 3, 2016

script creates methods taking '*mut JSContext' unsafe

Loading status checks…

0c6b26a

rebase + marked the necessary new code as unsafe

ab22 force-pushed the ab22:11485-make-dom-methods-taking-mut-JSContent-unsafe branch from 4e586ab to 0c6b26a Jun 4, 2016

highfive assigned nox and unassigned jdm Jun 7, 2016

nox added S-needs-code-changes and removed S-awaiting-review labels Jun 8, 2016

highfive added the S-needs-rebase label Jun 10, 2016

KiChjang mentioned this pull request Sep 15, 2016

Make DOM methods taking a `*mut JSContext` unsafe #11485

Closed

KiChjang closed this Sep 15, 2016

fflorent reviewed Oct 2, 2016

View changes

fflorent mentioned this pull request Nov 6, 2016

11485 make dom methods taking mut js content unsafe #14096

Merged

4 of 5 tasks complete

servo / servo

script creates methods taking '*mut JSContext' unsafe #11595

script creates methods taking '*mut JSContext' unsafe #11595

ab22 commented Jun 3, 2016 •

edited

highfive commented Jun 3, 2016

highfive commented Jun 3, 2016

ab22 commented Jun 3, 2016

highfive commented Jun 4, 2016

jdm commented Jun 7, 2016

nox commented Jun 8, 2016

ab22 commented Jun 9, 2016

bors-servo commented Jun 10, 2016

ab22 commented Jun 17, 2016

ab22 commented Jul 1, 2016

nox commented Jul 2, 2016

KiChjang commented Sep 14, 2016

ab22 commented Sep 15, 2016

This comment has been minimized.

This comment has been minimized.

servo / servo

Join GitHub today

script creates methods taking '*mut JSContext' unsafe #11595

script creates methods taking '*mut JSContext' unsafe #11595

Conversation

ab22 commented Jun 3, 2016 • edited

highfive commented Jun 3, 2016

highfive commented Jun 3, 2016

ab22 commented Jun 3, 2016

highfive commented Jun 4, 2016

jdm commented Jun 7, 2016

nox commented Jun 8, 2016

ab22 commented Jun 9, 2016

bors-servo commented Jun 10, 2016

ab22 commented Jun 17, 2016

ab22 commented Jul 1, 2016

nox commented Jul 2, 2016

KiChjang commented Sep 14, 2016

ab22 commented Sep 15, 2016

This comment has been minimized.

fflorent Oct 2, 2016

This comment has been minimized.

nox Oct 2, 2016

ab22 commented Jun 3, 2016 •

edited