Avoid explicitly referencing JSObject.slots

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Updated

13 years ago
Blocks: 331966
(Assignee)

Comment 1

13 years ago
Posted patch Implementation v1 (obsolete) — Splinter Review
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.
Attachment #247368 - Flags: review?(brendan)
Attachment #247368 - Attachment is obsolete: true
Attachment #247368 - Flags: review?(brendan)
(Assignee)

Comment 3

13 years ago
Posted patch Implementation v1 for real (obsolete) — Splinter Review
Attaching hopefully the properly generated file.
Attachment #247369 - Flags: review?(brendan)
(Assignee)

Comment 4

13 years ago
Posted patch Implementation v2 (obsolete) — Splinter Review
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.
Attachment #247369 - Attachment is obsolete: true
Attachment #247411 - Flags: review?(brendan)
Attachment #247369 - Flags: review?(brendan)
(Assignee)

Comment 5

13 years ago
Posted patch Implementation v3 (obsolete) — Splinter Review
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.
Attachment #247411 - Attachment is obsolete: true
Attachment #247423 - Flags: review?(brendan)
Attachment #247411 - Flags: review?(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
(Assignee)

Comment 7

13 years ago
Posted patch Implementation v4 (obsolete) — Splinter Review
The version with STO->STOBJ rename
Attachment #247423 - Attachment is obsolete: true
Attachment #247459 - Flags: review?(brendan)
Attachment #247423 - Flags: review?(brendan)
(Assignee)

Comment 8

13 years ago
Posted patch Implementation v4 for real (obsolete) — Splinter Review
I promise the next time to check the file before attaching it
Attachment #247459 - Attachment is obsolete: true
Attachment #247460 - Flags: review?(brendan)
Attachment #247459 - Flags: review?(brendan)
(Assignee)

Comment 9

13 years ago
(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.
(Assignee)

Comment 10

13 years ago
Posted patch Implementation v5 (obsolete) — Splinter Review
The patch update to switch few more places to use STOBJ_ macros.
Attachment #247460 - Attachment is obsolete: true
Attachment #247549 - Flags: review?(brendan)
Attachment #247460 - Flags: review?(brendan)
(Assignee)

Comment 11

13 years ago
Posted patch Implementation v6 (obsolete) — Splinter Review
Covering the remaining cases of explicit access to JSObject.slots in the liveconnect files.
Attachment #247549 - Attachment is obsolete: true
Attachment #247667 - Flags: review?(brendan)
Attachment #247549 - Flags: review?(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
(Assignee)

Comment 13

13 years ago
(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?
(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
(Assignee)

Comment 15

13 years ago
Posted patch Implementation v7 (obsolete) — Splinter Review
The version that addresses nits and uses "FIXME bug <number>:" in jslock.c comments.
Attachment #247667 - Attachment is obsolete: true
Attachment #247814 - Flags: review?(brendan)
Attachment #247667 - Flags: review?(brendan)
(Assignee)

Comment 16

13 years ago
v7 has a double blank in comments nicely exposed by bugzilla's interdiff. This version removes it.
Attachment #247814 - Attachment is obsolete: true
Attachment #247815 - Flags: review?(brendan)
Attachment #247814 - Flags: review?(brendan)
(Assignee)

Comment 17

13 years ago
Ping: chance for review soon?
Comment on attachment 247815 [details] [diff] [review]
Implementation v7b

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

/be
Attachment #247815 - Flags: review?(brendan) → review+
(Assignee)

Comment 19

13 years ago
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

13 years ago
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.

Updated

13 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.