GC: Handlify various public APIs

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({dev-doc-needed})

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

6 years ago
This is a bug to handlify the following APIs:

 - JS_DescribeScriptedCaller()
 - JS_EncodeScript()
 - JS_EncodeInterpretedFunction()
 - JS_ConvertValue()
 - JS_ValueToObject()
 - JS_ValueToFunction()
 - JS_ValueToConstructor()
Assignee

Comment 1

6 years ago
Attachment #807322 - Flags: review?(sphink)
Assignee

Comment 2

6 years ago
Attachment #807323 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 3

6 years ago
Attachment #807324 - Flags: review?(bugs)
Comment on attachment 807323 [details] [diff] [review]
2 - XPConnect changes

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

::: js/xpconnect/loader/mozJSLoaderUtils.cpp
@@ +17,5 @@
>  // principals when writing a script. Instead, when reading it back, we set the
>  // principals to the system principals.
>  nsresult
>  ReadCachedScript(StartupCache* cache, nsACString &uri, JSContext *cx,
> +                 nsIPrincipal *systemPrincipal, JS::MutableHandleScript scriptp)

This file can |using namespace JS|.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +159,5 @@
>  }
>  
>  NS_IMETHODIMP
>  mozJSSubScriptLoader::LoadSubScript(const nsAString& url,
> +                                    const JS::Value& targetArg,

using namespace JS;

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +977,5 @@
>  
>                  // Finally, check to see if this is the last JS frame on the
>                  // stack. If so then we always want to report it.
>                  if (!reportable) {
> +                    RootedScript script(cx);

Let's call this |ignored|.
Attachment #807323 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 807322 [details] [diff] [review]
1 - JS engine changes

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

::: js/src/builtin/RegExp.cpp
@@ +421,5 @@
>          return true;                                                            \
>      }
>  
>  DEFINE_STATIC_SETTER(static_input_setter,
> +                     if (!JSVAL_IS_STRING(vp) && !JS_ConvertValue(cx, vp, JSTYPE_STRING, vp))

As long as you're changing this... :)

vp->isString()

@@ +426,4 @@
>                           return false;
>                       res->setPendingInput(JSVAL_TO_STRING(vp)))
>  DEFINE_STATIC_SETTER(static_multiline_setter,
> +                     if (!JSVAL_IS_BOOLEAN(vp) && !JS_ConvertValue(cx, vp, JSTYPE_BOOLEAN, vp))

and here

::: js/src/jsapi.cpp
@@ -313,5 @@
>  
>  JS_PUBLIC_API(bool)
> -JS_ConvertValue(JSContext *cx, jsval valueArg, JSType type, jsval *vp)
> -{
> -    RootedValue value(cx, valueArg);

This one makes me a little nervous, since value and vp could be pointing to the same thing. But I think it's fine; nothing uses value after setting vp. Here, the only reason to would be in an error message, and that both doesn't happen here and shouldn't update *vp anyway.

::: js/src/jsapi.h
@@ +4214,5 @@
>  #ifdef DEBUG
>      JSObject *callee = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));
>      if (JS_ObjectIsFunction(cx, callee)) {
> +        JS::RootedValue calleeValue(cx, JS_CALLEE(cx, vp));
> +        JSFunction *fun = JS_ValueToFunction(cx, calleeValue);

Huh. This whole function seems bogus. It seems like it ought to be

  CallArgs args = CallArgsFromVp(vp);
  return args.isConstructing();

but maybe there's some wrinkle I'm missing? Anyway, doesn't matter for this bug. Filed bug 918462.

::: js/src/shell/js.cpp
@@ +2354,5 @@
>      JSPropertyDescArray pda;
>      JSPropertyDesc *pd;
>  
> +    if (argc == 0)
> +        return false;

Whoa! This doesn't look good. This would return false without setting an exception if you didn't pass any arguments.

Seems like it would be better to switch to CallArgs here, and use args.get(0).

@@ +4174,5 @@
>  
>      JS_SET_RVAL(cx, vp, UndefinedValue());
>  
> +    RootedValue arg(cx, vp[0]);
> +    fun = JS_ValueToFunction(cx, arg);

This function *really* should be using CallArgs, but it's too messy to bother with for this patch.
Attachment #807322 - Flags: review?(sphink) → review+
Attachment #807324 - Flags: review?(bugs) → review+
I think I just bitrotted attachmen 807322 a bit, but I think I just eliminated the need for the changes to builtin/RegExp.cpp entirely, so hopefully it's no work for you.  :-)
Assignee

Comment 7

6 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
Yes, it merged with no problem :)
https://hg.mozilla.org/mozilla-central/rev/2d2709188afa
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 923390
Assignee

Updated

6 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.