Last Comment Bug 752226 - Remove JSVAL_IS_OBJECT and fix its callers
: Remove JSVAL_IS_OBJECT and fix its callers
Status: RESOLVED FIXED
[js:p3]
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on: 754759 774770 780993
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-05 08:23 PDT by Tom Schuster [:evilpie]
Modified: 2012-12-11 10:00 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests (1.57 KB, patch)
2012-05-06 13:17 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Tests (1.77 KB, patch)
2012-05-06 13:26 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review
WIP (81.65 KB, patch)
2012-05-06 13:40 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v1 (82.10 KB, patch)
2012-05-08 06:29 PDT, Tom Schuster [:evilpie]
Ms2ger: review+
luke: review+
Details | Diff | Splinter Review
More tests (3.90 KB, patch)
2012-05-08 06:31 PDT, :Ms2ger (⌚ UTC+1/+2)
evilpies: review+
Details | Diff | Splinter Review

Description Tom Schuster [:evilpie] 2012-05-05 08:23:12 PDT
JSVAL_IS_OBJECT is evil, because a lot of people don't understand that is actually object || null.

At some point we should consider changing the API so it does the obvious.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-05 11:25:56 PDT
With JS::Value, which I'm pretty sure is part of the public API, arguably we should change all in-tree JSVAL_* users to that.

I wouldn't touch JSVAL_IS_OBJECT's semantics, myself -- it's been around too long, and a safe changeover for anyone requires auditing every use (which I suspect the average embedder wouldn't necessarily do, even if we loudly documented the requirement in release notes).  However, if we were to switch to being a purely C++ API, I'd say we should remove all the JSVAL_* methods.  They're all trivial to reimplement as inlines that call JS::Value methods, so it wouldn't be much skin off anyone's nose.
Comment 2 Tom Schuster [:evilpie] 2012-05-05 11:36:55 PDT
I wrote a message about changing the behavior of JSVAL_IS_OBJECT to the mailing-list already.
http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/cafb2a22725bd104#
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-05-06 13:17:08 PDT
Created attachment 621452 [details] [diff] [review]
Tests

Some null-dereferences evilpie is going to fix
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-05-06 13:26:21 PDT
Created attachment 621453 [details] [diff] [review]
Tests

hg add is good, I hear.
Comment 5 Tom Schuster [:evilpie] 2012-05-06 13:40:38 PDT
Created attachment 621459 [details] [diff] [review]
WIP
Comment 6 David Mandelin [:dmandelin] 2012-05-07 13:16:59 PDT
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #1)
> I wouldn't touch JSVAL_IS_OBJECT's semantics, myself -- it's been around too
> long, and a safe changeover for anyone requires auditing every use (which I
> suspect the average embedder wouldn't necessarily do, even if we loudly
> documented the requirement in release notes).  

Agreed. Let's not change JSVAL_IS_OBJECT. Feel free to add a new function that does the thing that's more useful for Firefox, and make it C++ if you want.

> With JS::Value, which I'm pretty sure is part of the public API, arguably we
> should change all in-tree JSVAL_* users to that.
> 
> However, if we were to switch to being a purely C++ API, I'd say we should 
> remove all the JSVAL_* methods. They're all trivial to reimplement as inlines
> that call JS::Value methods, so it wouldn't be much skin off anyone's nose.

Long-term, I think we want to do something like that. JS::Value is probably pretty stable, so if someone had the time to make that change it seems OK. (It's pretty big, right.) Updating to a C++ API is a big project that I think we should do, but (given plans on the table now) not this year.
Comment 7 Tom Schuster [:evilpie] 2012-05-08 06:29:15 PDT
Created attachment 621953 [details] [diff] [review]
v1

Sorry, I am a bit lazy on this on. The changes are very small and easy to review. There is also going to be one or two more tests ontop of what Ms2ger already contributed :)
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-05-08 06:31:55 PDT
Created attachment 621954 [details] [diff] [review]
More tests

Added another test evilpie suggested
Comment 9 Luke Wagner [:luke] 2012-05-08 11:06:10 PDT
Comment on attachment 621953 [details] [diff] [review]
v1

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

Cool.  I see quite a few potential null-crashes.

::: dom/base/nsDOMClassInfo.cpp
@@ +9006,5 @@
>    } else {
>      // document.all is not being detected, and it resolved with a
>      // qualified name. Expose the document.all collection.
>  
> +    if (!vp->isObject()) { 

Need !vp->isObjectOrNull() to preserve existing behavior.

@@ +9478,5 @@
>  {
>    JSAutoRequest ar(cx);
>  
>    // vp must refer to an object
> +  if (!vp->isObject()) {

This does not seem equivalent; JS_ConvertValue will convert primitives to objects.  Also, should be isObjectOrNull...

@@ -9486,5 @@
>    }
>  
>    nsCOMPtr<nsIDOMHTMLOptionElement> new_option;
> -
> -  if (!JSVAL_IS_NULL(*vp)) {

... which would mean you still need this test.

::: js/jsd/jsd_val.c
@@ +86,5 @@
>  
>  JSBool
>  jsd_IsValueObject(JSDContext* jsdc, JSDValue* jsdval)
>  {
> +    return !JSVAL_IS_PRIMITIVE(jsdval->val); /* or || IS_NULL ? */

Preserve existing behavior: so, 'yes', || JSVAL_IS_NULL

::: js/xpconnect/src/XPCConvert.cpp
@@ +833,5 @@
>                      *pErr = NS_ERROR_XPC_BAD_CONVERT_JS_ZERO_ISNOT_NULL;
>                  return false;
>              }
>  
> +            JSObject* obj = &s.toObject();

This shadows an enclosing 'obj'.  It is correct, but may lead to later confusion.  How about just using &s.toObject() directly as the actual argument?
Comment 10 Tom Schuster [:evilpie] 2012-05-08 11:29:16 PDT
Comment on attachment 621953 [details] [diff] [review]
v1

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

Thank you for this very quick review \o/

::: dom/base/nsDOMClassInfo.cpp
@@ +9006,5 @@
>    } else {
>      // document.all is not being detected, and it resolved with a
>      // qualified name. Expose the document.all collection.
>  
> +    if (!vp->isObject()) { 

This is weird in any case, need to test what is going on here. (Why would you check *vp in an GetProperty hook?

@@ +9478,5 @@
>  {
>    JSAutoRequest ar(cx);
>  
>    // vp must refer to an object
> +  if (!vp->isObject()) {

I discussed that with Ms2ger. I think this is never going to create an object which would be an nsIDOMHTMLOptionElement.
Comment 11 Luke Wagner [:luke] 2012-05-08 11:37:10 PDT
(In reply to Tom Schuster [:evilpie] from comment #10)
> This is weird in any case, need to test what is going on here. (Why would
> you check *vp in an GetProperty hook?

IIRC, this is an old "feature" of JSAPI: each getter gets a slot in the object which gets updated with the return value of the getter. This allows the slot to be used as a cache for the getter.  See js_NativeGetInline.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-05-09 05:48:02 PDT
Comment on attachment 621953 [details] [diff] [review]
v1

Ms2ger's review is sufficient for any of the stuff I would need to review here.
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-05-09 11:26:40 PDT
Comment on attachment 621953 [details] [diff] [review]
v1

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

Haven't reviewed js/src and js/jsd. Please be so kind to fix a few nits:

::: caps/src/nsSecurityManagerFactory.cpp
@@ +173,5 @@
>          /*
>           * "netscape" property of window object exists; get the
>           * "security" property.
>           */
> +        if (!JS_GetProperty(cx, &v.toObject(), "security", &v) || !v.isObject())

I would prefer if you kept the assignment to |obj|.

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +410,5 @@
>    // We need to figure out what our jsval is
>  
> +  // Just return for null
> +  if (aData.isNull())
> +    return NS_OK;

Remove this; null should fall through to the ToString() below.

::: content/base/src/nsDOMFile.cpp
@@ +543,1 @@
>      NS_ASSERTION(obj, "This is a bit odd");

No point in this assertion anymore.

::: content/base/src/nsFrameMessageManager.cpp
@@ +480,1 @@
>            thisValue = OBJECT_TO_JSVAL(object);

If you're going to use JS::ObjectValue above, do the same here.

::: content/base/src/nsWebSocket.cpp
@@ +609,5 @@
>  
>    if (aArgc == 2) {
> +    if (aArgv[1].isObject() &&
> +        JS_IsArrayObject(aContext, &aArgv[1].toObject())) {
> +      JSObject *jsobj = &aArgv[1].toObject();

* on the left

::: content/canvas/src/CustomQS_WebGL.h
@@ +192,5 @@
>  
>      if (argc < 6 || argc == 7 || argc == 8)
>          return xpc_qsThrow(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
>  
> +    JS::Value *argv = JS_ARGV(cx, vp);

* on the left

@@ +218,5 @@
>          GET_INT32_ARG(argv5, 5);
>          GET_UINT32_ARG(argv6, 6);
>          GET_UINT32_ARG(argv7, 7);
>  
> +        JSObject *argv8 = argv[8].toObjectOrNull();

Ditto.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +502,1 @@
>                                  nsISupports **aContext)

I've got a patch to fix this function over in bug 616401.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +476,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsHTMLMediaElement::SetSrc(JSContext* aCtx, const JS::Value & aParams)

No space before the &

::: dom/base/nsDOMClassInfo.cpp
@@ +6495,5 @@
>          return NS_ERROR_UNEXPECTED;
>        }
>  
> +      if (val.isObject()) {
> +        if (!::JS_LookupProperty(cx, &val.toObject(), "prototype", &val)) {

Get rid of the :: while you're here.

@@ +8934,5 @@
>    // another method, use the document.all callee object as self.
>    JSObject *self;
> +  JS::Value callee = JS_CALLEE(cx, vp);
> +  if (callee.isObject() &&
> +      ::JS_GetClass(&callee.toObject()) == &sHTMLDocumentAllClass) {

Same here.

@@ +9006,5 @@
>    } else {
>      // document.all is not being detected, and it resolved with a
>      // qualified name. Expose the document.all collection.
>  
> +    if (!vp->isObject()) { 

Making doc.all code saner probably isn't worth it.

@@ +9478,5 @@
>  {
>    JSAutoRequest ar(cx);
>  
>    // vp must refer to an object
> +  if (!vp->isObject()) {

r+ for this change.

@@ +9483,5 @@
>      return NS_ERROR_UNEXPECTED;
>    }
>  
>    nsCOMPtr<nsIDOMHTMLOptionElement> new_option;
> +  new_option = do_QueryWrapper(cx, &vp->toObject());

Please make this one line.

@@ +9486,5 @@
>    nsCOMPtr<nsIDOMHTMLOptionElement> new_option;
> +  new_option = do_QueryWrapper(cx, &vp->toObject());
> +  if (!new_option) {
> +	// Someone is trying to set an option to a non-option object.
> +	return NS_ERROR_UNEXPECTED;

Tabs :(

::: dom/workers/WorkerPrivate.cpp
@@ -3623,4 @@
>  
>    // Take care of the main argument.
> -  if (JSVAL_IS_OBJECT(argv[0])) {
> -    if (JS_ObjectIsCallable(aCx, JSVAL_TO_OBJECT(argv[0]))) {

bent--

::: dom/workers/WorkerScope.cpp
@@ +302,2 @@
>  
>      JSObject* wrapper = JSVAL_TO_OBJECT(JS_CALLEE(aCx, aVp));

toObject?

@@ +310,2 @@
>  
>      JSObject* event = JSVAL_TO_OBJECT(JS_ARGV(aCx, aVp)[0]);

toObject?

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1024,1 @@
>          }

Keep the early return.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3350,5 @@
>              if (!xpc)
>                  return NS_ERROR_XPC_UNEXPECTED;
>              nsCOMPtr<nsIXPConnectWrappedNative> wrapper;
> +            xpc->GetWrappedNativeOfJSObject(cx, &argv[0].toObject(),
> +                                                getter_AddRefs(wrapper));

You messed up the indentation.

@@ +3949,5 @@
>  
>  JSBool
>  FunctionWrapper(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    JS::Value v = js::GetFunctionNativeReserved(JSVAL_TO_OBJECT(JS_CALLEE(cx, vp)), 0);

I would have no objection to putting JSVAL_TO_OBJECT(JS_CALLEE(cx, vp)) in a local variable.

::: js/xpconnect/src/XPCConvert.cpp
@@ +536,2 @@
>                  if (pErr)
>                    *pErr = NS_ERROR_XPC_BAD_CONVERT_JS;

Fix the indentation while you're here. (Should be 4-space.)

@@ +833,5 @@
>                      *pErr = NS_ERROR_XPC_BAD_CONVERT_JS_ZERO_ISNOT_NULL;
>                  return false;
>              }
>  
> +            JSObject* obj = &s.toObject();

The shadowing already existed, but yes, please fix.

@@ +1772,3 @@
>      void* array = nsnull;
>      uint32_t initedCount;
>      jsval current;

Feel free to move these down a bit too, while you're here.

::: js/xpconnect/src/XPCVariant.cpp
@@ +117,5 @@
>  #endif
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(XPCVariant)
> +    JS::Value val = tmp->GetJSValPreserveColor();
> +    if (val.isObject()) {

OrNull

@@ +233,2 @@
>              type = tBool;
> +        else if (val.isUndefined()) {

Please brace everywhere while you're here.

::: storage/src/mozStoragePrivateHelpers.cpp
@@ +160,2 @@
>      nsDependentJSString value;
>      if (!value.init(aCtx, str))

Just call init with aValue.

@@ +170,4 @@
>      return new NullVariant();
>  
> +  if (aValue.isObject()) {
> +    JSObject *obj = &aValue.toObject();

* to the left.
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-05-09 13:02:22 PDT
Comment on attachment 621953 [details] [diff] [review]
v1

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +476,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsHTMLMediaElement::SetSrc(JSContext* aCtx, const JS::Value & aParams)

Actually, this bug was already fixed in bug 752087.
Comment 15 Tom Schuster [:evilpie] 2012-05-10 09:59:24 PDT
Comment on attachment 621953 [details] [diff] [review]
v1

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

Thanks for the good review. I am also going to change some stuff like *out = OBJECT_TO_JSVAL(obj) to out->setObject(*obj);.

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +410,5 @@
>    // We need to figure out what our jsval is
>  
> +  // Just return for null
> +  if (aData.isNull())
> +    return NS_OK;

This fails some test in the test suite, should I just change the failing test? btw, this test was already testing for a crash ;)

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +502,1 @@
>                                  nsISupports **aContext)

Okay.
Comment 16 Tom Schuster [:evilpie] 2012-05-10 11:18:49 PDT
Comment on attachment 621954 [details] [diff] [review]
More tests

LGTM
Comment 17 Tom Schuster [:evilpie] 2012-05-10 23:41:40 PDT
Ms2ger, looks like something in your test causes a crash? https://tbpl.mozilla.org/?tree=Try&rev=e7b85bd432ed
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-05-11 03:03:12 PDT
Looks like I forgot some try{}catch{}es; we don't crash anymore, but the exception causes a timeout
Comment 20 Ed Morley [:emorley] 2012-05-11 10:28:09 PDT
Sorry, backed out for:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/base/test/chrome/test_bug752226-3.xul | application timed out after 330 seconds with no output
...
PROCESS-CRASH | chrome://mochitests/content/chrome/content/base/test/chrome/test_bug752226-3.xul | application crashed (minidump found)
Crash dump filename: /tmp/tmpV1mKWu/minidumps/479db0a3-98ad-b111-13ddd177-26991089.dmp
Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.x86_64 #1 SMP Sat Nov 7 21:11:14 EST 2009 x86_64
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGABRT
Crash address: 0x1f4000008ce

Thread 0 (crashed)
 0  libc-2.11.so + 0xd4aa3
    rbx = 0x1ef5d6d0   r12 = 0x1c43f4e2   r13 = 0xd4ce5160   r14 = 0x00000009
    r15 = 0x1ef5d6d8   rip = 0xd2ed4aa3   rsp = 0x62a93e80   rbp = 0x06cdd6f0
    Found by: given as instruction pointer in context
 1  libxul.so!PollWrapper [nsAppShell.cpp : 66 + 0x12]
    rip = 0x1c43f50b   rsp = 0x62a93eb0
    Found by: stack scanning
 2  libglib-2.0.so.0.2200.2 + 0x3c9fb
    rip = 0xd4a3c9fc   rsp = 0x62a93ee0
    Found by: call frame info
 3  libglib-2.0.so.0.2200.2 + 0x2e4ac7
    rip = 0xd4ce4ac8   rsp = 0x62a93ee8
    Found by: stack scanning
 4  libglib-2.0.so.0.2200.2 + 0x2e4aff
    rip = 0xd4ce4b00   rsp = 0x62a93ef0
    Found by: stack scanning
 5  libpthread-2.11.so + 0x8daf
    rip = 0xd3608db0   rsp = 0x62a93f48
    Found by: stack scanning
 6  libglib-2.0.so.0.2200.2 + 0x3cd39
    rip = 0xd4a3cd3a   rsp = 0x62a93f60
    Found by: stack scanning
 7  libxul.so!nsAppShell::ProcessNextNativeEvent [nsAppShell.cpp : 162 + 0xa]
    rip = 0x1c43f4b7   rsp = 0x62a93f90
    Found by: stack scanning
 8  libxul.so!nsBaseAppShell::DoProcessNextNativeEvent [nsBaseAppShell.cpp : 172 + 0x9]
    rip = 0x1c455053   rsp = 0x62a93fa0
    Found by: call frame info
 9  libxul.so!nsBaseAppShell::OnProcessNextEvent [nsBaseAppShell.cpp : 331 + 0xa]
    rbx = 0x11aae160   rip = 0x1c45516a   rsp = 0x62a93fd0   rbp = 0x1ef1f100
    Found by: call frame info
10  libxul.so!nsThread::ProcessNextEvent [nsThread.cpp : 618 + 0x7]
    rbx = 0x1ef1f100   r12 = 0x8000ffff   r13 = 0x1ef5f000   r14 = 0x62a940bf
    r15 = 0x00000000   rip = 0x1c5ae97d   rsp = 0x62a94020   rbp = 0x1efc8f01
    Found by: call frame info
11  libxul.so!NS_ProcessNextEvent_P [nsThreadUtils.cpp : 245 + 0xd]
    rbx = 0x14a33801   r12 = 0x14a33818   r13 = 0x1ef5f000   r14 = 0x00000000
    r15 = 0x00000000   rip = 0x1c582557   rsp = 0x62a940b0   rbp = 0x1efc8ff0
    Found by: call frame info
}

https://tbpl.mozilla.org/php/getParsedLog.php?id=11679128&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/6eba756a0d37
Comment 21 Tom Schuster [:evilpie] 2012-05-13 03:37:22 PDT
I fixed the tests and pushed it again. (The SimpleTest.js and css had the wrong URI)
http://hg.mozilla.org/integration/mozilla-inbound/rev/366ab61b0af7
http://hg.mozilla.org/integration/mozilla-inbound/rev/2eaba70259bf
Comment 23 Matt Brubeck (:mbrubeck) 2012-05-13 21:09:00 PDT
This bug's tests are asserting intermittently (bug 754759).  It should either be backed out, or the assertion should be annotated if it is known not to be a problem.
Comment 24 Tom Schuster [:evilpie] 2012-06-02 11:34:36 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/2f02e08e634c
should refer to Bug 760555.
Comment 25 Jorge Villalobos [:jorgev] 2012-07-05 17:26:45 PDT
Ms2ger: what should I be looking for here in terms of add-on compat? Nothing obvious stands out.
Comment 26 :Ms2ger (⌚ UTC+1/+2) 2012-07-06 04:33:29 PDT
Add-ons that use JSAPI and call the JSVAL_IS_OBJECT function; I don't think there are many, though.
Comment 27 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-17 11:36:10 PDT
I don't think addons should be a problem here.  JSVAL_IS_OBJECT was a JS_ALWAYS_INLINE in the header, so calls to it should just work.  And I really don't think anyone's going to be taking the address of JSVAL_IS_OBJECT as the only other way it'd break.  :-)  (Not to mention that wouldn't have worked up until fatvals anyway.)
Comment 28 Tom Schuster [:evilpie] 2012-12-05 14:13:15 PST
https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference
https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JSVAL_IS_OBJECT
are updated. Do we have some good place to put JSAPI changes right now?

Note You need to log in before you can comment on or make changes to this bug.