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

Fix prototypes of interface objects (fixes #2665) #8954

Merged
merged 8 commits into from
Jan 12, 2016

Conversation

nox
Copy link
Contributor

@nox nox commented Dec 13, 2015

Callback interface objects' (i.e. NodeFilter's) prototype is now Object instead of
Function and non-callback interface objects' their proper ancestor, starting with
the Function prototype.

The function do_create_interface_objects is removed in favour of 4 functions:
create_callback_interface_object, create_interface_prototype_object,
create_noncallback_interface_object and create_named_constructors.

While this increases the amount of codegen'd code, this greatly improves the
readability of the code involved in this part of DOM, instead of having one function
doing 4 different things. We can always find a more adequate abstraction later.

NativeProperties and everything related to the interface objects have been removed
from the utils module.

Fixes #2665.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 13, 2015
@highfive
Copy link

warning Warning warning

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

@nox
Copy link
Contributor Author

nox commented Dec 13, 2015

r? @Ms2ger

@nox nox added A-content/dom Interacting with the DOM from web content A-content/bindings The DOM bindings labels Dec 13, 2015
@nox nox changed the title Fix prototypes of interface objects Fix prototypes of interface objects (fixes #2665) Dec 13, 2015
@nox
Copy link
Contributor Author

nox commented Dec 13, 2015

For Document, we end up with:

fn CreateInterfaceObjects(cx: *mut JSContext, global: HandleObject, cache: *mut ProtoOrIfaceArray, receiver: HandleObject) {
    unsafe {
        let mut prototype_proto = RootedObject::new(cx, ptr::null_mut());
        NodeBinding::GetProtoObject(cx, global, receiver, prototype_proto.handle_mut());
        assert!(!prototype_proto.ptr.is_null());

        let mut prototype = RootedObject::new(cx, ptr::null_mut());
        create_interface_prototype_object(cx,
                                          prototype_proto.handle(),
                                          &PrototypeClass,
                                          Some(sMethods),
                                          Some(sAttributes),
                                          &[],
                                          prototype.handle_mut());
        assert!(!prototype.ptr.is_null());
        (*cache)[PrototypeList::ID::Document as usize] = prototype.ptr;
        if <*mut JSObject>::needs_post_barrier(prototype.ptr) {
            <*mut JSObject>::post_barrier((*cache).as_mut_ptr().offset(PrototypeList::ID::Document as isize));
        }

        let mut interface_proto = RootedObject::new(cx, ptr::null_mut());
        NodeBinding::GetConstructorObject(cx, global, receiver, interface_proto.handle_mut());
        assert!(!interface_proto.ptr.is_null());

        let mut interface = RootedObject::new(cx, ptr::null_mut());
        create_noncallback_interface_object(cx,
                                            receiver,
                                            interface_proto.handle(),
                                            &InterfaceObjectClass,
                                            None,
                                            None,
                                            &[],
                                            prototype.handle(),
                                            b"Document\0",
                                            0,
                                            interface.handle_mut());
        assert!(!interface.ptr.is_null());
        (*cache)[PrototypeList::Constructor::Document as usize] = interface.ptr;
        if <*mut JSObject>::needs_post_barrier(prototype.ptr) {
            <*mut JSObject>::post_barrier((*cache).as_mut_ptr().offset(PrototypeList::Constructor::Document as isize));
        }

        let mut unforgeable_holder = RootedObject::new(cx, ptr::null_mut());
        unforgeable_holder.handle_mut().set(
            JS_NewObjectWithoutMetadata(cx, ptr::null(), HandleObject::null()));
        assert!(!unforgeable_holder.ptr.is_null());

        define_properties(cx, unforgeable_holder.handle(), sUnforgeableAttributes).unwrap();
        JS_SetReservedSlot(prototype.ptr, DOM_PROTO_UNFORGEABLE_HOLDER_SLOT,
                           ObjectValue(&*unforgeable_holder.ptr))
    }
}

@frewsxcv
Copy link
Contributor

Fails tidy

@frewsxcv frewsxcv added the S-fails-tidy `./mach test-tidy` reported errors. label Dec 13, 2015
@nox nox removed the S-fails-tidy `./mach test-tidy` reported errors. label Dec 13, 2015
@bors-servo
Copy link
Contributor

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

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Dec 16, 2015
@nox nox removed the S-needs-rebase There are merge conflict errors. label Dec 16, 2015
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Dec 18, 2015
@nox nox removed the S-needs-rebase There are merge conflict errors. label Dec 18, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 18, 2015

I'd still like to try and get the hasInstance stuff into a separate commit. Will try to look tomorrow.


Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion.


components/script/dom/bindings/interface.rs, line 173 [r2] (raw file):
I think define_on_global_object would be clearer.


Comments from the review on Reviewable.io

@nox
Copy link
Contributor Author

nox commented Dec 18, 2015

I've just rebuilt on d81fe2f with the hasInstance hook disabled (i.e. set to None):

± ./mach test-wpt --no-pause /dom/interfaces.html
Running 1 tests in web-platform-tests

  ▶ Unexpected subtest result in /dom/interfaces.html:
  │ FAIL [expected PASS] Document interface: xmlDoc must inherit property "implementation" with the proper type (0)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Document interface: xmlDoc must inherit property "children" with the proper type (29)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: xmlDoc must inherit property "childNodes" with the proper type (19)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] DocumentFragment interface: document.createDocumentFragment() must inherit property "children" with the proper type (1)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.createDocumentFragment() must inherit property "ownerDocument" with the proper type (15)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.createDocumentFragment() must inherit property "childNodes" with the proper type (19)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.doctype must inherit property "ownerDocument" with the proper type (15)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.doctype must inherit property "parentNode" with the proper type (16)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.doctype must inherit property "childNodes" with the proper type (19)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.doctype must inherit property "nextSibling" with the proper type (23)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Element interface: element must inherit property "classList" with the proper type (6)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Element interface: element must inherit property "attributes" with the proper type (8)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Element interface: element must inherit property "children" with the proper type (27)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: element must inherit property "ownerDocument" with the proper type (15)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: element must inherit property "childNodes" with the proper type (19)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Attr interface: document.querySelector("[id]").attributes[0] must inherit property "ownerElement" with the proper type (7)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.createTextNode("abc") must inherit property "ownerDocument" with the proper type (15)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.createTextNode("abc") must inherit property "childNodes" with the proper type (19)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: xmlDoc.createProcessingInstruction("abc", "def") must inherit property "ownerDocument" with the proper type (15)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: xmlDoc.createProcessingInstruction("abc", "def") must inherit property "childNodes" with the proper type (19)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.createComment("abc") must inherit property "ownerDocument" with the proper type (15)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Node interface: document.createComment("abc") must inherit property "childNodes" with the proper type (19)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Range interface: document.createRange() must inherit property "startContainer" with the proper type (0)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Range interface: document.createRange() must inherit property "endContainer" with the proper type (2)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Range interface: document.createRange() must inherit property "commonAncestorContainer" with the proper type (5)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Range interface: detachedRange must inherit property "startContainer" with the proper type (0)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Range interface: detachedRange must inherit property "endContainer" with the proper type (2)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] Range interface: detachedRange must inherit property "commonAncestorContainer" with the proper type (5)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] NodeIterator interface: document.createNodeIterator(document.body, NodeFilter.SHOW_ALL, null, false) must inherit property "root" with the proper type (0)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] NodeIterator interface: document.createNodeIterator(document.body, NodeFilter.SHOW_ALL, null, false) must inherit property "referenceNode" with the proper type (1)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] TreeWalker interface: document.createTreeWalker(document.body, NodeFilter.SHOW_ALL, null, false) must inherit property "root" with the proper type (0)
  │   → invalid 'instanceof' operand self[type]
  │ FAIL [expected PASS] TreeWalker interface: document.createTreeWalker(document.body, NodeFilter.SHOW_ALL, null, false) must inherit property "currentNode" with the proper type (3)
  │   → invalid 'instanceof' operand self[type]
  │
  │ IdlArray.prototype.assert_type_is@http://web-platform.test:8000/resources/idlharness.js:468:13
  │ IdlInterface.prototype.test_interface_of/<@http://web-platform.test:8000/resources/idlharness.js:1447:29
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ IdlInterface.prototype.test_interface_of@http://web-platform.test:8000/resources/idlharness.js:1416:1
  │ IdlInterface.prototype.test_object@http://web-platform.test:8000/resources/idlharness.js:1329:9
  │ IdlArray.prototype.test/<@http://web-platform.test:8000/resources/idlharness.js:325:17
  │ IdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:323:13
  └ @http://web-platform.test:8000/dom/interfaces.html:33:1

Ran 1 tests finished in 2.0 seconds.
  • 0 ran as expected. 0 tests skipped.
  • 1 tests had unexpected subtest results

So this was actually already provided by the fact that create_constructor uses JS_NewFunction in the current code. That's why I believe it can't be moved to another commit without a regression.

Unverified

This user has not yet uploaded their public signing key.
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 12, 2016
@nox
Copy link
Contributor Author

nox commented Jan 12, 2016

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLFormControlsCollection interface: existence and properties of interface object

  ▶ Unexpected subtest result in /_mozilla/mozilla/documentElement.html:
  │ FAIL [expected PASS] Untitled
  │   → assert_true: Should be Window expected true got false
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/documentElement.html:2:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/documentElement.html:1:1

  ▶ Unexpected subtest result in /_mozilla/mozilla/window.html:
  │ FAIL [expected PASS] Untitled
  │   → assert_true: Should be Window expected true got false
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/window.html:3:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/window.html:1:1

@nox nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-rebase There are merge conflict errors. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 12, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jan 12, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 12, 2016

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit d13da7d has been approved by Ms2ger

@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. S-needs-rebase There are merge conflict errors. labels Jan 12, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit d13da7d with merge e977a6e...

bors-servo pushed a commit that referenced this pull request Jan 12, 2016
Fix prototypes of interface objects (fixes #2665)

Callback interface objects' (i.e. NodeFilter's) prototype is now Object instead of
Function and non-callback interface objects' their proper ancestor, starting with
the Function prototype.

The function do_create_interface_objects is removed in favour of 4 functions:
create_callback_interface_object, create_interface_prototype_object,
create_noncallback_interface_object and create_named_constructors.

While this increases the amount of codegen'd code, this greatly improves the
readability of the code involved in this part of DOM, instead of having one function
doing 4 different things. We can always find a more adequate abstraction later.

NativeProperties and everything related to the interface objects have been removed
from the utils module.

Fixes #2665.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8954)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit d13da7d into servo:master Jan 12, 2016
@nox nox deleted the protochain branch January 12, 2016 17:20
@jdm jdm removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/bindings The DOM bindings A-content/dom Interacting with the DOM from web content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants