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 smup #17143

Closed
wants to merge 1 commit into from
Closed

WIP smup #17143

wants to merge 1 commit into from

Conversation

@fitzgen
Copy link
Member

fitzgen commented Jun 2, 2017

So this compiles and stuff, but not yet passing tests.

Here are the results of the DOM wpt, which I haven't started digging into yet:

Ran 285 tests finished in 104.0 seconds.
  • 21 ran as expected. 15 tests skipped.
  • 31 tests crashed unexpectedly
  • 233 tests timed out unexpectedly

This change is Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2017

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

@jdm jdm self-assigned this Jun 5, 2017
@fitzgen
Copy link
Member Author

fitzgen commented Jun 6, 2017

Fixed some issues in CodegenRust.py causing bad *const jsapi::JSJitInfo pointers; now seeing some MOZ_CRASHes inside spidermonkey.

@fitzgen
Copy link
Member Author

fitzgen commented Jun 8, 2017

Ok, fixed some more things to do with proxies, which upstream SpiderMonkey had changed where expandos and private objects are stored and I got subtly wrong when rebasing.

New dom WPT results are pretty close to passing:

Ran 285 tests finished in 81.0 seconds.
  • 248 ran as expected. 15 tests skipped.
  • 7 tests crashed unexpectedly
  • 30 tests timed out unexpectedly
@fitzgen
Copy link
Member Author

fitzgen commented Jun 16, 2017

No more crashes!

Ran 285 tests finished in 79.0 seconds.
  • 272 ran as expected. 15 tests skipped.
  • 13 tests timed out unexpectedly
  • 7 tests had unexpected subtest results
@fitzgen
Copy link
Member Author

fitzgen commented Jun 16, 2017

Seems like the timeouts and unexpected results were because I was using a debug + debugmozjs build \o/

Ran 285 tests finished in 26.0 seconds.
  • 285 ran as expected. 15 tests skipped.
@fitzgen fitzgen force-pushed the fitzgen:smup branch 2 times, most recently from f4aefda to ecde05f Jun 22, 2017
@KiChjang
Copy link
Member

KiChjang commented Jun 23, 2017

Huh, autospider is requiring gcc 4.9 or newer, and travis CI uses 4.8.4...

@jdm
Copy link
Member

jdm commented Jun 23, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2017

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

Copy link
Member

jdm left a comment

All pretty minor changes that I've suggested.

pub fn leak_as_static<T>(t: T) -> &'static T {
let boxed = Box::new(t);
let ptr = &*boxed as *const T;
mem::forget(boxed);

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

let ptr = Box::into_raw(Box::new(t));

if descriptor.weakReferenceable:
create += """
JS_SetReservedSlot(obj.get(), DOM_WEAK_SLOT, PrivateValue(ptr::null()));"""
jsapi::JS_SetReservedSlot(obj.get(), DOM_WEAK_SLOT, &PrivateValue(ptr::null()) as *const _);"""

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

This cast would not be necessary.

JS_SetReservedSlot(prototype.get(), DOM_PROTO_UNFORGEABLE_HOLDER_SLOT,
ObjectValue(unforgeable_holder.get()))"""))
jsapi::JS_SetReservedSlot(prototype.get(), DOM_PROTO_UNFORGEABLE_HOLDER_SLOT,
&ObjectValue(unforgeable_holder.get()) as *const _)"""))

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

Unnecessary cast.

unsafe extern "C" fn(*mut jsapi::JSContext,
jsapi::JS::HandleObject,
_,
_) -> bool) },

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

Why is this transmute required?

This comment has been minimized.

@fitzgen

fitzgen Sep 20, 2017

Author Member

It's there to change the *const ActualType parameter into a *const std::os::raw::c_void. Upon reflection this is better done with as.

This comment has been minimized.

@fitzgen

fitzgen Sep 20, 2017

Author Member

hmmm nevermind, as doesn't work here:

error[E0605]: non-primitive cast: `unsafe extern "C" fn(*mut js::jsapi::JSContext, js::jsapi::JS::Handle<*mut js::jsapi::JSObject>, *const dom::xmlhttprequesteventtarget::XMLHttpRequestEventTarget, js::jsapi::JSJitSetterCallArgs) -> bool {dom::bindings::codegen::Bindings::XMLHttpRequestEventTargetBinding::XMLHttpRequestEventTargetBinding::set_onloadend}` as `unsafe extern "C" fn(*mut js::jsapi::JSContext, js::jsapi::JS::Handle<*mut js::jsapi::JSObject>, *mut std::os::raw::c_void, *const js::jsapi::JSJitMethodCallArgs) -> bool`
   --> /home/fitzgen/servo/target/debug/build/script-11a815433a47bb39/out/Bindings/XMLHttpRequestEventTargetBinding.rs:885:22
    |
885 |           method: Some(set_onloadend as
    |  ______________________^
886 | |             unsafe extern "C" fn(*mut jsapi::JSContext,
887 | |                                  jsapi::JS::Handle<*mut jsapi::JSObject>,
888 | |                                  _,
889 | |                                  _) -> bool),
    | |___________________________________________^
    |
    = note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait
@@ -23,7 +23,7 @@
//! | DOMString | `DOMString` |
//! | USVString | `USVString` |
//! | ByteString | `ByteString` |
//! | object | `*mut JSObject` |
//! | object | `*mut jsapi::JSObject` |

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

nit: the alignment in this table is now off.

let proto_array = PrivateValue(Box::into_raw(proto_array) as *const libc::c_void);
jsapi::JS_SetReservedSlot(rval.get(),
DOM_PROTOTYPE_SLOT,
&proto_array as *const _);

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

Unnecessary cast.

@@ -60,7 +60,8 @@ pub trait WeakReferenceable: DomObject + Sized {
count: Cell::new(1),
value: Cell::new(Some(NonZero::new(self))),
});
JS_SetReservedSlot(object, DOM_WEAK_SLOT, PrivateValue(ptr as *const c_void));
let ptr = PrivateValue(ptr as *const c_void);
jsapi::JS_SetReservedSlot(object, DOM_WEAK_SLOT, &ptr as *const _);

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

Unnecessary cast.

@@ -123,10 +114,11 @@ impl WindowProxy {

// The window proxy owns the browsing context.
// When we finalize the window proxy, it drops the browsing context it owns.
SetProxyExtra(js_proxy.get(), 0, &PrivateValue((&*window_proxy).as_void_ptr()));
let private = js::jsval::PrivateValue((&*window_proxy).as_void_ptr());
SetProxyReservedSlot(js_proxy.get(), 0, &private as *const _);

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

Unnecessary cast.

@@ -159,19 +151,19 @@ impl WindowProxy {
let window = DissimilarOriginWindow::new(global_to_clone_from, &*window_proxy);
let window_jsobject = window.reflector().get_jsobject();
assert!(!window_jsobject.get().is_null());
assert!(((*get_object_class(window_jsobject.get())).flags & JSCLASS_IS_GLOBAL) != 0);
let _ac = JSAutoCompartment::new(cx, window_jsobject.get());
assert!(((*js::rust::get_object_class(window_jsobject.get())).flags & jsapi::JSCLASS_IS_GLOBAL) != 0);

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

Perhaps we should use the new helper that was added for this.

@@ -233,89 +229,84 @@ pub unsafe fn new_rt_and_cx() -> Runtime {
false
};
let mode = if val {
JSGCMode::JSGC_MODE_INCREMENTAL
jsapi::JSGCMode::JSGC_MODE_INCREMENTAL
} else if compartment {

This comment has been minimized.

@jdm

jdm Jul 13, 2017

Member

Perhaps rename this now.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 3, 2017

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

@fitzgen fitzgen force-pushed the fitzgen:smup branch from 7e31662 to 9ce7c16 Sep 20, 2017
@fitzgen
Copy link
Member Author

fitzgen commented Sep 20, 2017

Feedback addressed. Need to rebase again. Still blocked on windows stuff in servo/mozjs#119.

@jdm
jdm approved these changes Oct 2, 2017
@nox
Copy link
Member

nox commented Oct 3, 2017

Could you please revert all the qualified paths to jsapi items you introduced?

@fabricedesre
Copy link
Contributor

fabricedesre commented Mar 18, 2018

Is anyone still working on that?

@jdm
Copy link
Member

jdm commented Mar 18, 2018

@fabricedesre @asajeffrey is going to be resuming this work.

@asajeffrey
Copy link
Member

asajeffrey commented Mar 19, 2018

I'm going to talk to @nox in Berlin about how to move forward; I'm happy to look at smup, but the problem is that I have approximately zero windows experience.

@nox
Copy link
Member

nox commented Mar 19, 2018

@asajeffrey For Windows support I haven't much to say but "good luck".

@asajeffrey
Copy link
Member

asajeffrey commented Mar 19, 2018

Er, thanks.

@asajeffrey
Copy link
Member

asajeffrey commented Apr 7, 2018

@jdm
Copy link
Member

jdm commented Jun 19, 2018

Superseded.

@jdm jdm closed this Jun 19, 2018
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

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