387481 - jsatom.c must protect against mutations even with !JS_THREADSAFE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

[This is not applicable to a browser build as JS_THREADSAFE is always defined there.]

Consider the following fragment from js_AtomizeDouble in jsatom.c:

    hep = JS_HashTableRawLookup(table, keyHash, (void *)key);
    if ((he = *hep) == NULL) {
#ifdef JS_THREADSAFE
        uint32 gen = state->tablegen;
#endif
        JS_UNLOCK(&state->lock,cx);
        if (!js_NewDoubleValue(cx, d, &key))
            return NULL;
        JS_LOCK(&state->lock, cx);
#ifdef JS_THREADSAFE
        if (state->tablegen != gen) {
            hep = JS_HashTableRawLookup(table, keyHash, (void *)key);
            if ((he = *hep) != NULL) {
                atom = (JSAtom *)he;
                goto out;
            }
        }
#endif
        he = JS_HashTableRawAdd(table, hep, keyHash, (void *)key, NULL);

Here the code assumes that with !JS_THREADSAFE hep is always valid when JS_HashTableRawAdd. This is wrong as js_NewDoubleValue can trigger GC that can shrink and rehash the table.

A similar problem exists with js_AtomizeString.

Comment 1

[Igor Bukanov]

Attachment: patch v1

Fix for this bug and bug 389063 + atom state cleanup/refactoring to make patch for bug 386265 smaller:

1. The patch initialize/finish atom state in js_NewRuntime/js_DestroyRuntime to avoid counting live atoms and to remove JSAtomState.liveAtoms. 

2. The patch passes JSContext/JSRuntime to few atomState-related functions to remove JSAtomState.runtime.

3. To fix this bug JSAtomState.tablegen is defined and updated irrespective to JS_THREADSAFE.

4. To fix the bug 389063 the patch changes js_Atomize(String|Double) to jump to common lastAtom/flags initialization code.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> Created an attachment (id=273749) [details]
> patch v1

I will land this patch updating it if necessary after landing the fast native patch for bug 385393.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 273749 [details] [diff] [review]
patch v1

>+   /*
>+    * The caller must zero the state before calling the function.

s/the function/this function/

>+ * Initialize atom state. Return true on success, false when failed to

s/when failed/on failure/

>+ * allocate memory. The function must be called after js_InitGC. The caller
>+ * must call zero *state before calling the function and must call

s/the function/this function/

Suggest full stop after that, then "The caller must also call ...". Might even comment on exception requirement to call a finish function after an init function failure.

Is it worth cleaning up on partial init followed by failure, just to avoid this exception? I.e., call js_FinishAtomState from early failure returning code in js_InitAtomState?

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Is it worth cleaning up on partial init followed by failure, just to avoid this
> exception? I.e., call js_FinishAtomState from early failure returning code in
> js_InitAtomState?

The exception comes from the fact that js_NewRuntime calls js_DestroyRuntime on any failure. Thus js_FinishAtomState can be called on a partially initialized state. But it also points to a bug in the patch: js_FinishAtomState can also be called on unitialized state so it can not unconditionally free the lock.

Comment 5

[Igor Bukanov]

Recording patch dependency.

Comment 6

[Igor Bukanov]

Attachment: v2

This is a backup of the fixed version patych version that includes the fix for bug 390348.

Comment 7

[Igor Bukanov]

Attachment: v3

The new version fixes the initialization issue, addresses the nits, moves atomState to the end of JSRuntime so a compiler can access more frequently accessed fields using shorter offsets. I also changes js_InitCommonAtoms to use a table to map atom into atom names to shrink the code.

Comment 8

[Igor Bukanov]

Attachment: v3c

Fixing English grammar in comments

Comment 9

[Igor Bukanov]

Attachment: v4

The new version fixes toLocaleStringAtom ordering bug in js_InitCommonAtoms. The caused a few regressions as JSAtomState.toSource started to mean "toLocaleString".

Comment 10

[Igor Bukanov]

Comment on attachment 274916 [details] [diff] [review]
v4

This is a fix+atom state cleanup/optimizations.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 274916 [details] [diff] [review]
v4

>+    /*
>+     * Names for atoms starting from JSAtomState.booleanAtoms until
>+     * JSAtomState.lazy.
>+     */
>+    JS_STATIC_ASSERT(JSTYPE_LIMIT == 8);
>+    static const char * atomNames[] = {

Nit: no space after the *.

Do we want a jsatom.tbl file to consolidate the names, string-izing and token-pasting via macros when including that .tbl file? Now or followup bug, either is good.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> Do we want a jsatom.tbl file to consolidate the names, string-izing and
> token-pasting via macros when including that .tbl file? 

Using atom.tbl surely reduces the number of source lines. The problem is to define C strings shared with jskeyword.tbl only once given that not all keywords are used as common atoms. An alternative is to define all C strings in a single table but I am not sure that it would be worth it.

Comment 13

[Igor Bukanov]

Attachment: implementation v5

The new version uses a single table to define all common atoms. To share the strings with js_type_strs the latter is defines as a pointer and an array so it can be initialized via:

const char *const *const js_type_strs = &common_atom_names[1];

This requires extra indirection code that a compiler now has to generate to access js_type_strs in couple of non-debug places. But this overhead is significantly less than saving coming from using the single loop in js_InitAtoms.

The patch also adds js_undefined_str so jsxml.c/jsinterp.c can use the strings directly rather than getting them via js_type_strs.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 275405 [details] [diff] [review]
implementation v5

>+ * The elements of the array after the first empty string also define strings
>+ * corresponding to JSType enum from jspubtd.h. The following assert insists

s/enum/enums/ or even s/enum/enumerators/.

>+ * that JSType defines exactly 8 types.
>+ */
>+JS_STATIC_ASSERT(JSTYPE_LIMIT == 8);
>+static const char *const common_atom_names[] = {
>+    "",                         /* emptyAtom                    */
>+    js_undefined_str,           /* typeAtoms[JSTYPE_VOID]       */

Do you want another assertion that the first type is at index 1?

>+extern const char   *const * const js_type_strs;

Nit: no space between * and const.

/be

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> (From update of attachment 275405 [details] [diff] [review])
> >+ * The elements of the array after the first empty string also define strings
> >+ * corresponding to JSType enum from jspubtd.h. The following assert insists
> 
> s/enum/enums/ or even s/enum/enumerators/.

I am kindly asking for an English lesson here: why it is necessary to use plural "enumerators" here? Is it because members of C enum can be called "enumerators"?

Comment 16

[Brendan Eich [:brendan]]

"... define strings corresponding to ... enumerators" -- plural strings, and they correspond one to one, so the object of "corresponding to" should be plural too.

/be

Comment 17

[Igor Bukanov]

Attachment: implementation v6

Asking for a review and approval one more time. The new patch just exposes js_common_atom_names as extern array and uses JS_TYPE_STR/JS_BOOLEAN_STR macros to access the portion of it containing type names and booleans.

Comment 18

[Igor Bukanov]

Bugzilla's interdiff shows the right difference between the v5 and v6 patches despite the warning about different versions.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 275489 [details] [diff] [review]
implementation v6

Tight, I dig it.

>+ * The elements of the array after the first empty string also define strings
>+ * corresponding to JSType enumerators from jspubtd.h. The following assert
>+ * insists that JSType defines exactly 8 types.
>+ */
>+JS_STATIC_ASSERT(JSTYPE_LIMIT == 8);
>+const char *const js_common_atom_names[] = {
>+    "",                         /* emptyAtom                    */
>+    js_undefined_str,           /* typeAtoms[JSTYPE_VOID]       */
>+    js_object_str,              /* typeAtoms[JSTYPE_OBJECT]     */
>+    js_function_str,            /* typeAtoms[JSTYPE_FUNCTION]   */
>+    "string",                   /* typeAtoms[JSTYPE_STRING]     */
>+    "number",                   /* typeAtoms[JSTYPE_NUMBER]     */
>+    "boolean",                  /* typeAtoms[JSTYPE_BOOLEAN]    */
>+    js_null_str,                /* typeAtoms[JSTYPE_NULL]       */
>+    "xml",                      /* typeAtoms[JSTYPE_XML]        */
>+    js_false_str,               /* booleanAtoms[0]              */
>+    js_true_str,                /* booleanAtoms[1]              */

Would like the comment to talk about the boolean strings too.

I didn't see an assertion (static or otherwise) about types and then boolean strings starting as a dense group at index 1, but I might have missed it.

>+    /*
>+     * From this point until the end of struct definition the struct must
>+     * contain only JSAtom fields. We use this to access the storage occupied
>+     * by the common atoms in js_FinishCommonAtoms.
>+     *
>+     * common_atom_names defined in jsatom.c contains C strings for atoms in
>+     * the order of atom fields here. Thus update the array when changing the
>+     * order.

js_common_atom_names

Instead of "Thus update ...", "Therefore you must update that array if you change member order here."

r/a=me, final patch for posterity is fine, no need to resolicit review.

/be

Comment 20

[Igor Bukanov]

(In reply to comment #19)
> I didn't see an assertion (static or otherwise) about types and then boolean
> strings starting as a dense group at index 1, but I might have missed it.

The assertions are in jsatom.h:

/*
 * Macros to access C strings for JSType and boolean literals together with
 * checks that type names and booleans starts from index 1 and 1+JSTYPE_LIMIT
 * correspondingly.
 */
#define JS_TYPE_STR(type)    (js_common_atom_names[1 + (type)])
#define JS_BOOLEAN_STR(type) (js_common_atom_names[1 + JSTYPE_LIMIT + (type)])

JS_STATIC_ASSERT(1 * sizeof(JSAtom *) ==
                 offsetof(JSAtomState, typeAtoms) - ATOM_OFFSET_START);
JS_STATIC_ASSERT((1 + JSTYPE_LIMIT) * sizeof(JSAtom *) ==
                 offsetof(JSAtomState, booleanAtoms) - ATOM_OFFSET_START);

Comment 21

[Igor Bukanov]

Attachment: v6b

the version to address the nits

Comment 22

[Igor Bukanov]

I checked in the patch from comment 21 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.160; previous revision: 3.159
done
Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.343; previous revision: 3.342
done
Checking in jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.105; previous revision: 3.104
done
Checking in jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.75; previous revision: 3.74
done
Checking in jsbool.c;
/cvsroot/mozilla/js/src/jsbool.c,v  <--  jsbool.c
new revision: 3.30; previous revision: 3.29
done
Checking in jscntxt.c;
/cvsroot/mozilla/js/src/jscntxt.c,v  <--  jscntxt.c
new revision: 3.117; previous revision: 3.116
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.157; previous revision: 3.156
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.232; previous revision: 3.231
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.365; previous revision: 3.364
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.368; previous revision: 3.367
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.259; previous revision: 3.258
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.165; previous revision: 3.164
done