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

[WIP] script: Partially implement the window named getter. #19904

Closed
wants to merge 3 commits into from

Conversation

@emilio
Copy link
Member

emilio commented Jan 30, 2018

There's tons of layout tests that depend on this, and I keep forgetting Servo
doesn't support it :)


This change is Reviewable

@highfive
Copy link

highfive commented Jan 30, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/document.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script/dom/element.rs
  • @fitzgen: components/script/dom/document.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script/dom/element.rs
  • @KiChjang: components/script/dom/document.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script/dom/element.rs
@highfive
Copy link

highfive commented Jan 30, 2018

warning Warning 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!
@emilio
Copy link
Member Author

emilio commented Jan 30, 2018

r? @jdm

This fails at codegen in the call to create_global_object, expecting an (unexisting) &'static JSClass. I have absolutely no idea about what the right thing to do is there.

Gecko generates a JSClass: https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/WindowBinding.cpp#16292

While with this change we only generate a DOMClass.

Diff between old and new bindings:

diff --git a/wb-old.rs b/wb-new.rs
index 0a89b24f5c..807e410636 100644
--- a/wb-old.rs
+++ b/wb-new.rs
@@ -8864,37 +8864,199 @@ unsafe extern fn _trace(trc: *mut JSTracer, obj: *mut JSObject) {
     }), ());
 }
 
-static CLASS_OPS: js::jsapi::JSClassOps = js::jsapi::JSClassOps {
-    addProperty: None,
-    delProperty: None,
-    getProperty: None,
-    setProperty: None,
-    enumerate: Some(enumerate_global),
-    resolve: Some(resolve_global),
-    mayResolve: None,
-    finalize: Some(_finalize),
-    call: None,
-    hasInstance: None,
-    construct: None,
-    trace: Some(js::jsapi::JS_GlobalObjectTraceHook),
-};
-
-static Class: DOMJSClass = DOMJSClass {
-    base: js::jsapi::JSClass {
-        name: b"Window\0" as *const u8 as *const libc::c_char,
-        flags: JSCLASS_IS_DOMJSCLASS | JSCLASS_IS_GLOBAL | JSCLASS_DOM_GLOBAL |
-               (((JSCLASS_GLOBAL_SLOT_COUNT + 1) & JSCLASS_RESERVED_SLOTS_MASK) << JSCLASS_RESERVED_SLOTS_SHIFT)
-               /* JSCLASS_HAS_RESERVED_SLOTS(JSCLASS_GLOBAL_SLOT_COUNT + 1) */,
-        cOps: &CLASS_OPS,
-        reserved: [0 as *mut _; 3],
-    },
-    dom_class: DOMClass {
+pub unsafe fn DefineProxyHandler() -> *const libc::c_void {
+    let traps = ProxyTraps {
+        enter: None,
+        getOwnPropertyDescriptor: Some(getOwnPropertyDescriptor),
+        defineProperty: Some(proxyhandler::define_property),
+        ownPropertyKeys: Some(own_property_keys),
+        delete_: Some(proxyhandler::delete),
+        enumerate: None,
+        getPrototypeIfOrdinary: Some(proxyhandler::get_prototype_if_ordinary),
+        preventExtensions: Some(proxyhandler::prevent_extensions),
+        isExtensible: Some(proxyhandler::is_extensible),
+        has: None,
+        get: Some(get),
+        set: None,
+        call: None,
+        construct: None,
+        getPropertyDescriptor: Some(get_property_descriptor),
+        hasOwn: Some(hasOwn),
+        getOwnEnumerablePropertyKeys: Some(own_property_keys),
+        nativeCall: None,
+        hasInstance: None,
+        objectClassIs: None,
+        className: Some(className),
+        fun_toString: None,
+        boxedValue_unbox: None,
+        defaultValue: None,
+        trace: Some(_trace),
+        finalize: Some(_finalize),
+        objectMoved: None,
+        isCallable: None,
+        isConstructor: None,
+    };
+
+    CreateProxyHandler(&traps, Class.as_void_ptr())
+}
+
+#[inline] unsafe fn UnwrapProxy(obj: HandleObject) -> *const Window {
+    /*if (xpc::WrapperFactory::IsXrayWrapper(obj)) {
+        obj = js::UnwrapObject(obj);
+    }*/
+    //MOZ_ASSERT(IsProxy(obj));
+    let box_ = GetProxyPrivate(obj.get()).to_private() as *const Window;
+    return box_;
+}
+
+static Class: DOMClass = DOMClass {
     interface_chain: [ PrototypeList::ID::EventTarget, PrototypeList::ID::GlobalScope, PrototypeList::ID::Window, PrototypeList::ID::Last, PrototypeList::ID::Last, PrototypeList::ID::Last ],
     type_id: ::dom::bindings::codegen::InheritTypes::TopTypeId { eventtarget: (::dom::bindings::codegen::InheritTypes::EventTargetTypeId::GlobalScope(::dom::bindings::codegen::InheritTypes::GlobalScopeTypeId::Window)) },
     malloc_size_of: malloc_size_of_including_raw_self::<Window> as unsafe fn(&mut _, _) -> _,
     global: InterfaceObjectMap::Globals::WINDOW,
-}
 };
+
+unsafe extern fn own_property_keys(cx: *mut JSContext, proxy: HandleObject, props: *mut AutoIdVector) -> bool {
+    return wrap_panic(panic::AssertUnwindSafe(|| {
+        let unwrapped_proxy = UnwrapProxy(proxy);
+        for name in (*unwrapped_proxy).SupportedPropertyNames() {
+            let cstring = CString::new(name).unwrap();
+            let jsstring = JS_AtomizeAndPinString(cx, cstring.as_ptr());
+            rooted!(in(cx) let rooted = jsstring);
+            let jsid = INTERNED_STRING_TO_JSID(cx, rooted.handle().get());
+            rooted!(in(cx) let rooted_jsid = jsid);
+            AppendToAutoIdVector(props, rooted_jsid.handle().get());
+        }
+        rooted!(in(cx) let mut expando = ptr::null_mut::<JSObject>());
+        get_expando_object(proxy, expando.handle_mut());
+        if !expando.is_null() {
+            GetPropertyKeys(cx, expando.handle(), JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS, props);
+        }
+
+        return true;
+
+    }), false);
+}
+
+unsafe extern fn getOwnPropertyDescriptor(cx: *mut JSContext, proxy: HandleObject, id: HandleId, desc: MutableHandle<PropertyDescriptor>) -> bool {
+    return wrap_panic(panic::AssertUnwindSafe(|| {
+        rooted!(in(cx) let mut expando = ptr::null_mut::<JSObject>());
+        get_expando_object(proxy, expando.handle_mut());
+        //if (!xpc::WrapperFactory::IsXrayWrapper(proxy) && (expando = GetExpandoObject(proxy))) {
+        if !expando.is_null() {
+            if !JS_GetPropertyDescriptorById(cx, expando.handle(), id, desc) {
+                return false;
+            }
+            if !desc.obj.is_null() {
+                // Pretend the property lives on the wrapper.
+                desc.get().obj = proxy.get();
+                return true;
+            }
+        }
+
+        if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) {
+            let mut has_on_proto = false;
+            if !has_property_on_prototype(cx, proxy, id, &mut has_on_proto) {
+                return false;
+            }
+            if !has_on_proto {
+                        let name = jsid_to_string(cx, id).expect("Not a string-convertible JSID?");
+                let this = UnwrapProxy(proxy);
+                let this = &*this;
+                let result: Option<NonNull<JSObject>> = this.NamedGetter(cx, name);
+
+                if let Some(result) = result {
+                    rooted!(in(cx) let mut result_root = UndefinedValue());
+                    (result).to_jsval(cx, result_root.handle_mut());
+                    desc.get().value = result_root.get();
+                    fill_property_descriptor(desc, proxy.get(), JSPROP_ENUMERATE | JSPROP_READONLY);
+                    return true;
+                }
+            }
+        }
+        desc.get().obj = ptr::null_mut();
+        return true;
+    }), false);
+}
+
+unsafe extern fn className(cx: *mut JSContext, _proxy: HandleObject) -> *const i8 {
+    b"Window\0" as *const u8 as *const i8
+}
+
+unsafe extern fn get(cx: *mut JSContext, proxy: HandleObject, receiver: HandleValue, id: HandleId, vp: MutableHandleValue) -> bool {
+    return wrap_panic(panic::AssertUnwindSafe(|| {
+        //MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),
+        //"Should not have a XrayWrapper here");
+
+        rooted!(in(cx) let mut expando = ptr::null_mut::<JSObject>());
+        get_expando_object(proxy, expando.handle_mut());
+        if !expando.is_null() {
+            let mut hasProp = false;
+            if !JS_HasPropertyById(cx, expando.handle(), id, &mut hasProp) {
+                return false;
+            }
+
+            if hasProp {
+                return JS_ForwardGetPropertyTo(cx, expando.handle(), id, receiver, vp);
+            }
+        }
+
+        let mut found = false;
+        if !get_property_on_prototype(cx, proxy, receiver, id, &mut found, vp) {
+            return false;
+        }
+
+        if found {
+            return true;
+        }
+        if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) {
+            let name = jsid_to_string(cx, id).expect("Not a string-convertible JSID?");
+            let this = UnwrapProxy(proxy);
+            let this = &*this;
+            let result: Option<NonNull<JSObject>> = this.NamedGetter(cx, name);
+
+            if let Some(result) = result {
+
+                (result).to_jsval(cx, vp);
+                return true;
+            }}
+
+        vp.set(UndefinedValue());
+        return true;
+    }), false);
+}
+
+unsafe extern fn hasOwn(cx: *mut JSContext, proxy: HandleObject, id: HandleId, bp: *mut bool) -> bool {
+    return wrap_panic(panic::AssertUnwindSafe(|| {
+        rooted!(in(cx) let mut expando = ptr::null_mut::<JSObject>());
+        get_expando_object(proxy, expando.handle_mut());
+        if !expando.is_null() {
+            let ok = JS_HasPropertyById(cx, expando.handle(), id, bp);
+            if !ok || *bp {
+                return ok;
+            }
+        }
+        if RUST_JSID_IS_STRING(id) || RUST_JSID_IS_INT(id) {
+            let mut has_on_proto = false;
+            if !has_property_on_prototype(cx, proxy, id, &mut has_on_proto) {
+                return false;
+            }
+            if !has_on_proto {
+                        let name = jsid_to_string(cx, id).expect("Not a string-convertible JSID?");
+                let this = UnwrapProxy(proxy);
+                let this = &*this;
+                let result: Option<NonNull<JSObject>> = this.NamedGetter(cx, name);
+
+                *bp = result.is_some();
+                return true;
+            }
+        }
+
+        *bp = false;
+        return true;
+    }), false);
+}
+
 pub unsafe fn Wrap(cx: *mut JSContext, object: Box<Window>) -> DomRoot<Window> {
     let raw = Box::into_raw(object);
     let _rt = RootedTraceable::new(&*raw);
@@ -8921,10 +9083,12 @@ pub unsafe fn Wrap(cx: *mut JSContext, object: Box<Window>) -> DomRoot<Window> {
     define_guarded_properties(cx, obj.handle(), sAttributes);
     define_guarded_methods(cx, obj.handle(), sMethods);
 
+    rooted!(in(cx) let mut expando = ptr::null_mut::<JSObject>());
+    ensure_expando_object(cx, obj.handle(), expando.handle_mut());
     rooted!(in(cx) let mut unforgeable_holder = ptr::null_mut::<JSObject>());
     unforgeable_holder.handle_mut().set(
         JS_GetReservedSlot(proto.get(), DOM_PROTO_UNFORGEABLE_HOLDER_SLOT).to_object());
-    assert!(JS_CopyPropertiesFrom(cx, obj.handle(), unforgeable_holder.handle()));
+    assert!(JS_CopyPropertiesFrom(cx, expando.handle(), unforgeable_holder.handle()));
 
 
     DomRoot::from_ref(&*raw)
@@ -8933,7 +9097,7 @@ pub unsafe fn Wrap(cx: *mut JSContext, object: Box<Window>) -> DomRoot<Window> {
 impl IDLInterface for Window {
     #[inline]
     fn derives(class: &'static DOMClass) -> bool {
-        class as *const _ == &Class.dom_class as *const _
+        class as *const _ == &Class as *const _
     }
 }
 
@@ -9177,6 +9341,8 @@ pub trait WindowMethods {
     unsafe fn SetInterval(&self, cx: *mut JSContext, handler: Rc<Function>, timeout: i32, arguments: Vec<HandleValue>) -> i32;
     unsafe fn SetInterval_(&self, cx: *mut JSContext, handler: DOMString, timeout: i32, arguments: Vec<HandleValue>) -> i32;
     fn ClearInterval(&self, handle: i32) -> ();
+    fn SupportedPropertyNames(&self) -> Vec<DOMString>;
+    unsafe fn NamedGetter(&self, cx: *mut JSContext, name: DOMString) -> Option<NonNull<JSObject>>;
 }
 const sMethods_specs: &'static [&'static[JSFunctionSpec]] = &[
 &[
@highfive highfive assigned jdm and unassigned Manishearth Jan 30, 2018
@emilio
Copy link
Member Author

emilio commented Jan 30, 2018

@bors-servo try

  • Worth a shot
bors-servo added a commit that referenced this pull request Jan 30, 2018
[WIP] script: Partially implement the window named getter.

There's tons of layout tests that depend on this, and I keep forgetting Servo
doesn't support it :)

<!-- 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/19904)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 30, 2018

Trying commit feeb833 with merge a86b2f2...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 30, 2018

💔 Test failed - linux-dev

@emilio
Copy link
Member Author

emilio commented Jan 30, 2018

Ok, so that's clearly wrong. Just so this doesn't get lost on IRC:

17:10 <~jdm> https://searchfox.org/mozilla-central/source/dom/bindings/Configuration.py#465-468
17:10 <~jdm> gecko uses a separate object as the prototype of the global for this case
17:10 <~jdm> https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/WindowBinding.cpp#16217
17:10 <~jdm> https://searchfox.org/mozilla-central/source/dom/base/WindowNamedPropertiesHandler.cpp#257
17:11 <~jdm> "global scope polluter" is the other key term here
17:11 <nox> Well,
17:11 <nox> the spec does that, no?
17:11 <nox> jdm, emilio: 'For every interface declared with the [Global] extended attribute that supports named properties, there must exist an object known as the named properties object for that interface on which named properties are exposed.'
17:11 <~jdm> mhm
17:11 <nox> https://heycam.github.io/webidl/#named-properties-object
17:12 <nox> It ends up on the prototype chain of the global.
17:13 <nox> See https://heycam.github.io/webidl/#ref-for-dfn-interface-prototype-object①③
17:13 <nox> That's one fancy anchor.
emilio added 3 commits Jan 30, 2018
There's tons of layout tests that depend on this, and I keep forgetting Servo
doesn't support it :)
@emilio emilio force-pushed the emilio:window-named-getter branch from feeb833 to c098d21 Feb 4, 2018
@emilio
Copy link
Member Author

emilio commented Feb 4, 2018

I tried to write the appropriate hook into the bindings. @jdm does the last commit look reasonable?

@@ -267,8 +267,10 @@ def addOperation(operation, m):
continue

def addIndexedOrNamedOperation(operation, m):
self.proxy = True
if not self.isGlobal():
self.proxy = True

This comment has been minimized.

Copy link
@jdm

jdm Feb 6, 2018

Member

self.proxy = not self.isGlobal()

@@ -5523,7 +5537,7 @@ def members():
rettype = "ErrorResult"
yield name, attribute_arguments(typeNeedsCx(m.type, False), m.type), rettype

if descriptor.proxy:
if descriptor.proxy or descriptor.hasNamedPropertiesObject():

This comment has been minimized.

Copy link
@jdm

jdm Feb 6, 2018

Member

I'm on the fence about this, because the SupportedPropertyNames and NamedGetter methods won't actually be hooked up to anything automatically. Is the idea that the named properties proxy object would grab the window object and invoke those methods? I think it would be less confusing if we did not reuse this machinery in a way that's different than how it's used for proxy objects.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 13, 2018

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

@nox
Copy link
Member

nox commented Aug 31, 2018

I found some WebGL tests which need this.

bors-servo added a commit that referenced this pull request Oct 4, 2018
Window getter

This is a rebase of #19904 with an attempt at porting Gecko's named property getter support to Servo on top of it. It's very rough but I want to see what effect it has on tests so far.
bors-servo added a commit that referenced this pull request Oct 4, 2018
[WIP] Named window getter

This is a rebase of #19904 with an attempt at porting Gecko's named property getter support to Servo on top of it. It's very rough but I want to see what effect it has on tests so far.

<!-- 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/21869)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Oct 4, 2018
[WIP] Named window getter

This is a rebase of #19904 with an attempt at porting Gecko's named property getter support to Servo on top of it. It's very rough but I want to see what effect it has on tests so far.

<!-- 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/21869)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Oct 5, 2018
[WIP] Named window getter

This is a rebase of #19904 with an attempt at porting Gecko's named property getter support to Servo on top of it. It's very rough but I want to see what effect it has on tests so far.

<!-- 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/21869)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Oct 5, 2018

Closing in favour of #21869.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.