Handlify JS_WrapValue

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 822011 [details] [diff] [review]
WIP

This compiles, but I am still running it through try and I also want to take a second look at the diff myself.
(Assignee)

Comment 2

5 years ago
Created attachment 822222 [details] [diff] [review]
v1

Was looking good.
Attachment #822011 - Attachment is obsolete: true
Attachment #822222 - Flags: review?(terrence)
Comment on attachment 822222 [details] [diff] [review]
v1

Review of attachment 822222 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice! Just a few nits and questions. r=me

::: dom/base/nsGlobalWindow.cpp
@@ +621,5 @@
>    // False return value means we threw an exception.  True return value
>    // but false "found" means we didn't have a subframe at that index.
>    bool GetSubframeWindow(JSContext *cx, JS::Handle<JSObject*> proxy,
>                           JS::Handle<jsid> id,
> +                         JS::MutableHandleValue vp,

JS::MutableHandle<JS::Value> to match the local style.

@@ +881,5 @@
>  bool
>  nsOuterWindowProxy::GetSubframeWindow(JSContext *cx,
>                                        JS::Handle<JSObject*> proxy,
> +                                      JS::Handle<jsid> id,
> +                                      JS::MutableHandleValue vp,

<ditto>

@@ +886,3 @@
>                                        bool& found)
>  {
> +  vp.set(JS::UndefinedValue());

Why is this necessary? The previous version doesn't seem to touch vp until right before wrapping?

::: dom/base/nsJSEnvironment.cpp
@@ +1218,5 @@
>  
> +  JSObject *args = ::JS_NewArrayObject(mContext, argc, array.array);
> +  if (!args) {
> +    return NS_ERROR_FAILURE;
> +  }

Good catch!

::: dom/base/nsStructuredCloneContainer.cpp
@@ +48,5 @@
>    // Make sure that we serialize in the right context.
>    MOZ_ASSERT(aCx == nsContentUtils::GetCurrentJSContext());
>    JS::Rooted<JS::Value> jsData(aCx, aData);
> +  bool success = JS_WrapValue(aCx, &jsData);
> +  NS_ENSURE_STATE(success);

Good catch!

::: dom/bindings/BindingUtils.cpp
@@ +182,5 @@
>    MOZ_ASSERT(!mMightHaveUnreportedJSException,
>               "Why didn't you tell us you planned to handle JS exceptions?");
> +
> +  JS::RootedValue exception(cx, mJSException);
> +  if (JS_WrapValue(cx, &exception)) {

Don't you need to write |exception| back into |mJSException| after wrapping the stack location?

@@ +1620,5 @@
>    if (!str) {
>      return false;
>    }
>  
> +  v.set(JS::StringValue(str));

I thought .setString() was the preferred spelling. Does that not work here?
Attachment #822222 - Flags: review?(terrence) → review+
Comment on attachment 822222 [details] [diff] [review]
v1

Review of attachment 822222 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +886,3 @@
>                                        bool& found)
>  {
> +  vp.set(JS::UndefinedValue());

But if you keep it, setUndefined()

@@ +904,5 @@
>    if (MOZ_UNLIKELY(!obj)) {
>      return xpc::Throw(cx, NS_ERROR_FAILURE);
>    }
> +
> +  vp.set(JS::ObjectValue(*obj));

setObject()

::: dom/base/nsJSUtils.cpp
@@ +305,5 @@
>      }
>    }
>  
>    // Wrap the return value into whatever compartment aCx was in.
> +  if (aRetValue) {

Followup to make EvaluateString take a handle?

@@ +306,5 @@
>    }
>  
>    // Wrap the return value into whatever compartment aCx was in.
> +  if (aRetValue) {
> +    JS::RootedValue v(aCx, *aRetValue);

<>

@@ +308,5 @@
>    // Wrap the return value into whatever compartment aCx was in.
> +  if (aRetValue) {
> +    JS::RootedValue v(aCx, *aRetValue);
> +    if (!JS_WrapValue(aCx, &v))
> +      return NS_ERROR_OUT_OF_MEMORY;

{}

::: dom/bindings/BindingUtils.cpp
@@ +181,5 @@
>  {
>    MOZ_ASSERT(!mMightHaveUnreportedJSException,
>               "Why didn't you tell us you planned to handle JS exceptions?");
> +
> +  JS::RootedValue exception(cx, mJSException);

<>

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +328,5 @@
>      if (cache && ok && writeScript) {
>          WriteCachedScript(cache, cachePath, cx, mSystemPrincipal, script);
>      }
>  
>      return NS_OK;

You probably need to set *retval here. Ask bholley, please?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1234,4 @@
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (!args.thisv().isObject()) {

Please don't change the computeThis behaviour without peer signoff
(Assignee)

Comment 5

5 years ago
Thanks for review you two. Truly hate the style mismatch between JS and everything else.

To the last two. Seems like a preexisting weirdness. Even when ok is false, we return NS_OK and do caching? I don't see the problem with the last bit, this is strictly more secure, and acting on the global would most likely be wrong. But bholley can take a look.
Flags: needinfo?(bobbyholley+bmo)
(In reply to Tom Schuster [:evilpie] from comment #5)

> To the last two. Seems like a preexisting weirdness. Even when ok is false,
> we return NS_OK and do caching?

I don't have an opinion about this.

> I don't see the problem with the last bit,
> this is strictly more secure, and acting on the global would most likely be
> wrong. But bholley can take a look.

JS_THIS_OBJECT does non-strict unboxing, while .thisv() does strict()? If so, that seems fine with me.
Flags: needinfo?(bobbyholley+bmo)
https://hg.mozilla.org/mozilla-central/rev/05c31c03a8dd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.