js_FillPropertyCache is called with garbage-collected pobj

VERIFIED FIXED

Status

()

defect
P1
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({fixed1.8.1.24, verified1.9.0.11, verified1.9.1})

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.11 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs approval/landing for 1.8 branch][sg:critical?] fixed-in-tracemonkey)

Attachments

(6 attachments, 13 obsolete attachments)

7.62 KB, text/plain
Details
22.30 KB, text/plain
Details
1.34 KB, patch
brendan
: review+
Details | Diff | Splinter Review
1.44 KB, patch
Details | Diff | Splinter Review
2.39 KB, text/plain
Details
817 bytes, patch
Details | Diff | Splinter Review
Assignee

Description

10 years ago
The current code, when calls js_FillPropertyCache for properties found on prototype chains, does not detect that a setter or getter could have deleted the prototype and GC-ed it. This would cause js_FillPropertyCache to access GC-ed objects. Here is an example that demonstrates this:

@watson~/m/tm/js/src> cat ~/s/x1.js
var obj = { __proto__: { get a() { this.__proto__ = null; gc(); return 0; }}};

obj.a;

@watson~/m/tm/js/src> ~/build/js32.tm.dbg/js ~/s/x1.js
before 20480, after 20480, break 08262000
Segmentation fault

This bug exists on 1.9.* branches and may (or may not) be related to the bug 484042.
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?
Assignee

Comment 1

10 years ago
I wonder why js_FillPropertyCache is called after the code calls js_NativeGet or js_NativeSet that could have invalidate the property cache and collect objects?
Priority: -- → P1
Assignee

Comment 2

10 years ago
Another problem with the current code comes from patterns like

js_NativeGet(cx, obj, obj2, sprop, vp)
OBJ_DROP_PROPERTY(cx, obj2, prop);

With examples from the comment 0 this is unsound as obj2 can be GC-ed in js_NativeGet.
Assignee: general → igor
Assignee

Comment 3

10 years ago
Posted patch v1 (obsolete) — Splinter Review
Here is a minimal fix that should require minimal 1.9.0 porting efforts. It does 2 things:

1. Root pobj in js_NativeGet. This missing rooting exists on 181 branch as well. It is just sad that I have missed the need for that after all those changes to root sprop in js_NativeGet some time ago. 

2. Change js_FillPropertyCache callers to invoke the function before any getter or setter has a chance to run.

I will do some testing before asking for a review.
Assignee

Updated

10 years ago
Flags: blocking1.8.1.next?
Assignee

Comment 4

10 years ago
Comment on attachment 371452 [details] [diff] [review]
v1

The patch passes shell tests and try server.
Attachment #371452 - Flags: review?(brendan)

Updated

10 years ago
Summary: js_FillPropertyCache is called with garabge-collected pobj → js_FillPropertyCache is called with garbage-collected pobj
We want to remove settable proto (resettable, I should say: initializers should be able to preset), in which happy day the code added here should be removed.

js_FillPropertyCache is called after because it seemed best not to fill after an error from the getter.

Did you consider avoiding tvr2 by extending the way propertyRemovals is bumped and testing it earlier?

/be
Assignee

Comment 6

10 years ago
(In reply to comment #5)
> js_FillPropertyCache is called after because it seemed best not to fill after
> an error from the getter.

Such fill-after-getter is wrong since the getter can run GC invalidating all shape values.

> 
> Did you consider avoiding tvr2 by extending the way propertyRemovals is bumped
> and testing it earlier?

When propertyRemovals is bumped, the code does not know if it was bumped due to removal of sprop or some other property on this or another object. So this would not help the GC safety of sprop and pobj.

To optimize the rooting I considered adding a specialized TVR to root both sprop and pobj in one step. But that requires quite a few extra lines of code and I wanted to have a minimal patch for simpler porting to branches.
(In reply to comment #6)
> (In reply to comment #5)
> > js_FillPropertyCache is called after because it seemed best not to fill after
> > an error from the getter.
> 
> Such fill-after-getter is wrong since the getter can run GC invalidating all
> shape values.

We want to leave live shape values alone during GC if we can, but that is a bug for another day.

But the shapes will not be invalid in the objects, and js_FillPropertyCache uses these regenerated shapes when filling to prime the cache for a future hit assuming no GC between fill and future hit. So I do not see how GC invalidating the shapes at the time the getter is called breaks anything used to fill the cache for future hits after the getter unwinds.

The GC safety bug is separate.

> When propertyRemovals is bumped, the code does not know if it was bumped due to
> removal of sprop or some other property on this or another object.

The idea (rename it getterHazards or some such) is that any change from sample means a (rare) bad thing happened, and js_NativeGet should cope. But this does not help its caller avoid using pobj/obj2, of course. We'd have to extend the API to js_NativeGet somehow.

/be
Assignee

Comment 8

10 years ago
(In reply to comment #7)
> But the shapes will not be invalid in the objects, and js_FillPropertyCache
> uses these regenerated shapes when filling to prime the cache for a future hit
> assuming no GC between fill and future hit.

This is not true. js_FillPropertyCache uses the passed kshape when the property comes from the prototype. It does not read the regenerated shape. If the function is called after the getter, that shape may not match the real value of the shape.

> The idea (rename it getterHazards or some such) is that any change from sample
> means a (rare) bad thing happened, and js_NativeGet should cope.

SpiderMonkey has not yet include a function called ungc(). Until such function would be provided js_NativeGet could not cope with sporadically GC-ed property objects.

>  We'd have to extend the API to js_NativeGet somehow.

Just consider why js_NativeGet needs to root both sprop and pobj. This is just to execute LOCKED_OBJ_SET_SLOT(pobj, slot, *vp):

    JS_LOCK_SCOPE(cx, scope);
    JS_ASSERT(scope->object == pobj);
    if (SLOT_IN_SCOPE(slot, scope) &&
        (JS_LIKELY(cx->runtime->propertyRemovals == sample) ||
         SCOPE_GET_PROPERTY(scope, sprop->id) == sprop)) {
        LOCKED_OBJ_SET_SLOT(pobj, slot, *vp);
    }

I could understand the compatibility reasoning why this SET_SLOT may be necessary when pobj == obj. But I fail to see why for a getter comming from a prototype the code needs to store its result inside the pobj and not in the object itself. As the example shows, pobj may even no longer be a prototype for obj!

So if anything, the API should be changed so remove that need for LOCKED_OBJ_SET_SLOT. After that one no longer would needs to lock the scope second time or root pobj or scope.
Assignee

Comment 9

10 years ago
As a consequence of that caching-aftter-(getter|setter) js_FillPropertyCache needs to explicitly check if the sprop is still in the scope:

    /*                                                                                                                                                             
     * Check for fill from js_SetPropertyHelper where the setter removed sprop                                                                                     
     * from pobj's scope (via unwatch or delete, e.g.).                                                                                                            
     */
    scope = OBJ_SCOPE(pobj);
    JS_ASSERT(scope->object == pobj);
    if (!SCOPE_HAS_PROPERTY(scope, sprop)) {
        PCMETER(cache->oddfills++);
        *entryp = NULL;
        return;
    }

This de-optimizes the fill for most getters and setters that typically do not remove any properties from the scope. Another problem is the need to re-calculate protoIndex.

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+
(In reply to comment #8)
> (In reply to comment #7)
> > But the shapes will not be invalid in the objects, and js_FillPropertyCache
> > uses these regenerated shapes when filling to prime the cache for a future hit
> > assuming no GC between fill and future hit.
> 
> This is not true. js_FillPropertyCache uses the passed kshape when the property
> comes from the prototype. It does not read the regenerated shape. If the
> function is called after the getter, that shape may not match the real value of
> the shape.

Good point -- but bear with me here, if we have a way to detect removals/clear-scope/gc then we could reload kshape too. I'm wrong about the current code but thinking about a possible future that avoids too much manual rooting for a rare case (which in part -- __proto__ setting -- will go away, I think in the next release).

> > The idea (rename it getterHazards or some such) is that any change from sample
> > means a (rare) bad thing happened, and js_NativeGet should cope.
> 
> SpiderMonkey has not yet include a function called ungc(). Until such function
> would be provided js_NativeGet could not cope with sporadically GC-ed property
> objects.

I'm suggesting the API to js_NativeGet change such that the caller can know to bail (not fill, not unlock a dangling pobj). We have similar optimizations and asymmetric calling conventions, some recently engineered by you IIRC. Pessimal rooting and locking for symmetry's sake is best in public APIs, but we are deep in the belly of the beast here.

> >  We'd have to extend the API to js_NativeGet somehow.
> 
> Just consider why js_NativeGet needs to root both sprop and pobj. This is just
> to execute LOCKED_OBJ_SET_SLOT(pobj, slot, *vp):
> 
>     JS_LOCK_SCOPE(cx, scope);
>     JS_ASSERT(scope->object == pobj);
>     if (SLOT_IN_SCOPE(slot, scope) &&
>         (JS_LIKELY(cx->runtime->propertyRemovals == sample) ||
>          SCOPE_GET_PROPERTY(scope, sprop->id) == sprop)) {
>         LOCKED_OBJ_SET_SLOT(pobj, slot, *vp);
>     }
> 
> I could understand the compatibility reasoning why this SET_SLOT may be
> necessary when pobj == obj.

That's the only good reason. The rest is ancient history.

> But I fail to see why for a getter coming from a
> prototype the code needs to store its result inside the pobj and not in the
> object itself.

Indeed this is an old design flaw, which led to entrainment and pigeon-hole problems in proto-slots, which begat JSPROP_SHARED. We should try to get rid of it now, if we can -- even in a patch for this bug.

> As the example shows, pobj may even no longer be a prototype for
> obj!

That is only due to settable __proto__. You're right about that particular hazard but we want to remove it, so we should bias away from it.

> So if anything, the API should be changed so remove that need for
> LOCKED_OBJ_SET_SLOT. After that one no longer would needs to lock the scope
> second time or root pobj or scope.

Agreed!

/be
Assignee

Comment 11

10 years ago
(In reply to comment #10)
> Indeed this is an old design flaw, which led to entrainment and pigeon-hole
> problems in proto-slots, which begat JSPROP_SHARED. We should try to get rid of
> it now, if we can -- even in a patch for this bug.

This bug exists on 1.9.0 (and on 1.8.1). So before doing any optimization that could break things I would like to have a minimal patch that could land with minimal porting efforts on branches.

Given that, consider that if we can drop that slot assignment for the pobj != obj case, then there would be no need to lock pobj the second time (assuming that the cache fill is done before calling the getter as the current patch suggests). It would remove the need to root pobj.

And if we can remove slot's assignment for pobj == obj, then rooting of sprop can also be removed. But this is for another bug.
Assignee

Comment 12

10 years ago
(In reply to comment #11)
> This bug exists on 1.9.0 (and on 1.8.1). So before doing any optimization that
> could break things I would like to have a minimal patch that could land with
> minimal porting efforts on branches.

On the second thought, given how bad is this bug (call through a function pointer on GC-ed object in OBJ_DROP_PROPERTY), it could be dangerous to leak any information about it. A minimal patch for a restricted bug landing on the trunk could in theory provide quite a strong hint where to look for a bug on 1.9.0. An optimization patch for an open 1.9.1 bug would reveal nothing. 

But I could be too paranoid about this.
Between paranoia and desire for optimization to stay competitive, I am hoping you will apply some of that patented Igor case analysis goodness, tout suite!

/be
Assignee

Comment 14

10 years ago
Posted patch v2 (obsolete) — Splinter Review
The new version of the patch removes extra validating code from js_FillPropertyCache that SCOPE_HAS_PROPERTY(scope, sprop). It is possible since now js_FillPropertyCache is always called before the object is unlocked and any setter or getter are run. It also fixes the premature shape access issues from the bug 487846 - that was trivial to do since code always passes OBJ_SHAPE(obj) for kshape argument where obj is the second argument to the function.

These two changes are more then enough to offset any slowdown coming from the extra rooting in js_NativeGet. But there are more opportunities to optimize even without semantic changes proposed at the end of the comment 8. 

The slot writing code in js_Native(Get|Set) is only need to run for non-shared getters. Thus for shared getters the code can already skip the need to lock the object again after the getter returns. But this implies changing js_Native(Get|Set) semantic to always return unlocked object. It would make the patch much bigger and not suitable for 1.9.0 porting. Hence I give this patch for references.
Attachment #371452 - Attachment is obsolete: true
Attachment #371452 - Flags: review?(brendan)
Assignee

Comment 15

10 years ago
Posted patch v3 (base) (obsolete) — Splinter Review
The new version fixes bogus changes for kshape calculation in  js_FillPropertyCache.
Attachment #372200 - Attachment is obsolete: true
Assignee

Comment 16

10 years ago
Comment on attachment 372204 [details] [diff] [review]
v3 (base)

Asking for a review of this part - other changes to skip extra locking for shared getters or setters require that properties are cached before calling them.
Attachment #372204 - Flags: review?(brendan)
Assignee

Comment 17

10 years ago
Posted patch v3 (avoiding extra locking) (obsolete) — Splinter Review
This new patch skips the need to take the scope look the second time for shared getters and setters. This changes the semantic of js_Native(Get|Set) to always unlock the scope on return.
Attachment #372204 - Attachment is obsolete: true
Attachment #372240 - Flags: review?(brendan)
Attachment #372204 - Flags: review?(brendan)
Assignee

Comment 18

10 years ago
Comment on attachment 372204 [details] [diff] [review]
v3 (base)

clearing accidentally set obsolete flag
Attachment #372204 - Attachment is obsolete: false
Attachment #372204 - Flags: review?(brendan)
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.10?
Caching based on the before-SetPropertyHelper key-object shape is critical for performance. Have you regression tested, e.g., shell sunspider? Using the post-set shape means a guaranteed miss when the set or define property operation is predictably evolving an object from fewer to more properties.

/be
Assignee

Comment 20

10 years ago
(In reply to comment #19)
> Caching based on the before-SetPropertyHelper key-object shape is critical for
> performance. Have you regression tested, e.g., shell sunspider?

I observed no differences with sunspider or with synthetic benchmarks.

> Using the
> post-set shape means a guaranteed miss when the set or define property
> operation is predictably evolving an object from fewer to more properties.

You do not mean post-set here, but rather post-adding-sprop-to-scope, right? This is very different from caching based on the sprop after getter/setter execution. I see the need for the former (but only for the case when the property is really added, not when set has discovered it) but the later is a bug.
Assignee

Comment 21

10 years ago
I suspect the reason the benchmarks has not detected any deoptimization with the patch is that those predictive shape optimizations is done is js_Interpret. So the patch cannot affect them.
(In reply to comment #20)
> (In reply to comment #19)
> > Caching based on the before-SetPropertyHelper key-object shape is critical for
> > performance. Have you regression tested, e.g., shell sunspider?
> 
> I observed no differences with sunspider or with synthetic benchmarks.

Real work must happen in js_Interpret for the performance-critical cases. My memory is unclear of 3.0-era testing, but that's where the "add" (not set per se as you point out) case is optimized.

> > Using the
> > post-set shape means a guaranteed miss when the set or define property
> > operation is predictably evolving an object from fewer to more properties.
> 
> You do not mean post-set here, but rather post-adding-sprop-to-scope, right?

Right, although the setter for an already-added property might predictably alter the shape too. But this seems not to be perf-critical, and the worst case is exactly this bug, so we should fix the safety problem first and worry about any lingering perf issue second.

> This is very different from caching based on the sprop after getter/setter
> execution. I see the need for the former (but only for the case when the
> property is really added, not when set has discovered it) but the later is a
> bug.

It still seems desirable to avoid re-sampling kshape when there is no hazard. It does not seem like that much code. Thoughts?

/be
Assignee

Comment 23

10 years ago
(In reply to comment #19)
> Using the
> post-set shape means a guaranteed miss when the set or define property
> operation is predictably evolving an object from fewer to more properties.

This I do not understand. The current code checks the real shape of the object when doing hash-compare. If js_FillPropertyCache writes the shape before the property was added, how that can match the shape on the following shape test in js_Interpret?
Assignee

Comment 24

10 years ago
I just checked the following script with the debugger.

(function() {
  var obj = {};
  for (var i = 0; i != 2; ++i)
    obj.a = 1;
})()

Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a. This is precisely because the code stores the wrong value of obj's shape. *With* the patch on the second run the cache was hit.  So the patch makes things better, not worse.
(In reply to comment #23)
> (In reply to comment #19)
> > Using the
> > post-set shape means a guaranteed miss when the set or define property
> > operation is predictably evolving an object from fewer to more properties.
> 
> This I do not understand. The current code checks the real shape of the object
> when doing hash-compare. If js_FillPropertyCache writes the shape before the
> property was added, how that can match the shape on the following shape test in
> js_Interpret?

Different object, same progression. The key includes the pc, you don't see the same pc add the same property to the same object again. E.g.,

function fast()   { var o = {p:1, q:2, r:3} ... }
function fast2(o) { o.p = 1; o.q = 2; o.r = 3; }

/be
(In reply to comment #24)
> I just checked the following script with the debugger.
> 
> (function() {
>   var obj = {};
>   for (var i = 0; i != 2; ++i)
>     obj.a = 1;
> })()
> 
> Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a.

First time is a miss no matter what.

After the second time, no more misses -- right? (Loop till 10 instead of 2.)

Cache is for predictable regimes such as adding properties explicitly, or loop after it has warmed up.

/be
Assignee

Comment 27

10 years ago
(In reply to comment #25)

> Different object, same progression. The key includes the pc, you don't see the
> same pc add the same property to the same object again. E.g.,
> 
> function fast()   { var o = {p:1, q:2, r:3} ... }
> function fast2(o) { o.p = 1; o.q = 2; o.r = 3; }

I just checked in the debugger both these cases without the patch. They both show the cache miss 3 times. There were no cache hits. This is no surprising since the code does that special adding of properties without changing the shapes only when pc stays the same. This picture remains the same with the patch.

>> Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a.

> First time is a miss no matter what.

> After the second time, no more misses -- right? (Loop till 10 instead of 2.)
 
Yes, it no longer misses. The question is why it should miss the second time?
(In reply to comment #27)
> (In reply to comment #25)
> 
> > Different object, same progression. The key includes the pc, you don't see the
> > same pc add the same property to the same object again. E.g.,
> > 
> > function fast()   { var o = {p:1, q:2, r:3} ... }
> > function fast2(o) { o.p = 1; o.q = 2; o.r = 3; }
> 
> I just checked in the debugger both these cases without the patch. They both
> show the cache miss 3 times. There were no cache hits.

Calling which function how many times?

Of course you have to call fast twice, or fast2 twice, to get hits.

> This is no surprising
> since the code does that special adding of properties without changing the
> shapes only when pc stays the same. This picture remains the same with the
> patch.

Right.

> >> Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a.
> 
> > First time is a miss no matter what.
> 
> > After the second time, no more misses -- right? (Loop till 10 instead of 2.)
> 
> Yes, it no longer misses. The question is why it should miss the second time?

First is a miss because nothing filled for the "callsite". For this miss we fill using the before-set kshape (without your patch). That is a guaranteed miss for this kind of code the second time around the loop, as you noted. This is a bug. Can we have best of both worlds with the fix you attached? Or do we need more logic, to distinguish the two cases and handle each only when it is at hand?

If your patch shows zero regressions, then it means the interpreter handles all the fast/fast2 type cases, and the jsobj.cpp code handles the obj.a = 1 in a loop cases.

/be
Assignee

Comment 29

10 years ago
(In reply to comment #28)
> Calling which function how many times?

When the function is called the second times, then it works with or without the patch in the same way. The reason for this is that the interpreter would optimize this cases adding the properties *without* changing the shapes. 

There is zero prediction here. It is just that interpreter knows when it is safe to inline js_SetPropertyHelper and add a property without changing the shape.

> First is a miss because nothing filled for the "callsite". For this miss we
> fill using the before-set kshape (without your patch). That is a guaranteed
> miss for this kind of code the second time around the loop, as you noted. This
> is a bug. Can we have best of both worlds with the fix you attached?

Yes, this is so with the patch. As before, the interpreter takes care about known cases and does not update the shape so the cache entry will be hit again. Plus with the patch js_FillPropertyCache always writes to the cache the value that ensure the hit when the bytecode is executed again.
(In reply to comment #29)
> There is zero prediction here. It is just that interpreter knows when it is
> safe to inline js_SetPropertyHelper and add a property without changing the
> shape.

The prediction is based on pc and what happened last time (last call to fast or fast2); at least that is what I meant by "prediction". It's history-based, aka cache-based.

> Yes, this is so with the patch. As before, the interpreter takes care about
> known cases and does not update the shape so the cache entry will be hit again.
> Plus with the patch js_FillPropertyCache always writes to the cache the value
> that ensure the hit when the bytecode is executed again.

Ok, sold. I think I had some low-level misses from the Firefox 3.0 daze that this explains. I will review shortly.

Combined patch worth it for branch too?

/be
Assignee

Comment 31

10 years ago
I was very wrong when claiming that the patch does not deoptimizes the cases of adding new property. It does precisely that! If another object follows the same property addition path as the cached one, then for caching to work it must store the shape before adding the last property.

When I was debugging those cases with the patch I was too quick to assume that everything was right. Very bad!
Whew, that accords with my original thinking. I'm preoccupied with upvar fallout, so glad you are checking my early claims here!

/be
Assignee

Comment 33

10 years ago
Posted patch v4 (base) (obsolete) — Splinter Review
The new patch makes sure that mutators cache based on the value of the shape prior the addition of the property not to deoptimize object initializers. But the patch reads the shape after any resolve hooks and js_AddScopeProperty are run. This way it never stores GC-ed shape number in the cache.
Attachment #372204 - Attachment is obsolete: true
Attachment #372240 - Attachment is obsolete: true
Attachment #372618 - Flags: review?(brendan)
Attachment #372204 - Flags: review?(brendan)
Attachment #372240 - Flags: review?(brendan)
Assignee

Comment 34

10 years ago
Posted patch v4 (avoiding extra locking) (obsolete) — Splinter Review
Here is an updated patch to avoid extra locking after running a shared getter or setter. It could be ported to 1.9.0 for an extra performance, but this is not required.
Attachment #372621 - Flags: review?(brendan)
Comment on attachment 372618 [details] [diff] [review]
v4 (base)

>+     * Check for overdeep scope and prototype chain. Because resolve hooks
>+     * that js_LookupPropertyWithFlags runs can change the prototype chain
>+     * using JS_SetPrototype, we have to validate protoIndex if it is
>+     * non-zero. If it is zero, then we know from the fact that obj == pobj,
>+     * that protoIndex is invariant.

No need for , at end of next-to-last comment line.

>+                 * Here on the first iteration JSOP_SETPROP fills the cache
>+                 * with the shape of newly created object, no the shape after

s/no the shape/not the shape/

>+                 * obj.x is assigned. That mismatches obj's shape on the
>+                 * second iteration. Note that on third and the following
>+                 * iterations the cache will be hit since the shape no longer
>+                 * mutates.
>+                 */
>+                JS_ASSERT(scope->object == obj);
>+                JS_ASSERT(sprop == scope->lastProp);
>+                if (sprop->parent) {
>+                    kshape = sprop->parent->shape;
>+                } else {
>+                    JSObject *proto = STOBJ_GET_PROTO(obj);
>+                    if (proto && OBJ_IS_NATIVE(proto))
>+                        kshape = OBJ_SHAPE(proto);
>+                }
>+            }

What if the scope is branded? Its shape then evolves independently from scope->lastProp->shape, but uniquely with each added property. See SCOPE_EXTEND_SHAPE and its uses.

>+/*
>+ * If addedSprop is not null, it must be initialized to false. On exit it will
>+ * be set to true when the property was added to the scope.

s/when the property was added/if this call added the property/

/be
Comment on attachment 372621 [details] [diff] [review]
v4 (avoiding extra locking)

> JSBool
> js_NativeGet(JSContext *cx, JSObject *obj, JSObject *pobj,
 . . .
>+    *vp = LOCKED_OBJ_GET_SLOT(pobj, slot);
>+    if (!SPROP_HAS_STUB_GETTER(sprop)) {
>+        /*
>+         * For API compatibility we must set the slot with getter's result so

"the getter's result", and commas after "compatibility" and "result".

>+         * we have to make sure that it is still valid after we run the

s/have to/must/

>+         * getter.
>+         */
>+        int32 sample = cx->runtime->propertyRemovals;
>+        JSTempValueRooter tvr, tvr2;

Could we have a followup bug on breaking the API and otherwise optimizing more here? I believe we can get rid of tvr2 at least, with similar/combined approach as is used for propertyRemovals.

>+ * This function must be called with pobj's scope locked. On return that scope
>+ * is always unlocked. After function returns the caller must not access pobj

"After this function returns" and comma after "returns".

>+ * or sprop unless it knows that sprop's getter is a stub.
>  */
> extern JSBool
> js_NativeGet(JSContext *cx, JSObject *obj, JSObject *pobj,
>              JSScopeProperty *sprop, jsval *vp);
> 
>+/*
>+ * This function must be called with obj's scope locked. On return that scope
>+ * is always unlocked. After function returns the caller must not access sprop

Ditto.

/be
Attachment #372621 - Flags: review?(brendan) → review+
Given size of first patch, I would prefer a combined patch for 1.9.0.x and newer branches/repos.

/be
Assignee

Comment 38

10 years ago
(In reply to comment #35)
> What if the scope is branded? Its shape then evolves independently from
> scope->lastProp->shape, but uniquely with each added property. See
> SCOPE_EXTEND_SHAPE and its uses.

The branded scopes are not set-optimized, right? I.e. the next time interpreter hits the PC that adds a property to branded scope the shape will always be different. Then it is better to cache on the shape after the property is added to gain efficiency with loop that creates a property on the first iteration and then only updates its value. 

I will update the patch to set addedSprop only if js_AddScopeProperty has evolved the scope predictably. Then the flag should be called differently, like predictablyAddedSprop or something.
Branding ideally ends after an object has been "set up" (methods, i.e. function valued properties, have been assigned -- some work remains here, see bug 471214).

It might be best to be concrete, and pass kshape into js_FillPropertyCache again, declaring it in js_{DefineNative,Set}Property, and passing it as out parameter to js_AddScopeProperty so it can be set in all successful return cases.

/be
(In reply to comment #39)
> Branding ideally ends after an object has been "set up"

I meant "shape evolution", not "branding" -- the SCOPE_BRANDED flag must remain set so that GC_WRITE_BARRIER can do its thing and protect the cache and traces from using stale "method values".

/be
Assignee

Comment 41

10 years ago
(In reply to comment #39)
> It might be best to be concrete, and pass kshape into js_FillPropertyCache
> again, declaring it in js_{DefineNative,Set}Property, and passing it as out
> parameter to js_AddScopeProperty so it can be set in all successful return
> cases.

The problem is that implies a few extra checks due to shape-invalidating js_GenerateShape. That may happen in js_AddScopeProperty and also can happen in js_FillPropertyCache itself during SCOPE_MAKE_UNIQUE_SHAPE for the branded scope (note the bug in the current code that updates kshape only if OBJ_SCOPE(obj) == scope - this is not enough).

So passing kshape parameter is not particularly useful and just invites for hazards due to hidden shape-invalidating js_GenerateShape.
Assignee

Comment 42

10 years ago
(In reply to comment #41)

> The problem is that implies a few extra checks due to shape-invalidating
> js_GenerateShape. That may happen in js_AddScopeProperty and also can happen in
> js_FillPropertyCache itself during SCOPE_MAKE_UNIQUE_SHAPE for the branded
> scope

Which also implies that js_FillPropertyCache must check if the cache was not disabled after that SCOPE_MAKE_UNIQUE_SHAPE. One more bug to fix.
Comment on attachment 372618 [details] [diff] [review]
v4 (base)

>     /*
>-     * Check for fill from js_SetPropertyHelper where the setter removed sprop
>-     * from pobj's scope (via unwatch or delete, e.g.).
>-     */
>-    scope = OBJ_SCOPE(pobj);
>-    JS_ASSERT(scope->object == pobj);
>-    if (!SCOPE_HAS_PROPERTY(scope, sprop)) {
>-        PCMETER(cache->oddfills++);
>-        *entryp = NULL;
>-        return;
>-    }
>-

This case could happen, is it safe to replace it with an assertion? If so, please remove the oddfills meter.

/be
Assignee

Comment 44

10 years ago
(In reply to comment #43)
> >     /*
> >-     * Check for fill from js_SetPropertyHelper where the setter removed sprop
> >-     * from pobj's scope (via unwatch or delete, e.g.).
> >-     */
> 
> This case could happen, is it safe to replace it with an assertion? If so,
> please remove the oddfills meter.

I will remove oddfills - with the patch the js_FillPropertyCache should always be called before any getter or setter invocation.
Assignee

Comment 45

10 years ago
(In reply to comment #36)
> >+ * This function must be called with pobj's scope locked. On return that scope
> >+ * is always unlocked. After function returns the caller must not access pobj
> 
> "After this function returns" and comma after "returns".

To Brendan:

When I fixed the comments as suggested, the whole comment block became aligned much better improving readability. This happens noticeably more often than the change in the opposite direction when the alignment worsens. So how do you do that??? I don't believe that you count the number of letters in the text ;) !
Whiteboard: [needs landing t-m, m-c, 1.9.1]
It's my mutant gift ;-).

/be
Assignee

Comment 47

10 years ago
Posted patch v5 (base) (obsolete) — Splinter Review
The new version cache on the old shape only for predictive scope adds. It also fixes js_FillPropertyCache to exit when SCOPE_MAKE_UNIQUE_SHAPE for branded scopes disable the cache due its overflow.

Initially I tried to use uint32 *shape as out parameter for js_AddScopeProperty instead of using the boolean. But that does not work. First, js_DefineNativeProperty calls the add hook for the class after js_AddScopeProperty. There is no guarantee that the hook would not cause an extra shape generation. Second, js_AddScopeProperty would also need one more parameter - pobj to find out the previous shape for the shared scope from the prototype. So the patch continues to use a boolean flag with code to find the previous shape in js_FillPropertyScope.
Attachment #372618 - Attachment is obsolete: true
Attachment #372621 - Attachment is obsolete: true
Attachment #372874 - Flags: review?(brendan)
Attachment #372618 - Flags: review?(brendan)
Assignee

Comment 48

10 years ago
(In reply to comment #37)
> Given size of first patch, I would prefer a combined patch for 1.9.0.x and
> newer branches/repos.

That is true - I have not expected initially the scope of the necessary changes to fully address the issues without hurting the performance.
Assignee

Comment 49

10 years ago
Posted patch v5 (avoiding extra locking) (obsolete) — Splinter Review
here is for references the second part. I will shortly post the combined patch.
Assignee

Comment 50

10 years ago
Posted patch v5 (obsolete) — Splinter Review
The whole change as a single patch.
Attachment #372874 - Attachment is obsolete: true
Attachment #372875 - Attachment is obsolete: true
Attachment #372877 - Flags: review?(brendan)
Attachment #372874 - Flags: review?(brendan)
Comment on attachment 372877 [details] [diff] [review]
v5

>+    *vp = LOCKED_OBJ_GET_SLOT(pobj, slot);
>+    if (!SPROP_HAS_STUB_GETTER(sprop)) {
>+        /*
>+         * For API compatibility, we must set the slot with the getter's
>+         * result, so we have to make sure that it is still valid after we run
>+         * the getter.

Cite the new bug in a FIXME: comment here and possibly in js_NativeSet (different issue there, maybe needing a separate followup bug).

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

Comment 52

10 years ago
Posted patch v5b (obsolete) — Splinter Review
adding FIXME references to the bug 488458
Attachment #372877 - Attachment is obsolete: true
Attachment #372933 - Flags: review+
Assignee

Comment 53

10 years ago
landed to TM  - http://hg.mozilla.org/tracemonkey/rev/d1a4ee3d0c59
Whiteboard: [needs landing t-m, m-c, 1.9.1] → [needs landing m-c, 1.9.1] fixed-in-tracemonkey

Comment 54

10 years ago
With this changeset, the time to run trace-test.js goes from 30s to 600s.
That's a bad performance hit -- presumably for the reason worried about in this bug of defeating the cache's predictions. A quick rebuild with

JS_PROPERTY_CACHE_METERING

defined will result at runtime in /tmp/propcache.stats. Please post it with and without this patch.

/be
(In reply to comment #54)
> With this changeset, the time to run trace-test.js goes from 30s to 600s.

With -j (safe bet; without is too slow irrespective of this bug). Probably the regression is jit-only, since the propcache.stats hit rates seem uniformly better with the patch.

Igor, do you have time to look at this?

/be
Jim, could you get TRACEMONKEY=stats results with and without the patch for trace-test.js? Thanks,

/be
Assignee

Comment 60

10 years ago
It could be the case (In reply to comment #56)
> Created an attachment (id=372960) [details]
> Property cache stats, pre-d1a4ee3d0c59

(In reply to comment #57)
> Created an attachment (id=372961) [details]
>  Property cache stats, post-d1a4ee3d0c59

Here is the essence:

Before:

hit rates: pc 54.694% (proto 0.146141%), set 80.9384%, ini 8.99743%, full 75.65%
hit rates: pc 54.8281% (proto 0.145467%), set 80.8767%, ini 9.29241%, full 75.6896%
hit rates: pc 54.9644% (proto 0.144788%), set 80.8349%, ini 9.58439%, full 75.7307%
hit rates: pc 55.0618% (proto 0.144222%), set 80.6952%, ini 9.87342%, full 75.7491%
hit rates: pc 55.2644% (proto 0.142697%), set 80.4095%, ini 11.0652%, full 75.7904%
hit rates: pc 55.3603% (proto 0.142165%), set 80.3399%, ini 11.1934%, full 75.8119%
hit rates: pc 55.4589% (proto 0.141625%), set 80.29%, ini 11.3208%, full 75.835%
hit rates: pc 55.5232% (proto 0.141203%), set 80.1527%, ini 11.4473%, full 75.8406%
hit rates: pc 55.6686% (proto 0.14025%), set 80.0443%, ini 11.8644%, full 75.8739%
hit rates: pc 63.9982% (proto 0.113134%), set 79.8673%, ini 12.4303%, full 80.3536%


After:
hit rates: pc 89.8626% (proto 0.000519039%), set 77.8673%, ini 99.5293%, full 89.937%
hit rates: pc 94.8399% (proto 0.000264192%), set 88.9242%, ini 99.624%, full 94.8778%
hit rates: pc 96.4619% (proto 0.000177162%), set 92.6145%, ini 99.688%, full 96.4873%
hit rates: pc 97.3025% (proto 0.000133503%), set 94.4575%, ini 99.7136%, full 97.3217%
hit rates: pc 97.8127% (proto 0.000107122%), set 95.5642%, ini 99.7333%, full 97.8281%
hit rates: pc 96.4858% (proto 8.9456e-05%), set 89.6316%, ini 99.7492%, full 96.4986%
hit rates: pc 96.9828% (proto 7.68031e-05%), set 91.1106%, ini 99.7611%, full 96.9938%
hit rates: pc 97.3565% (proto 6.72911e-05%), set 92.2201%, ini 99.7708%, full 97.3661%
hit rates: pc 97.6378% (proto 5.98705e-05%), set 93.0836%, ini 99.781%, full 97.6464%
hit rates: pc 97.8726% (proto 5.3919e-05%), set 93.7747%, ini 99.7917%, full 97.8803%
hit rates: pc 98.0544% (proto 4.90407e-05%), set 94.3403%, ini 99.8024%, full 98.0615%
hit rates: pc 97.3634% (proto 4.4919e-05%), set 91.4483%, ini 99.8281%, full 97.3698%
hit rates: pc 97.4542% (proto 4.33699e-05%), set 91.5753%, ini 99.9197%, full 97.4605%
hit rates: pc 97.4644% (proto 4.26946e-05%), set 91.5808%, ini 99.9239%, full 97.4706%
hit rates: pc 97.594% (proto 3.94228e-05%), set 92.2339%, ini 99.9338%, full 97.5997%
hit rates: pc 97.0603% (proto 3.67294e-05%), set 89.945%, ini 99.9359%, full 97.0656%
hit rates: pc 97.2335% (proto 3.43803e-05%), set 90.6082%, ini 99.9378%, full 97.2385%
hit rates: pc 97.3911% (proto 3.2328e-05%), set 91.1874%, ini 99.9388%, full 97.3958%
hit rates: pc 97.538% (proto 3.05082e-05%), set 91.6992%, ini 99.9396%, full 97.5424%
hit rates: pc 97.6635% (proto 2.88836e-05%), set 92.1546%, ini 99.9403%, full 97.6677%
hit rates: pc 97.2641% (proto 2.74244e-05%), set 90.4791%, ini 99.941%, full 97.2681%
hit rates: pc 97.3956% (proto 2.61067e-05%), set 90.9494%, ini 99.9415%, full 97.3993%
hit rates: pc 97.0451% (proto 2.49087e-05%), set 89.4883%, ini 99.942%, full 97.0487%
hit rates: pc 97.1749% (proto 2.38146e-05%), set 89.9613%, ini 99.9427%, full 97.1783%
hit rates: pc 97.2888% (proto 2.2812e-05%), set 90.3938%, ini 99.9435%, full 97.2921%
hit rates: pc 97.3835% (proto 2.18718e-05%), set 90.7942%, ini 99.9462%, full 97.3866%
hit rates: pc 97.3692% (proto 2.15298e-05%), set 90.6204%, ini 99.9603%, full 97.3723%
hit rates: pc 97.3848% (proto 2.1371e-05%), set 90.6204%, ini 99.9604%, full 97.3879%
hit rates: pc 97.4048% (proto 2.12e-05%), set 90.6204%, ini 99.9603%, full 97.4079%
hit rates: pc 97.4106% (proto 2.11423e-05%), set 90.6204%, ini 99.9603%, full 97.4137%


Note that "After" has 3 times more lines meaning that GC was running more frequent. If that is indeed the case, then it would explain the jit slowdown.
Are we backing this out from tm?  (Did earlier versions of the patch not cause the 20x regression? Not sure how we missed that.)

Updated

10 years ago
Whiteboard: [needs landing m-c, 1.9.1] fixed-in-tracemonkey
Assignee

Comment 63

10 years ago
Comment on attachment 372933 [details] [diff] [review]
v5b

>-#ifdef JS_TRACER
>-                if (!entry && TRACE_RECORDER(cx))
>-                    js_AbortRecording(cx, "SetPropUncached");
>-#endif
>+                ABORT_RECORDING(cx, "SetPropUncached");

This must be the culprit for the slow down.
Assignee

Comment 64

10 years ago
Posted patch v6 (obsolete) — Splinter Review
The new patch fixes that always-abort-setprop issue using ABORT_RECORDING_IF macro. It also uses ((void) 0), not ((void 0)), for non-jit case of the macros.

With the new patch I see no differences with trace-test.js timing.
Attachment #372933 - Attachment is obsolete: true
Attachment #373009 - Flags: review?(brendan)
Comment on attachment 373009 [details] [diff] [review]
v6

In the patch for bug 471214 I added ABORT_RECORDING too -- but I kept the cx param explicit. On IRC just now Igor agreed to do that and lose ABORT_RECORDING_IF, since compilers can optimize away if (pure_cond) ((void)0); without warning -- no #ifdef JS_TRACER / #endif needed.

Otherwise looks great. I can't believe I saw the loss of if (!entry) and blinked past it (distracted by checking how my ABORT_RECORDING appeared sooner than in the patch for bug 471214).

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

Comment 66

10 years ago
Posted patch v7 (obsolete) — Splinter Review
using canonical ABORT_RECORDING(cx, reason) and explicit ifs
Attachment #373009 - Attachment is obsolete: true
Attachment #373019 - Flags: review+
Assignee

Comment 67

10 years ago
landed to TM - http://hg.mozilla.org/tracemonkey/rev/dccd96fc69cc
Whiteboard: fixed-in-tracemonkey
(In reply to comment #68)
> I think this patch might be leaking
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1239844666.1239848204.19217.gz

That orange cycle's blame is

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

or bug 488203, not this bug.

/be

Comment 70

10 years ago
Backed out to see if that fixes macosx leak. Still not 100% clear the patch is guilty.

http://hg.mozilla.org/tracemonkey/rev/5bd116148175
Assignee

Comment 71

10 years ago
(In reply to comment #70)
> Backed out to see if that fixes macosx leak.

You meant Linux leak, right? On Mac the patch alone so far has not caused leaks any leaks.
Assignee

Comment 72

10 years ago
(In reply to comment #71)
> (In reply to comment #70)
> > Backed out to see if that fixes macosx leak.
> 
> You meant Linux leak, right? On Mac the patch alone so far has not caused leaks
> any leaks.

Specifically, with the patch one of the two linux tinderbox runs has reported:

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1239901924.1239908080.22165.gz

reftest
2932/0/164 LEAK
Depends on: 488734
note, this patch might have caused a regression on topsites, see Bug 488734

Comment 75

10 years ago
Thanks tomcat.

Comment 76

10 years ago
Backed out again.

http://hg.mozilla.org/tracemonkey/rev/9c63c15b92b5
Whiteboard: fixed-in-tracemonkey
(In reply to comment #76)
> Backed out again.
> 
> http://hg.mozilla.org/tracemonkey/rev/9c63c15b92b5

the backout fix Bug 488734
About to attach updated patch that fixes trivially the no-longer-valid assertion whose botch was sole cause of bug 488734 (AFAICT).

/be
Blocks: 488734
No longer depends on: 488734
Posted patch v8 (obsolete) — Splinter Review
Attachment #373019 - Attachment is obsolete: true
Attachment #373393 - Flags: review?(igor)
Assignee

Comment 80

10 years ago
Bug 488734 happens due to the following code:

                    if (!js_SetPropertyHelper(cx, obj, id, &rval, &entry))
                        goto error;
#ifdef JS_TRACER
                    if (entry)
                        TRACE_1(SetPropMiss, entry);
#endif

With the patch this sequence is wrong as the cache will be filled before the setter call. That setter changes the shape. It triggers the assert in the recorder since the recorder checks if shape matches the cache. Note that without the patch the recorder later abort the recording due to presence of the setter.
No longer blocks: 488734
Depends on: 488734
Assignee

Comment 81

10 years ago
Comment on attachment 373393 [details] [diff] [review]
v8

The assert fix is not enough - when SetPropMis is called the entry may point to the completely unrelated sprop due to the setter activity.

The right way to fix is to replace SetPropertyHelper with function that does not call js_NativeSet at the end delegating that jot to the caller. Then the interpreter can use NATIVE_SET which would properly abort the trace before calling the setter.
Attachment #373393 - Flags: review?(igor) → review-
(In reply to comment #81)
> (From update of attachment 373393 [details] [diff] [review])
> The assert fix is not enough - when SetPropMis is called the entry may point to
> the completely unrelated sprop due to the setter activity.

Yeah -- I was trying to goose the process after you dropped off irc, but it was not right.

> The right way to fix is to replace SetPropertyHelper with function that does
> not call js_NativeSet at the end delegating that jot to the caller. Then the
> interpreter can use NATIVE_SET which would properly abort the trace before
> calling the setter.

Abort under what condition? TraceRecorder::record_SetPropMiss does not abort. If you can, let's talk on IRC.

/be
We want to trace setter calls, the logic to abort or not will change. It should live in jstracer.cpp.

/be
Assignee

Comment 84

10 years ago
Summary of IRC discussion:

For simplicity I will update v5 (base) and fix SetPropMis problem there.
Assignee

Comment 85

10 years ago
(In reply to comment #84)
> Summary of IRC discussion:
> 
> For simplicity I will update v5 (base) and fix SetPropMis problem there.

I will finish that tomorrow. The idea is to move SetPropMis to NativeSet to call the function there before calling any setter. But that is little bit involved due to the need to unlock the object before calling the function.
Assignee

Comment 86

10 years ago
I will work on the issue of stalled cache shapes in the bug 487846. The minimal fix here can be trivially back-ported to branches and can be included in 1.9.0.10. Given the complexity of js_GetSprop and the need to do new locking after running the getter the extra complexity of writing of 4 words to the memory without doing any ifs on a hot cache is nothing.
Attachment #373393 - Attachment is obsolete: true
Attachment #373462 - Flags: review?(brendan)
Assignee

Comment 87

10 years ago
Regarding disabling the GC for the getter or setter:

This cannot be a simple patch with few lines of code. For example, the iteration with the cycle collector has to be considered. The setter can run an arbitrary script so disabling the GC has to be propagated to the CC code as well. Another issue is __proto__ assignments done using the GC machinery. The patch has to be careful not to disable that. AFAICS all this is doable, but it requires some work. Plus it cannot be taken for granted that a port for 1.9.0 would just work.
Attachment #373462 - Flags: review?(brendan) → review+
Assignee

Comment 88

10 years ago
http://hg.mozilla.org/tracemonkey/rev/4ea30a50bc4f
Whiteboard: fixed-in-tracemonkey
Assignee

Comment 90

10 years ago
Comment on attachment 373633 [details] [diff] [review]
just fixing GC hazard for 1.9.0

The version for 1.9.0 branch required a trivial merge due to SPROP_GET->js_GetSprop changes on 1.9.1.
Attachment #373633 - Attachment description: jus → just fixing GC hazard for 1.9.0
Attachment #373633 - Attachment is patch: true
Attachment #373633 - Attachment mime type: application/octet-stream → text/plain
Attachment #373633 - Flags: approval1.9.0.10?
Assignee

Updated

10 years ago
Flags: blocking1.9.0.10?
Given the number of iterations on the patch and 1.9.0.10 code-freeze in two days this will probably have to go in 1.9.0.11 after testing on 1.9.1
Flags: blocking1.9.0.10? → blocking1.9.0.11?
Whiteboard: fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey
Attachment #373633 - Flags: approval1.9.0.10? → approval1.9.0.11?
Assignee

Comment 92

10 years ago
(In reply to comment #91)
> Given the number of iterations on the patch and 1.9.0.10 code-freeze in two
> days this will probably have to go in 1.9.0.11 after testing on 1.9.1

The iterations were unrelated to the minimal fix. They were related to my initial attempt to have a fix that would address both this bug and the bug 487846. 

Moreover, the backed-out patch only had issues related to interactions with the trace recorder. There are no evidence that with the jit disabled (or no jit code as on 1.9.0) the patch would do any harm.

My worry with delaying the minimal fix until 1.9.0.11 is that the change itself is quite revealing and gives a strong hint about how to bake an exploit. On the other hand, building an exploit does require good knowledge of SpiderMonkey internals...
Attachment #373633 - Flags: approval1.9.0.11? → approval1.9.0.10+
Comment on attachment 373633 [details] [diff] [review]
just fixing GC hazard for 1.9.0

Thanks for the explanation.

Approved for 1.9.0.10, a=dveditz for release-drivers
Flags: blocking1.9.0.11? → blocking1.9.0.10+

Comment 94

10 years ago
http://hg.mozilla.org/mozilla-central/rev/4ea30a50bc4f
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Assignee

Comment 95

10 years ago
landed to 1.9.0 branch:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.482; previous revision: 3.481
done
Keywords: fixed1.9.0.10
Bob, can you verify this fix for 1.9.0.11?

Comment 99

10 years ago
v 1.9.0, 1.9.1
Flags: in-testsuite+
Assignee

Updated

10 years ago
Blocks: 484042
Flags: blocking1.8.1.next?
we need this fix for the 1.8 branch too. Probably a simple variant of the 1.9.0 patch will work.
Flags: blocking1.8.1.next?
Depends on: 498132
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [needs approval/landing for 1.8 branch][sg:critical?] fixed-in-tracemonkey
Attachment #383224 - Flags: approval1.8.1.next?

Comment 102

10 years ago
v 1.9.3, 1.9.2
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Attachment #383224 - Flags: approval1.8.1.next? → approval1.8.1.next+
Comment on attachment 383224 [details] [diff] [review]
1.8 version (no code change)

Approved for 1.8.1.24, a=dveditz for release-drivers
Checked in for 1.8.1.24:
Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.63; previous revision: 3.208.2.62
Keywords: fixed1.8.1.24
when this bug is opened, the test should be checked in.
Flags: in-testsuite+ → in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.