362668 - Avoid explicitly referencing JSObject.slots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

One of the reason that causes the patch for bug 331966 to be rather big is that the current code often accesses JSObject.slots directly when it is known that this is a thread safe operation.

Thus I suggest to replace that by code that uses LOCKED_OBJ_GET_SLOT and LOCKED_OBJ_SET_SLOT macros.

Comment 1

[Igor Bukanov]

Attachment: Implementation v1

This is a preliminary patch that replaces all references to JSObject.slot by LOCKED_OBJ_(GET|SET)_(SLOT|PROTO|PARENT|CLASS).

This is a preliminary version since it causes an assert for a browser build on startup:

 Assertion failure: (uint32)slot < ((obj)->map)->freeslot, at /home/igor/m/trunk/mozilla/js/src/jsobj.c:4886

with the following stack trace:

#3  0xb7e0403f in JS_Assert (
    s=0xb7e200d8 "(uint32)slot < ((obj)->map)->freeslot", 
    file=0xb7e21878 "/home/igor/m/trunk/mozilla/js/src/jsobj.c", ln=4886)
    at /home/igor/m/trunk/mozilla/js/src/jsutil.c:61
#4  0xb7dc6fde in js_SetRequiredSlot (cx=0x810fc58, obj=0x8197fa0, slot=6, 
    v=135320088) at /home/igor/m/trunk/mozilla/js/src/jsobj.c:4886
#5  0xb7d6a57d in JS_SetReservedSlot (cx=0x810fc58, obj=0x8197fa0, index=2, 
    v=135320088) at /home/igor/m/trunk/mozilla/js/src/jsapi.c:3516
#6  0xb7db3dc4 in js_Interpret (cx=0x810fc58, pc=0x81939bc "�", 
    result=0xbfb4e838) at /home/igor/m/trunk/mozilla/js/src/jsinterp.c:4272
#7  0xb7dbded6 in js_Invoke (cx=0x810fc58, argc=3, flags=2)
    at /home/igor/m/trunk/mozilla/js/src/jsinterp.c:1418
#8  0xb6026f1a in nsXPCWrappedJSClass::CallMethod (this=0x8496088, 
    wrapper=0x833bd50, methodIndex=3, info=0x80a4cd0, nativeParams=0xbfb4eb6c)
    at /home/igor/m/trunk/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1419
#9  0xb602061b in nsXPCWrappedJS::CallMethod (this=0x833bd50, methodIndex=3, 
    info=0x80a4cd0, params=0xbfb4eb6c)
    at /home/igor/m/trunk/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:479
#10 0xb7d1aadb in PrepareAndDispatch (methodIndex=<value optimized out>, 
    self=0x833bd88, args=<value optimized out>)
    at /home/igor/m/trunk/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95
#11 0xb7cb1683 in nsObserverList::NotifyObservers (this=0x8140cf8, 
    aSubject=0x0, aTopic=0xb7ed75b9 "profile-after-change", 
    someData=0xb7ed7ca2)
    at /home/igor/m/trunk/mozilla/xpcom/ds/nsObserverList.cpp:128
#12 0xb7cb2a8f in nsObserverService::NotifyObservers (this=0x809a9f0, 
    aSubject=0x0, aTopic=0xb7ed75b9 "profile-after-change", 
    someData=0xb7ed7ca2)
    at /home/igor/m/trunk/mozilla/xpcom/ds/nsObserverService.cpp:181
#13 0xb7eb7dfb in nsXREDirProvider::DoStartup (this=0xbfb4edc0)
    at /home/igor/m/trunk/mozilla/toolkit/xre/nsXREDirProvider.cpp:677
#14 0xb7eafcf1 in XRE_main (argc=2, argv=0xbfb4f044, aAppData=0x80497e0)
    at /home/igor/m/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2407
#15 0x08048663 in main (argc=170, argv=0x6cb3e18)
    at /home/igor/m/trunk/mozilla/browser/app/nsBrowserApp.cpp:61

I will try to see why the assert is wrong is this case.

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

Looks like attachment 247368 [details] [diff] [review] is a dud?

Comment 3

[Igor Bukanov]

Attachment: Implementation v1 for real

Attaching hopefully the properly generated file.

Comment 4

[Igor Bukanov]

Attachment: Implementation v2

I figured out what was wrong with the previous patch. LOCKED_OBJ_ macros check that the slot < min(map->nslots, map->freeslot). But this is wrong when obj->map->object != obj. Thus to replace the current code directly accessing JSDObject.slots I need another set of macros that would not do such checks and this is what the new patch does. It adds STO_(GET|SET)_(SLOT|PROTO|PARENT|CLASS) where STO means Single-Threaded-Object to replace the current code. The macros do not contain any checks.

Strictly speaking STO_(GET|SET)_(PROTO|PARENT|CLASS) are unnecessary as LOCKED_OBJ_(GET|SET)_(PROTO|PARENT|CLASS) checks should always pass the map constrains for proto, parent and class slots, but I wanted to ensure that the patch does not change the semantics of the current code.

Comment 5

[Igor Bukanov]

Attachment: Implementation v3

The new version covers the remaining cases of direct access to obj->slots with the goal of minimizing the subsequent fat-object patch.

The non-trivial changes in jslock.c regarding MakeMutableString comes from the desire to avoid exposing &obj->slots[slot] through macros.

Comment 6

[Brendan Eich [:brendan]]

Code size effects of v3?

Minor plea for STOBJ_ instead of STO_ -- having "OBJ" in the name helps me as a reader, at least.

/be

Comment 7

[Igor Bukanov]

Attachment: Implementation v4

The version with STO->STOBJ rename

Comment 8

[Igor Bukanov]

Attachment: Implementation v4 for real

I promise the next time to check the file before attaching it

Comment 9

[Igor Bukanov]

(In reply to comment #6)
> Code size effects of v3?

The size optimized build of js shell with GCC 4.1.2:

          -O       -O2     -Os
Before  588204   607564   501868
After   588236   607564   501868

That is, when the compiler can inline static functions and move common code ouside the loops, the patch does not change the code size. This is expected.

Note that the goal of this patch is to make the following fat-object patch smaller and code clearer rather then to optimize for code size.

Comment 10

[Igor Bukanov]

Attachment: Implementation v5

The patch update to switch few more places to use STOBJ_ macros.

Comment 11

[Igor Bukanov]

Attachment: Implementation v6

Covering the remaining cases of explicit access to JSObject.slots in the liveconnect files.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 247667 [details] [diff] [review]
Implementation v6

Thanks, I was looking for no code size change (not a win -- looking forward to that in the dependent bug).

BTW, bug 333078 has a patch that will want STOBJ_NSLOTS.

>@@ -597,26 +597,29 @@ js_ComputeThis(JSContext *cx, JSObject *
>         if (JSVAL_IS_PRIMITIVE(argv[-2]) ||
>             !OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(argv[-2]))) {
>             thisp = cx->globalObject;
>         } else {
>             jsid id;
>             jsval v;
>             uintN attrs;
>+            JSObject *parent;
> 
>             /* Walk up the parent chain. */
>             thisp = JSVAL_TO_OBJECT(argv[-2]);
>             id = ATOM_TO_JSID(cx->runtime->atomState.parentAtom);
>             for (;;) {
>                 if (!OBJ_CHECK_ACCESS(cx, thisp, id, JSACC_PARENT, &v, &attrs))
>                     return NULL;
>                 if (JSVAL_IS_VOID(v))
>-                    v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT);
>-                if (JSVAL_IS_NULL(v))
>+                    parent = OBJ_GET_PARENT(cx, thisp);
>+                else
>+                    parent = JSVAL_TO_OBJECT(v);

Bug?  Why the change from JSVAL_IS_VOID to JSVAL_IS_NULL?

Nit: use ?: instead of if/else to consolidate 'parent = ...'.

>+    for (i = 0; i != nslots; ++i) {
>+        v = STOBJ_GET_SLOT(obj, i);
>+        if (JSVAL_IS_STRING(v) &&
>+            !MakeStringImmutable(rt, JSVAL_TO_STRING(v))) {
>+            /*
>+             * XXX Report failure here as the it is not propogated?

Report how?  (Propagated misspelled, btw).  This should be a FIXME: bug nnnnnnn by the rule against XXX comments.  But I'm not convinced it's worth it.

>+             * XXX The following is a workarround that does not replace all
>+             * copies of v but as long as the copies of v are shared between
>+             * threads using only thread-safe OBJ_SET_SLOT macros, this is
>+             * safe.
>+             */
>+            STOBJ_SET_SLOT(obj, i, JSVAL_VOID);
>+        }

Typo (workarround) but mainly, this is not an XXX comment.  s/XXX/NB:/, I say.

>+    if (JSVAL_IS_STRING(v) &&
>+        !MakeStringImmutable(cx->runtime, JSVAL_TO_STRING(v))) {
>+        /* XXX See comments in js_FinishSharingScope. */

Lose the XXX here too.


>+/*
>+ * STOBJ prefix means Single Threaded Object. Use the following fast macros to
>+ * directly manipulate obj->slots when only one thread can access the obj and
>+ * when obj->map->freeslot, obj->map->nslots can be incosistent with slots.

Nit: lose the "the" before "obj in the second line, and fix the "inconsistent" misspelling.

Looks good otherwise, I'll re-review a new patch and stamp r+.

/be

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> 
> >@@ -597,26 +597,29 @@ js_ComputeThis(JSContext *cx, JSObject *
...
> >+            JSObject *parent;
...
> >             for (;;) {
...
> >                 if (JSVAL_IS_VOID(v))
> >-                    v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT);
> >-                if (JSVAL_IS_NULL(v))
> >+                    parent = OBJ_GET_PARENT(cx, thisp);
> >+                else
> >+                    parent = JSVAL_TO_OBJECT(v);
> 
> Bug?  Why the change from JSVAL_IS_VOID to JSVAL_IS_NULL?

The patch does not change it. Rather it transforms the code to use from:

if (JSVAL_IS_VOID(v))
    v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT);
if (JSVAL_IS_NULL(v))
    break;
thisp = JSVAL_TO_OBJECT(v);

into:

if (JSVAL_IS_VOID(v))
    parent = OBJ_GET_PARENT(cx, thisp);
else
    parent = JSVAL_TO_OBJECT(v);
if (!parent)
    break;
thisp = parent;

which uses OBJ_GET_PARENT macro. This is equivalent. 

> >+    for (i = 0; i != nslots; ++i) {
> >+        v = STOBJ_GET_SLOT(obj, i);
> >+        if (JSVAL_IS_STRING(v) &&
> >+            !MakeStringImmutable(rt, JSVAL_TO_STRING(v))) {
> >+            /*
> >+             * XXX Report failure here as the it is not propogated?
> 
> Report how?

In the same way as out-of-memory would be reported.

  (Propagated misspelled, btw).  This should be a FIXME: bug nnnnnnn
> by the rule against XXX comments.  But I'm not convinced it's worth it.

> 
> >+             * XXX The following is a workarround that does not replace all
> >+             * copies of v but as long as the copies of v are shared between
> >+             * threads using only thread-safe OBJ_SET_SLOT macros, this is
> >+             * safe.
> >+             */
> >+            STOBJ_SET_SLOT(obj, i, JSVAL_VOID);
> >+        }
> 
> Typo (workarround) but mainly, this is not an XXX comment.  s/XXX/NB:/, I say.

But why NB when the code silently alters script semantics by storing void, not the string in the slot array on low-memory pressure? Another problem is with an embedding that stores a jsval in thread-shared location. SpiderMonkey knows that all string jsvals must be made independent before such store, but this is very local knowledge and AFAICS is not stated anywhere.     

So should I file 2 bugs about it?

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> The patch does not change it. Rather it transforms the code to use from:

Sorry, misread the patch.

> In the same way as out-of-memory would be reported.

That requires a cx, which we lack here.  Anyhoo, a FIXME: is the way to go, so please file another bug.

>   (Propagated misspelled, btw).  This should be a FIXME: bug nnnnnnn
> > Typo (workarround) but mainly, this is not an XXX comment.  s/XXX/NB:/, I say.
> 
> But why NB when the code silently alters script semantics by storing void, not
> the string in the slot array on low-memory pressure?

Because we can't do any better?  Ok, second FIXME.

> Another problem is with an
> embedding that stores a jsval in thread-shared location. SpiderMonkey knows
> that all string jsvals must be made independent before such store, but this is
> very local knowledge and AFAICS is not stated anywhere.

Not so, see http://lxr.mozilla.org/mozilla/source/js/src/jsapi.h#1886 and the following lines' comments.

/be

Comment 15

[Igor Bukanov]

Attachment: Implementation v7

The version that addresses nits and uses "FIXME bug <number>:" in jslock.c comments.

Comment 16

[Igor Bukanov]

Attachment: Implementation v7b

v7 has a double blank in comments nicely exposed by bugzilla's interdiff. This version removes it.

Comment 17

[Igor Bukanov]

Ping: chance for review soon?

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 247815 [details] [diff] [review]
Implementation v7b

Looks good, sorry for the delay in r+'ing.

/be

Comment 19

[Igor Bukanov]

I committed the pacth from comments 16 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.293; previous revision: 3.292
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.230; previous revision: 3.229
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.172; previous revision: 3.171
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.190; previous revision: 3.189
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.306; previous revision: 3.305
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.56; previous revision: 3.55
done
Checking in jslock.c;
/cvsroot/mozilla/js/src/jslock.c,v  <--  jslock.c
new revision: 3.58; previous revision: 3.57
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.304; previous revision: 3.303
done
Checking in jsobj.h;
/cvsroot/mozilla/js/src/jsobj.h,v  <--  jsobj.h
new revision: 3.58; previous revision: 3.57
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.264; previous revision: 3.263
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.136; previous revision: 3.135
done
Checking in liveconnect/jsj_JavaObject.c;
/cvsroot/mozilla/js/src/liveconnect/jsj_JavaObject.c,v  <--  jsj_JavaObject.c
new revision: 1.42; previous revision: 1.41
done
Checking in xpconnect/src/xpcdebug.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcdebug.cpp,v  <--  xpcdebug.cpp
new revision: 1.16; previous revision: 1.15
done

Comment 20

[Igor Bukanov]

Attachment: Version of Implementation v7b patch with U=3

I attach the patch with smaller context chunks (-U 3) in case this will be considered for 1.8.1 branch to minimize the difference with the trunk.

It would be nice to avoid js_EqualString story when replacing js_CompareString with it in jsxml.c on trunk and 1.8.1 branch caused non-trivial merge efforts for almost each security patch touching jsxml.c to port them to 1.8.0.