560796 - Crash [@ DropWatchPointAndUnlock] or "Assertion failure: hasSetterValue() && rawSetter, at ../jsscope.h"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

__defineSetter__("x", function(){})
this.watch("x", "".localeCompare)
window = x
Object.defineProperty(this, "x", ({
    set: window
}))

(pass this into the shell as a CLI argument, e.g. ./js a.js)

asserts js debug shell on TM tip without -j at Assertion failure: hasSetterValue() && rawSetter, at ../jsscope.h:737 and crashes js opt shell on TM tip without -j at DropWatchPointAndUnlock

s-s for the moment because we don't 0-day nightly users.

autoBisecting soon...


Opt stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000004
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   js-opt-32-tm-darwin           	0x0002b391 DropWatchPointAndUnlock(JSContext*, JSWatchPoint*, unsigned int) + 209
1   js-opt-32-tm-darwin           	0x0002b4ba JS_ClearAllWatchPoints + 58
2   js-opt-32-tm-darwin           	0x0002279b js_DestroyContext(JSContext*, JSDestroyContextMode) + 123
3   js-opt-32-tm-darwin           	0x0000ee59 JS_DestroyContext + 25
4   js-opt-32-tm-darwin           	0x000050e3 DestroyContext(JSContext*, bool) + 99
5   js-opt-32-tm-darwin           	0x00008d80 main + 1104
6   js-opt-32-tm-darwin           	0x00002a7d _start + 208
7   js-opt-32-tm-darwin           	0x000029ac start + 40

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to bug 430133:

The first bad revision is:
changeset:   36651:766a6b2e74e7
user:        Jeff Walden
date:        Fri Jun 05 12:56:45 2009 -0700
summary:     Bug 430133 - Implement ES3.1's Object.defineProperty and Object.defineProperties.  r=jorendorff

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Waldo says assign to him on IRC..

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Using the technique in bug 562028 comment 10,

js> __defineSetter__("x", function(){})
js> this.watch("x", "".localeCompare)
js> window = x
js> Object.defineProperty(this, "x", ({
    set: window
}))
({window:(void 0)})
js> 
js> quit()


Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000004
0x0002b0f1 in DropWatchPointAndUnlock ()
(gdb) q
The program is running.  Exit anyway? (y or n) n
Not confirmed.
(gdb) bt
#0  0x0002b0f1 in DropWatchPointAndUnlock ()
#1  0x0002b21a in JS_ClearAllWatchPoints ()
#2  0x000224fb in js_DestroyContext ()
#3  0x0000eb49 in JS_DestroyContext ()
#4  0x00004dd3 in DestroyContext ()
#5  0x00008a70 in main ()
(gdb) x/i $eip
0x2b0f1 <_ZL23DropWatchPointAndUnlockP9JSContextP12JSWatchPointj+209>:  mov    0x4(%edx),%eax
(gdb) x/1b $edx
0x0:    Cannot access memory at address 0x0
(gdb) x/1b $eax
0x1:    Cannot access memory at address 0x1
(gdb)

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #3)
> (gdb) x/i $eip
> 0x2b0f1 <_ZL23DropWatchPointAndUnlockP9JSContextP12JSWatchPointj+209>:  mov   
> 0x4(%edx),%eax
> (gdb) x/1b $edx
> 0x0:    Cannot access memory at address 0x0
> (gdb) x/1b $eax
> 0x1:    Cannot access memory at address 0x1

Null deref - we seem to be moving the null value from register edx to another register eax.

Comment 5

[Brendan Eich [:brendan]]

Waldo, DefinePropertyObject makes inconsistent pairs of

 (getter, (attrs & JSPROP_GETTER)) and
 (setter, (attrs & JSPROP_SETTER))

where these are the locals that flow into the js_DefineProperty tail call. Suggest you clear JSPROP_GETTER from attrs if getter ends up equal to JS_PropertyStub, likewise for SETTER/setter.

It's hard to do this upstream of the assignment to attrs, since bits are being muxed from desc.attrs and sprop->attributes() according to the changed bitmask. Simpler to do it after.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: something like this

Comment 0 test passes with this applied.

/be

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 442023 [details] [diff] [review]
something like this

This will fail this testcase, which the defineProperty mega-test definitely exercises:

var o = Object.defineProperty({}, "foo", { get: undefined, set: undefined });
var d = Object.getOwnPropertyDescriptor(o, "foo");
assert("get" in d);
assert("set" in d);
assert(!("value" in d));
assert(!("writable" in d));

If it weren't for this case I'd be willing to make the change.  But with this case, any remove-bit-when-stubbed change is nugatory.

Comment 8

[Brendan Eich [:brendan]]

This may sound like a flame, but so be it:

I don't remember soliciting review. I threw a patch here to show how to undo the overt inconsistency in the data structures introduced by the implementation of DefinePropertyObject.

You are not "willing" to make this change, fine. How about fixing this seriously non-nugatory bug? Without adding extra the runtime overhead of more tests on every read of a getter or setter.

If ES5 needs a new state for undefined getter or setter, which is different from the SpiderMonkey under-the-hood stub JSPropertyOp getter or setter, then add one. Beware the freedom in C to merge identical stub functions.

/be

Comment 9

[Jeff Walden [:Waldo]]

In good time; I'm working on a number of different things besides this and simply haven't gotten to fixing this yet.

Comment 10

[Brendan Eich [:brendan]]

Attachment: ok, trying to be helpful since this is biting gary's fuzzing

I can take this bug to offload you if the patch is ok. I don't mind helping out since the novelty of get: undefined and/or set: undefined in ES5 is something I should have noticed earlier. It really does require a new (object, not property op) encoding, to avoid extra tests on internal reads of the getter or setter.

Simplest encoding seemed to be null rawGetter or rawSetter, since the inline methods mostly cope with a null pointer. The setterObject call in jsdbgapi.cpp had to change, or else setterObject's definition and its use here have to change. I.e., should the inline [gs]etterObject methods be changed to return null, and callers adjusted accordingly?

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: better try

Dangerous to rely on global class having JS_PropertyStub for its getProperty and setProperty, which the last patch did. Too easy to pass in the shell.

/be

Comment 12

[Brendan Eich [:brendan]]

Apologies to Waldo if he was already all over this. The bug is only a null +4 pointer dereference.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: ok, this eliminates the abuse of JS_PropertyStub as a JSObject pointer

Calling js_Define*Property with JSPROP_GETTER and/or JSPROP_SETTER in attrs but getter and/or setter set to JS_PropertyStub relied on NormalizeGetterAndSetter to convert these completely invalid function object pointer values to NULL.

Trying to fix this by passing NULL for getter and/or setter when calling js_Define*Property with JSPROP_GETTER | JSPROP_SETTER in attrs fell into the logic that takes NULL to mean "default to clasp->[gs]etProperty", which in the js shell would be JS_PropertyStub, which NormalizeGetterAndSetter encoded as NULL, which saved us from crashing trying to js_InternalGetOrSet a C function pointer instead of a JS function object.

This patch fixes the whole mess by defaulting NULL to clasp->[gs]etProperty only if !(attrs & (JSPROP_GETTER | JSPROP_SETTER)) (and no JSDNP_SETMETHOD as before), and passing NULL when desc.has[GS]et in DefinePropertyObject.

/be

Comment 14

[Brendan Eich [:brendan]]

The whole mess is the latent bug in js_DefineNativeProperty, which ES5 extensions fell into. Again my fault for not advising Waldo earlier; he'd mentioned the NULL turning into clasp->[gs]etProperty defaulting but I hadn't realized it was biting the user-defined getter/setter path from Object.defineProperty.

/be

Comment 15

[Brendan Eich [:brendan]]

To finish the story: originally there were no user-defined getters or setters, so no JSPROP_[GS]ETTER. NULL to JS_DefineProperty, etc. meant "default to the class getProperty or setProperty".

Then came user-defined getters and setters, which are function object pointers punned as JSPropertyOp C function pointers in sprop->[gs]etter, along with JSPROP_[GS]ETTER to discriminate the union (or the pun -- we could use the union added recently for freelist linkage). But it was not legal to use a null function object pointer as a user-defined getter.

Then came ES5 Object.defineProperty, etc., which due to {get: undefined, set: undefined}, need that new state: JSPROP_[GS]ETTER in attrs but null raw[GS]etter.

Problem was persuading the layers to pass null through in this case instead of defaulting (and then, if you were lucky about object class [gs]etProperty being JS_PropertyStub) undoing the defaulting and restoring null -- or not (crash or assertbotch), as described in comment 13 and comment 14.

/be

Comment 16

[Brendan Eich [:brendan]]

The subtlety (the good kind) of this new-with-ES5 code is worth commenting on:

        /* 8.12.9 step 12. */
        uintN changed = 0;
        if (desc.hasConfigurable)
            changed |= JSPROP_PERMANENT;
        if (desc.hasEnumerable)
            changed |= JSPROP_ENUMERATE;
        if (desc.hasGet)
            changed |= JSPROP_GETTER | JSPROP_SHARED;
        if (desc.hasSet)
            changed |= JSPROP_SETTER | JSPROP_SHARED;

        attrs = (desc.attrs & changed) | (sprop->attributes() & ~changed);
        if (desc.hasGet)
            getter = desc.getterObject() ? desc.getter() : NULL;
        else
            getter = sprop->hasDefaultGetter() ? JS_PropertyStub : sprop->getter();
        if (desc.hasSet)
            setter = desc.setterObject() ? desc.setter() : NULL;
        else
            setter = sprop->hasDefaultSetter() ? JS_PropertyStub : sprop->setter();

The else clauses (when !desc.has[GS]et) use conditional ?: expressions to select JS_PropertyStub (a C function pointer, correct) or sprop->[gs]etter(), an inline method that merely returns sprop->raw[GS]etter.

What if (sprop->attrs & JSPROP_[GS]ETTER)? Then because we're in the else clause, changed will *not* include that JSPROP_[GS]ETTER attribute, so attrs will preserve the bit from sprop->attributes() & ~changed, and the function pointer will propagate to [gs]etter. If !(sprop->attrs & JSPROP_[GS]ETTER), the raw C JSPropertyOp pointer will propagate.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: Avoid unnecessary test in ES5 logic

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #16)
>         if (desc.hasGet)
>             changed |= JSPROP_GETTER | JSPROP_SHARED;
>         if (desc.hasSet)
>             changed |= JSPROP_SETTER | JSPROP_SHARED;
> 
>         attrs = (desc.attrs & changed) | (sprop->attributes() & ~changed);
>         if (desc.hasGet)
>             getter = desc.getterObject() ? desc.getter() : NULL;
>         else
>             getter = sprop->hasDefaultGetter() ? JS_PropertyStub :
> sprop->getter();
>         if (desc.hasSet)
>             setter = desc.setterObject() ? desc.setter() : NULL;
>         else
>             setter = sprop->hasDefaultSetter() ? JS_PropertyStub :
> sprop->setter();
> 
> The else clauses (when !desc.has[GS]et) use conditional ?: expressions to
> select JS_PropertyStub (a C function pointer, correct) or sprop->[gs]etter(),
> an inline method that merely returns sprop->raw[GS]etter.
> 
> What if (sprop->attrs & JSPROP_[GS]ETTER)? Then because we're in the else
> clause, changed will *not* include that JSPROP_[GS]ETTER attribute, so attrs
> will preserve the bit from sprop->attributes() & ~changed, and the function
> pointer will propagate to [gs]etter. If !(sprop->attrs & JSPROP_[GS]ETTER), the
> raw C JSPropertyOp pointer will propagate.

Oops, but (anyone still with me?) if sprop->has[GS]etterValue() in the else clause, and !sprop->hasDefault[GS]etter(), then we select JS_PropertyStub but attrs contains JSPROP_[GS]ETTER (changed does not, sprop->attributes() does). So we are once again relying on NormalizeGetterAndSetter way down in jsscope.cpp to encode JS_PropertyOp as NULL, so we can read back NULL as a function object ptr.

This is still a type confusion. It happens to "work", but I'd rather assert in NormalizeGetterAndSetter that if a getter or setter is JS_PropertyStub, then attrs does *not* contain the corresponding JSPROP_[GS]ETTER bit. Next patch...

/be

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #18)
> (In reply to comment #16)
> >         if (desc.hasGet)
> >             changed |= JSPROP_GETTER | JSPROP_SHARED;
> >         if (desc.hasSet)
> >             changed |= JSPROP_SETTER | JSPROP_SHARED;
> > 
> >         attrs = (desc.attrs & changed) | (sprop->attributes() & ~changed);
> >         if (desc.hasGet)
> >             getter = desc.getterObject() ? desc.getter() : NULL;
> >         else
> >             getter = sprop->hasDefaultGetter() ? JS_PropertyStub :
> > sprop->getter();
> >         if (desc.hasSet)
> >             setter = desc.setterObject() ? desc.setter() : NULL;
> >         else
> >             setter = sprop->hasDefaultSetter() ? JS_PropertyStub :
> > sprop->setter();
[snip]

> Oops, but (anyone still with me?) if sprop->has[GS]etterValue() in the else
> clause, and !sprop->hasDefault[GS]etter(), then we select JS_PropertyStub but

Typo: "and sprop->hasDefault[GS]etter()", no "!" there.

> attrs contains JSPROP_[GS]ETTER (changed does not, sprop->attributes() does).
> So we are once again relying on NormalizeGetterAndSetter way down in
> jsscope.cpp to encode JS_PropertyOp as NULL, so we can read back NULL as a
> function object ptr.
> 
> This is still a type confusion. It happens to "work", but I'd rather assert in
> NormalizeGetterAndSetter that if a getter or setter is JS_PropertyStub, then
> attrs does *not* contain the corresponding JSPROP_[GS]ETTER bit. Next patch...

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: Fix remaining JSPropertyOp vs. (null) JSObject* type confusion

The assertions in NormalizeGetterAndSetter paid off, showing an early tail call to js_DefineProperty that passed JS_PropertyStub as a getter or setter where the corresponding JSPROP_[GS]ETTER bit was set in attrs -- fixed to pass NULL.

En passant nit-pick: DefineProperty{Object,Array} names have prepositions now, so "Property" no longer reads like a modifier on {Object,Array}.

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: Defaulting in js_DefineNativeProperty must be case by case

This came to me in a dream...

/be

Comment 22

[Brendan Eich [:brendan]]

Inline interdiff (why does bugzilla's fail so often?):

diff -u b/js/src/jsobj.cpp b/js/src/jsobj.cpp
--- b/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -4256,10 +4256,10 @@
 
     /* Use the object's class getter and setter by default. */
     clasp = obj->getClass();
-    if (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)) && !(defineHow & JSDNP_SET_METHOD)) {
-        if (!getter)
+    if (!(defineHow & JSDNP_SET_METHOD)) {
+        if (!getter && !(attrs & JSPROP_GETTER))
             getter = clasp->getProperty;
-        if (!setter)
+        if (!setter && !(attrs & JSPROP_SETTER))
             setter = clasp->setProperty;
     }
 
/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: full logic minimization, use unions in JSScopeProperty, js::cleanup

js_CastAs* -> js::CastAs*

IsWatchedProperty can use sprop->setterObject() now that that method no longer asserts non-null setter (getterObject does not, and the assert only complicates code and breaks symmetry unnecessarily).

DefinePropertyOnObject had too many ?: expressions of the form:

-                                 desc.getterObject() ? desc.getter() : NULL,

that can be just desc.getter(), since that is defined as

    JSPropertyOp getter() const {
        return js::CastAsPropertyOp(getterObject());
    }

and PropertyDescriptor::getterObject is

    JSObject* getterObject() const {
        return (get != JSVAL_VOID) ? JSVAL_TO_OBJECT(get) : NULL;
    }

Likewise for setter.

Same reduction for

-                              sprop->hasGetterValue() ? sprop->getterValue() : JSVAL_VOID

which is now sprop->getterValue() (this is in code that asserts or tests to ensure that sprop->isAccessorProperty()). Likewise for setterValue().

Add getterObj and setterObj arms to the anonymous unions around rawGetter and rawSetter in JSScopeProperty, eliminating some casts and compressing code. We rely on pointers to functions and pointers to objects representing null using the same bits, as elsewhere.

Restore JSScopeProperty::[gs]etterObject symmetry, setterObject no longer asserts rawSetter is non-null.

/be

Comment 24

[Brendan Eich [:brendan]]

Attachment: interdiff of last two attachments (bugzilla interdiff fails again)

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 442767 [details] [diff] [review]
full logic minimization, use unions in JSScopeProperty, js::cleanup

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>@@ -2241,24 +2239,22 @@ DefinePropertyObject(JSContext *cx, JSOb
> 
>     JSScopeProperty *sprop = reinterpret_cast<JSScopeProperty *>(current);
>     do {
>         if (desc.isAccessorDescriptor()) {
>             if (!sprop->isAccessorDescriptor())
>                 break;
> 
>             if (desc.hasGet &&
>-                !js_SameValue(desc.getterValue(), 
>-                              sprop->hasGetterValue() ? sprop->getterValue() : JSVAL_VOID, cx)) {
>+                !js_SameValue(desc.getterValue(), sprop->getterValue(), cx)) {
>                 break;
>             }
> 
>             if (desc.hasSet &&
>-                !js_SameValue(desc.setterValue(),
>-                              sprop->hasSetterValue() ? sprop->setterValue() : JSVAL_VOID, cx)) {
>+                !js_SameValue(desc.setterValue(), sprop->setterValue(), cx)) {
>                 break;

These conditions both fit on one 99-wide line, and we can even kill braces now.


>         /* 8.12.9 step 11. */
>         JS_ASSERT(desc.isAccessorDescriptor() && sprop->isAccessorDescriptor());
>         if (!sprop->configurable()) {
>             if ((desc.hasSet &&
>-                 !js_SameValue(desc.setterValue(),
>-                               sprop->hasSetterValue() ? sprop->setterValue() : JSVAL_VOID,
>-                               cx)) ||
>+                 !js_SameValue(desc.setterValue(), sprop->setterValue(), cx)) ||
>                 (desc.hasGet &&
>-                 !js_SameValue(desc.getterValue(),
>-                               sprop->hasGetterValue() ? sprop->getterValue() : JSVAL_VOID, cx)))
>+                 !js_SameValue(desc.getterValue(), sprop->getterValue(), cx)))
>             {

More don't-wrap-because-99-is-bignum lines.


>-        if (desc.hasGet)
>-            getter = desc.getterObject() ? desc.getter() : JS_PropertyStub;
>-        else
>-            getter = sprop->hasDefaultGetter() ? JS_PropertyStub : sprop->getter();
>-        if (desc.hasSet)
>-            setter = desc.setterObject() ? desc.setter() : JS_PropertyStub;
>-        else
>-            setter = sprop->hasDefaultSetter() ? JS_PropertyStub : sprop->setter();
>+        if (desc.hasGet) {
>+            getter = desc.getter();
>+        } else {
>+            getter = (sprop->hasDefaultGetter() && !sprop->hasGetterValue())
>+                     ? JS_PropertyStub
>+                     : sprop->getter();
>+        }
>+        if (desc.hasSet) {
>+            setter = desc.setter();
>+        } else {
>+            setter = (sprop->hasDefaultSetter() && !sprop->hasSetterValue())
>+                     ? JS_PropertyStub
>+                     : sprop->setter();
>+        }

Purty.


>diff --git a/js/src/jsprvtd.h b/js/src/jsprvtd.h

> struct PropertyCacheEntry;
>+
>+static inline JSPropertyOp
>+CastAsPropertyOp(JSObject *object)
>+{
>+    return JS_DATA_TO_FUNC_PTR(JSPropertyOp, object);
>+}
> } /* namespace js */

Blank line before close of the namespace.

> /* Common instantiations. */
> typedef js::Vector<jschar, 32> JSCharBuffer;
> 
>-static inline JSPropertyOp
>-js_CastAsPropertyOp(JSObject *object)
>-{
>-    return JS_DATA_TO_FUNC_PTR(JSPropertyOp, object);
>-}
>-
> } /* export "C++" */
> #endif  /* __cplusplus */

...like we see here for a similar-ish block.


>diff --git a/js/src/jsscope.h b/js/src/jsscope.h

> /*
>  * Helpers for reinterpreting JSPropertyOp as JSObject* for scripted getters
>  * and setters.
>  */
>+namespace js {
> inline JSObject *
>-js_CastAsObject(JSPropertyOp op)
>+CastAsObject(JSPropertyOp op)

Newline after namespace js { too...

> {
>     return JS_FUNC_TO_DATA_PTR(JSObject *, op);
> }
> 
> inline jsval
>-js_CastAsObjectJSVal(JSPropertyOp op)
>+CastAsObjectJSVal(JSPropertyOp op)
> {
>     return OBJECT_TO_JSVAL(JS_FUNC_TO_DATA_PTR(JSObject *, op));
> }
>+} /* namespace js */
> 
> namespace js {
> class PropertyTree;
> }

...or just open this namespace earlier?  (Plus add a newline after the class declaration -- we only omit the blank lines when the namespace is enclosing only one or two declarations without inline definitions.)


>     {
>         JS_ASSERT_IF(getter && (attrs & JSPROP_GETTER),
>-                     JSVAL_TO_OBJECT(getterValue())->isCallable());
>+                     getterObj->isCallable());
>         JS_ASSERT_IF(setter && (attrs & JSPROP_SETTER),
>-                     JSVAL_TO_OBJECT(setterValue())->isCallable());
>+                     setterObj->isCallable());
>     }

Make these single-line.


>+    // Per ES5, decode null getterObj as the undefined value, which encodes as null.
>+    // Per ES5, decode null setterObj as the undefined value, which encodes as null.

Are we really starting to use C++-style single line comments outside the tracer wasteland?

Comment 26

[Brendan Eich [:brendan]]

Attachment: patch to commit

Had to put back four ?: expressions to avoid assert-botching, where the inline containing the assert duplicates the ?: in the caller. Optimized builds collapse the redundant test (I checked).

Waldo, I did something that I think is strictly better style in restoring these ?: expressions. They make lines long again, so I restored the layout you used, except that there's a trailing "..., cx)" actual param, sometimes at the far end of the line, in other cases (one at least) on its own line. I put all such on their own lines. It's more regular and readable.

Thanks for the namespace nits -- some ugliness creeping in there (and a missing extern "C" {...} to boot).

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: interdiff of last two patches (bugzilla interdiff fails again)

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 442824 [details] [diff] [review]
interdiff of last two patches (bugzilla interdiff fails again)

>diff -u b/js/src/jsprvtd.h b/js/src/jsprvtd.h

>@@ -91,10 +91,14 @@
> #ifdef __cplusplus
> 
> /* Class and struct forward declarations in namespace js. */
>+extern "C++" {
> namespace js {
>+
> struct Parser;
> struct Compiler;
>-}
>+
>+} /* namespace js */
>+} /* export "C++" */

This was the one exception to blank-lines-near-namespace-stuff: when it just encloses a (small) sequence of forward declarations each of which fits on a single line, needing no commenting.  Aside from the blank lines inserted here, all else looks great.

Comment 29

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/e365ccc7be1d

/be

Comment 30

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e365ccc7be1d

Comment 32

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929