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

Incorrect Signature for JS_DeletePropertyById1 #320

Open
Redfire75369 opened this issue Nov 18, 2022 · 2 comments
Open

Incorrect Signature for JS_DeletePropertyById1 #320

Redfire75369 opened this issue Nov 18, 2022 · 2 comments

Comments

@Redfire75369
Copy link
Contributor

Redfire75369 commented Nov 18, 2022

Actual Signature:

pub unsafe extern "C" fn JS_DeletePropertyById1(cx: *mut JSContext, obj: Handle<*mut JSObject>, id: PropertyKey) -> bool

Expected Signature:

bool JS_DeletePropertyById(JSContext* cx, HandleObject obj, HandleId id);

Expected Signature (Rust):

pub unsafe extern "C" fn JS_DeletePropertyById1(cx: *mut JSContext, obj: Handle<*mut JSObject>, id: Handle<PropertyKey>) -> bool

The last parameter should be a Handle to the PropertyKey, but for some reason, the raw PropertyKey is in the signature instead. This leads to a linking error when trying to compile.

This issue does not apply to JS_DeletePropertyById.

@sagudev
Copy link
Member

sagudev commented Sep 22, 2023

I can confirm that the problem still exist, but I think that's not our/bindgen bug.

Mozilla-esr115 has these two functions declared in https://searchfox.org/mozilla-esr115/source/js/public/PropertyAndElement.h#381:

// maped to JS_DeletePropertyById by bindgen
extern JS_PUBLIC_API bool JS_DeletePropertyById(JSContext* cx,
                                                JS::Handle<JSObject*> obj,
                                                JS::Handle<jsid> id,
                                                JS::ObjectOpResult& result);

// maped to JS_DeletePropertyById1 by bindgen
extern JS_PUBLIC_API bool JS_DeletePropertyById(JSContext* cx,
                                                JS::Handle<JSObject*> obj,
                                                jsid id);

While their definition is in https://searchfox.org/mozilla-esr115/source/js/src/vm/PropertyAndElement.cpp#739:

JS_PUBLIC_API bool JS_DeletePropertyById(JSContext* cx,
                                         JS::Handle<JSObject*> obj,
                                         JS::Handle<jsid> id,
                                         JS::ObjectOpResult& result) {
  AssertHeapIsIdle();
  CHECK_THREAD(cx);
  cx->check(obj, id);

  return js::DeleteProperty(cx, obj, id, result);
}


JS_PUBLIC_API bool JS_DeletePropertyById(JSContext* cx,
                                         JS::Handle<JSObject*> obj,
                                         JS::Handle<jsid> id) {
  JS::ObjectOpResult ignored;
  return JS_DeletePropertyById(cx, obj, id, ignored);
}

As you notice second function (the one that is mapped to JS_DeletePropertyById1) is not matching it's declaration, so this is actually upstream bug.

Why hasn't been caught? Because they do not use Rust 😄 also they do not actually use second function from at least esr91 (two years).

This bug is still present in mozilla-central.

@jandem
Copy link

jandem commented Sep 22, 2023

This bug is still present in mozilla-central.

Thanks for reporting this on Matrix. I posted a patch here: https://bugzilla.mozilla.org/show_bug.cgi?id=1854643

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

No branches or pull requests

3 participants