Last Comment Bug 352846 - Passing unrooted value to OBJ_DEFAULT_VALUE is GC hazard
: Passing unrooted value to OBJ_DEFAULT_VALUE is GC hazard
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 355029 js1.7src
  Show dependency treegraph
 
Reported: 2006-09-15 11:22 PDT by Igor Bukanov
Modified: 2007-02-08 16:19 PST (History)
5 users (show)
mtschrep: blocking1.8.1-
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (14.38 KB, patch)
2006-09-18 09:07 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
Fix v2 (14.28 KB, patch)
2006-09-18 15:10 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Fix v2b (14.28 KB, patch)
2006-09-18 16:20 PDT, Igor Bukanov
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review
e4x/GC/regress-352846-01.js (2.69 KB, text/plain)
2006-09-19 06:26 PDT, Bob Clary [:bc:]
no flags Details
e4x/GC/regress-352846-02.js (2.72 KB, text/plain)
2006-09-19 06:26 PDT, Bob Clary [:bc:]
no flags Details
e4x/GC/regress-352846-03.js (2.75 KB, text/plain)
2006-09-19 06:27 PDT, Bob Clary [:bc:]
no flags Details
Fix v2b for 1.8.0 branch (14.25 KB, patch)
2006-11-03 14:48 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.0.9+
Details | Diff | Splinter Review
diff between trunk and 1.8.0 version of patches (7.34 KB, patch)
2006-11-03 14:49 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
For 1.8.1: Fix v2b + regression fix from bug 355029 (14.64 KB, patch)
2006-11-07 11:49 PST, Igor Bukanov
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review
Fix v2b for 1.8.0 branch + regression fix from bug 355029 (15.06 KB, patch)
2006-11-07 11:57 PST, Igor Bukanov
dveditz: approval1.8.0.9+
Details | Diff | Splinter Review

Description Igor Bukanov 2006-09-15 11:22:54 PDT
Consider the following example:

function prepare_xml()
{
  delete XML.prototype.function::toString;
  Object.prototype.toString getter = toSource_getter;
  return new XML("<a>xml_text</a>"); 
}

var counter = 0;
  
function toSource_getter() 
{
  // Make sure that lastInternalResult does not contain prepare_xml 
  var tmp = { toSource: function() { return [1,2,3]; } };
  uneval(tmp);

  if (counter++ < 2) return undefined;
    
  // run GC onr overwrite the heap
  if (typeof gc == "function") gc();
  var x = 1e-100;
  for (var i = 0; i != 50000; ++i)
    var x2 = x / 4;
  
  // Return a function that would access this in non-trivial way to
  // check if prepare_xml() was rooted.
  return function() { return this.toXMLString(); };
}

uneval({ toSource: prepare_xml });

Currently it gives me segmentation fault in the shell since the result of  prepare_xml() is accessed after it was GC-ed. The culprit here is js_ValueToSource code that does not root the result of toSource method before passing it to js_ValueToString. 

This is a GC-hazard since it is possible to trigger a GC and collect the unrooted argument of js_ValueToString before it is accessed using a specially crafted xml object. 

When js_ValueToString  calls OBJ_DEFAULT_VALUE on an unrooted xml object, eventually  JS_CallFunctionName calls getMethod on the object to find its "toString" method. That in turn via GetFunction  would query the prototype chain. Since it is possible to remove toString from XML objects and define a getter for Object.prototype.toString, it allows to run GC in the getter and collect the object.
Comment 1 Mike Schroepfer 2006-09-16 11:43:25 PDT
My recommendation would be to differ this to 2.0.1 - we need to stop taking changes for FF2...
Comment 2 Igor Bukanov 2006-09-16 16:40:15 PDT
I am looking at all the places to check that js_ValueToString(cx, v) is always called with a rooted value. It looks like jsstr.c has problematic place in find_replen. That function contains the following lines:

        /* Lift current frame to include the args and do the call. */
        fp = cx->fp;
        oldsp = fp->sp;
        fp->sp = sp;
        ok = js_Invoke(cx, argc, JSINVOKE_INTERNAL);
        rval = fp->sp[-1];
        fp->sp = oldsp;

        if (ok) {
            /*
             * NB: we count on the newborn string root to hold any string
             * created by this js_ValueToString that would otherwise be GC-
             * able, until we use rdata->repstr in do_replace.
             */
            repstr = js_ValueToString(cx, rval);

Here rval apparently is not rooted when js_ValueToString is called. But testing with GC_MARK_DEBUG revealed that it is rooted through a stack. But how exactly given that  the temporary stack is already replaced by the original value?
Comment 3 Brendan Eich [:brendan] 2006-09-16 18:07:20 PDT
(In reply to comment #2)
> But how exactly
> given that  the temporary stack is already replaced by the original value?

It's protected by the stack segment allocated by js_AllocStack and scanned by the GC (see JSStackHeader, jscntxt.h) until the js_FreeStack at the lambda_out: label.

/be
Comment 4 Igor Bukanov 2006-09-16 18:44:42 PDT
Here is another similar hazard in sort_compare (will Array.sort be ever 100% correct?). The function calls js_ValueToNumber passing as argument a value that is returned by the compare function  and which is only rooted through cx->lastResult. As such the following gives the crash:

var counter = 0;
  
function prepare_xml()
{
  delete XML.prototype.function::toString;
  Object.prototype.toString getter = toSource_getter;
  return new XML("<a>xml_text</a>"); 
}

function toSource_getter() 
{
  // Make sure that lastInternalResult does not contain prepare_xml 
  var tmp = { toSource: function() { return [1,2,3]; } };
  uneval(tmp);

  if (counter++ < 2) return undefined;
    
  // run GC onr overwrite the heap
  if (typeof gc == "function") gc();
  var x = 1e-100;
  for (var i = 0; i != 50000; ++i)
    var x2 = x / 4;
  
  // Return a function that would access this in non-trivial way to
  // check if prepare_xml() was rooted.
  return function() { 
    print("xxx");
    return this.toXMLString(); 
  };
}

var a = [1, 2];

a.sort(prepare_xml);
Comment 5 Igor Bukanov 2006-09-16 19:04:38 PDT
The same story is with js_GetLengthProperty from jsarray.c. It calls js_ValueToECMAUint32 which eventually calls OBJ_DEFAULT_VALUE without rooting the value of length property:

var counter = 0;
  
function prepare_xml()
{
  delete XML.prototype.function::toString;
  Object.prototype.toString getter = toSource_getter;
  return new XML("<a>xml_text</a>"); 
}

function toSource_getter() 
{
  // Make sure that lastInternalResult does not contain prepare_xml 
  var tmp = { toSource: function() { return [1,2,3]; } };
  uneval(tmp);

  if (counter++ < 2) return undefined;
    
  // run GC onr overwrite the heap
  if (typeof gc == "function") gc();
  var x = 1e-100;
  for (var i = 0; i != 50000; ++i)
    var x2 = x / 4;
  
  // Return a function that would access this in non-trivial way to
  // check if prepare_xml() was rooted.
  return function() { 
    print("xxx");
    return this.toXMLString(); 
  };
}

var obj = { };
obj.length getter = prepare_xml;
Array.reverse(obj);

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/s/x.js
before 9232, after 9232, break 08180000
Segmentation fault
Comment 6 Igor Bukanov 2006-09-18 09:07:00 PDT
Created attachment 239036 [details] [diff] [review]
Fix v1

The patch addresses the 3 problems with test cases plus the following:

1. js_HasLengthProperty from jsarray.c has exactly the same issue as js_GetLengthProperty but since the browser or shells do not call it, no script-only test case can be provided.

2. Implementation of Array.protype.toLocalString in array_join_sub contains the following code:
                if (!js_ValueToObject(cx, v, &obj2) ||
                    !js_TryMethod(cx, obj2,
                                  cx->runtime->atomState.toLocaleStringAtom,
                                  0, NULL, &v)) {

This is GC-safe currently because js_ValueToObject never returns a weakly rooted XML object with weakly meaning "can be unrooted with a script". But such assumptions are extremely fragile like this bug demonstrates. Thus the patch explicitly roots the value.
Comment 7 Brendan Eich [:brendan] 2006-09-18 10:19:23 PDT
Comment on attachment 239036 [details] [diff] [review]
Fix v1


>+    /* After this point the control must flow through out: */
>+    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>     id = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom);
>-    if (!OBJ_GET_PROPERTY(cx, obj, id, &v))
>-        return JS_FALSE;
>+    ok = OBJ_GET_PROPERTY(cx, obj, id, &tvr.u.value);
>+    if (!ok)
>+        goto out;
> 
>-    /* Short-circuit, because js_ValueToECMAUint32 fails when
>-     * called during init time.
>+    /*
>+     * Short-circuit, because js_ValueToECMAUint32 fails when called during
>+     * init time.
>      */
>-    if (JSVAL_IS_INT(v)) {
>-        i = JSVAL_TO_INT(v);
>+    if (JSVAL_IS_INT(tvr.u.value)) {
>+        i = JSVAL_TO_INT(tvr.u.value);
>         /* jsuint cast does ToUint32. */
>         *lengthp = (jsuint)i;
>-        return JS_TRUE;
>+        ok = JS_TRUE;
>+    } else {
>+        ok = js_ValueToECMAUint32(cx, tvr.u.value, (uint32 *)lengthp);
>     }
>-    return js_ValueToECMAUint32(cx, v, (uint32 *)lengthp);
>+  out:
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+    return ok;

This function is short enough, and enough lines are being changed, that I would avoid goto out and simply nest the bulk of the code inside if (ok) {...} after the OBJ_GET_PROPERTY call.

Bonus cleanup point for moving the minor "jsuint cast does ToUint32." comment to the right of the line it applies to, tabbed one stop and with the period at the end removed.  It hurts my eyes.

>+ * If count is -3, then u.object holds single rooted JSObject*.

Is this worth the source code cost in general, and the compiled code cost in jsgc.c, small though the increase is for this new case?  OBJECT_TO_JSVAL(obj) passed to JS_PUSH_SINGLE_TEMP_ROOT should be equivalent in compiled code.

Looks good otherwise -- totally agree on avoiding fragile "hanging by a thread" rooting. I doubt that rogerl or whoever wrote the toLocaleString method was counting on it, or was aware of the GC hazard -- I obviously was not and I probably reviewed that patch.

/be
Comment 8 Mike Schroepfer 2006-09-18 11:49:27 PDT
Pushing to 1.8.1.1...
Comment 9 timeless 2006-09-18 12:08:49 PDT
we should not be passing on gc hazards for releases.
Comment 10 Brendan Eich [:brendan] 2006-09-18 12:52:08 PDT
(In reply to comment #9)
> we should not be passing on gc hazards for releases.

You know that we have a security bug policy and a patch update system, and you know how they have worked together since Firefox 1.5.  You know that we have a non-trivial list of to-be-fixed security bugs, and that we will have such a list for the foreseeable future.  So why are you making categorical statements about how every such bug ("passing on gc hazards") must be fixed before any particular release occurs?

Igor, mrbkap and I at least have been working hard on memory safety, and I'm pretty sure that we don't agree with you that every such bug should slip the release.  It's true we are taking a chance that some bad guy might independently discover the flaw.  But that's not the only thing to avoid here.  Not shipping Firefox 2 this year is also worth a lot, including in security coverage.

See bug 352064 comment 19.

/be
Comment 11 Igor Bukanov 2006-09-18 13:33:47 PDT
(In reply to comment #7)
> Is this worth the source code cost in general, and the compiled code cost in
> jsgc.c, small though the increase is for this new case?  

I wanted to have a root which address could be passed without ugly casts to a function taking JSObject**. An alternative is to add an object to the union, but stil use -1 as the count assuming that the tagging should do the right job with jsval and assuming a sensible compiler, but that looked unclean. 

But I guess code bloat is always bad a thing. Thus I will use the union without -3 but with an assert that the sizeof(union) == sizeof(jsval) == sizeof(JSObject *). 

> I doubt that rogerl or whoever wrote the toLocaleString method was
> counting on it, or was aware of the GC hazard -- I obviously was not and I
> probably reviewed that patch.

When that code was written, it was 100% safe since you need an XML object there. Without jsxml.c js_TryMethod, OBJ_DEFAULT_VALUE etc. are safe against passing unrooted values. This is even true with getter/setters since there "this" still points to to the original object no matter what and as such is rooted. It was jsxml.c that violated that. 

Also note that without getters/setters almost all reported GC hazards would not exit and probably quite a few fixed places were written before scripted getters and setters were added. 

So there is pattern:

GC-safe-code -> new feature that violates a previous hidden assumption -> GC-hazard. 

Now the interesting part is to think about close hooks (new feature!) and what hidden assumptions they violate?
Comment 12 Igor Bukanov 2006-09-18 13:49:20 PDT
(In reply to comment #9)
> we should not be passing on gc hazards for releases.

These are safe hazards since they require very deliberate code to exploit. As such a probability that they happen in real code is practical 0. Moreover, no tools exists yet that can help to find them, so finding such bugs is rather manual-intensive task.

Compare that with, for example, decompiler bugs. They can be and *is* found by an automated tool. Moreover, given the amount of junk that exists on web pages such bugs can be revealed by real-life examples.

So it is really important to have a bulletproof decompiler while fixing hazards like this bug can wait for another release.
Comment 13 Brendan Eich [:brendan] 2006-09-18 13:56:41 PDT
(In reply to comment #12)
> So it is really important to have a bulletproof decompiler while fixing hazards
> like this bug can wait for another release.

Hence my working night and day recently :-/.

Igor, you're right that jsxml.c upset an old assumption, but I bet the assumption was not well understood.  I used to think I could see the run-to-completion paths during which GC could not nest, but I know better now.  This is like seeing where interrupts need to be blocked in single-processor Unix, or seeing the paths in SP Unix during which no subroutine calls sleep.

All such systems violate abstraction, in particular the ability to hook new code in (compositionality).  To make the abstractions bullet-proof for GC, we need to track all references.  Doing this manually with JSTempValueRooters is hard, but do-able if we keep after it.  Conservative GC would be better use of everyone's time, I think.

I had occasion to talk to a JVM guru recently, who was convinced that exact GC is fine ("it took Sun six months to switch from conservative to exact").  But Java doesn't have getters and setters, and that makes a big difference.  It's not the only or (since JS is evolving) last such difference.  Comments?

/be
Comment 14 Igor Bukanov 2006-09-18 14:24:47 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > So it is really important to have a bulletproof decompiler while fixing hazards
> > like this bug can wait for another release.
> 
> Hence my working night and day recently :-/.

I wish I could help, but decompiler is a magic box for me :(

....
> To make the abstractions bullet-proof for GC, we
> need to track all references.  Doing this manually with JSTempValueRooters is
> hard, but do-able if we keep after it.  Conservative GC would be better use of
> everyone's time, I think.

I think the problem with SM is that newborn array gives really false-sence-of-security. With API that takes only explicit roots like tvr, most of the bugs would not simply exist. Add to that an a tool that check type annotations like one Linux kernel uses, and you get rather safe system.  

> I had occasion to talk to a JVM guru recently, who was convinced that exact GC
> is fine ("it took Sun six months to switch from conservative to exact").  But
> Java doesn't have getters and setters, and that makes a big difference.  It's
> not the only or (since JS is evolving) last such difference.  Comments?

In Java almost all the code is written in Java itself. Compare that with SM where all host objects are written in C. I wish SM had a high level assembler so at least Array can written using it. How many GC from jsarray.c it would remove?

Plus with the right internal and external API it should be easy to enforce everything is rooted model. Just imagine that it would be impossible to pass unrooted jsval* to OBJ_GET_PROPERTY and this is what happens with Java native interface AFAICS.
Comment 15 Brendan Eich [:brendan] 2006-09-18 15:05:59 PDT
(In reply to comment #14)
> I think the problem with SM is that newborn array gives really
> false-sence-of-security.

That's true, wherefore bug 313437.

> With API that takes only explicit roots like tvr, most
> of the bugs would not simply exist.

That's not clear to me at all.  If you have to take extra effort with macros and balanced PUSH and POP, not everyone will remember to do it.  An implicit model would be better.

> Add to that an a tool that check type
> annotations like one Linux kernel uses, and you get rather safe system.

Type system support would be great.

> In Java almost all the code is written in Java itself. Compare that with SM
> where all host objects are written in C. I wish SM had a high level assembler
> so at least Array can written using it. How many GC from jsarray.c it would
> remove?

Absolutely.  But do you really need assembler, or just JS2 features and optimizing runtime?  First one, then the other, is one way to do it.  In any event, self-hosting more of the standard library is a definite longer-term SpiderMonkey goal for me.

> Plus with the right internal and external API it should be easy to enforce
> everything is rooted model. Just imagine that it would be impossible to pass
> unrooted jsval* to OBJ_GET_PROPERTY and this is what happens with Java native
> interface AFAICS.

Sure, if you make everyone hold jref or equivalent handle on value.  But that is not what I did ten years ago, and we have to break APIs to get there.  It also has a cost that isn't obviously lower than conservative GC.

Anyway, we should talk about this more in a non-bugzilla forum.  But if you would like to take bug 313437 from my list, please do so.

/be
Comment 16 Igor Bukanov 2006-09-18 15:10:06 PDT
Created attachment 239084 [details] [diff] [review]
Fix v2

The patch besides addressing the nits removes that count == -3 and assumes instead that the trick with tagged jsval works as expected.

Another thing that I forgot to mention in the comments to v1 is the declaration of GC_MARK_DEBUG-only js_LiveThingToFind and js_DumpGCHeap in jsgc.c. It helps to avoid writing extra "extren js_LiveThingToFind" when debugging.
Comment 17 Brendan Eich [:brendan] 2006-09-18 15:24:02 PDT
Comment on attachment 239084 [details] [diff] [review]
Fix v2


>+/*
>+ * The following allows to reinterpret JSTempValueUnion.object as jsval using
>+ * the tagging property of a generic jsval described bellow.

"below"

Looks great, thanks for fixing.  Yeah, js_LiveThingToFind started out life in the dog-house and only now is escaping. ;-)

/be
Comment 18 Igor Bukanov 2006-09-18 15:40:17 PDT
(In reply to comment #15)
> That's not clear to me at all.  If you have to take extra effort with macros
> and balanced PUSH and POP, not everyone will remember to do it.  An implicit
> model would be better.

Except it would not protect against bug 353    165 as the only implicit model that can stop it is no GC at all during native calls. 

> 
> > Add to that an a tool that check type
> > annotations like one Linux kernel uses, and you get rather safe system.
> 
> Type system support would be great.

I filed bug 353231about it.

> Absolutely.  But do you really need assembler, or just JS2 features and
> optimizing runtime?  First one, then the other, is one way to do it.  In any
> event, self-hosting more of the standard library is a definite longer-term
> SpiderMonkey goal for me.

See bug 353235.
 
> It
> also has a cost that isn't obviously lower than conservative GC.

In principle I agree, but GCJ-compiled Eclipse sucks compared with one that runs inside JVM. Yes, its compiler is faster but the rest og GUI is slower. I wonder how much of that slowness comes from conservative GC. 
Comment 19 Brendan Eich [:brendan] 2006-09-18 15:51:07 PDT
(In reply to comment #18)
> (In reply to comment #15)
> > That's not clear to me at all.  If you have to take extra effort with macros
> > and balanced PUSH and POP, not everyone will remember to do it.  An implicit
> > model would be better.
> 
> Except it would not protect against bug 353165 as the only implicit model
> that can stop it is no GC at all during native calls.

The other implicit model that can stop such bugs is conservative GC of course ;-).

/be
Comment 20 Igor Bukanov 2006-09-18 16:13:00 PDT
(In reply to comment #19)
>> > An implicit
> > > model would be better.
> > 
> > Except it would not protect against bug 353165 as the only implicit model
> > that can stop it is no GC at all during native calls.
> 
> The other implicit model that can stop such bugs is conservative GC of course

Right, so here comes bug 353242.
Comment 21 Igor Bukanov 2006-09-18 16:20:53 PDT
Created attachment 239098 [details] [diff] [review]
Fix v2b

Patch to commit with spelling-in-comments fixes.
Comment 22 Igor Bukanov 2006-09-18 23:49:32 PDT
I committed the patch from comment 21 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.125; previous revision: 3.124
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.97; previous revision: 3.96
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.119; previous revision: 3.118
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.177; previous revision: 3.176
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.58; previous revision: 3.57
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.133; previous revision: 3.132
done
Comment 23 Bob Clary [:bc:] 2006-09-19 06:26:10 PDT
Created attachment 239167 [details]
e4x/GC/regress-352846-01.js
Comment 24 Bob Clary [:bc:] 2006-09-19 06:26:36 PDT
Created attachment 239168 [details]
e4x/GC/regress-352846-02.js
Comment 25 Bob Clary [:bc:] 2006-09-19 06:27:04 PDT
Created attachment 239171 [details]
e4x/GC/regress-352846-03.js
Comment 26 Bob Clary [:bc:] 2006-09-19 15:58:06 PDT
verified fixed 1.9 20060919 windows/mac*/linux
Comment 27 Daniel Veditz [:dveditz] 2006-09-19 16:02:14 PDT
Restoring lost blocking flag
Comment 28 Igor Bukanov 2006-11-03 14:24:46 PST
Comment on attachment 239098 [details] [diff] [review]
Fix v2b

The patch applies to 1.8.1 as is.
Comment 29 Igor Bukanov 2006-11-03 14:48:22 PST
Created attachment 244623 [details] [diff] [review]
Fix v2b for 1.8.0 branch

The backport required non-minimal efforts warranting a review.
Comment 30 Igor Bukanov 2006-11-03 14:49:44 PST
Created attachment 244624 [details] [diff] [review]
diff between trunk and 1.8.0 version of patches

This is to simplify the review.
Comment 31 Brendan Eich [:brendan] 2006-11-03 21:38:52 PST
Comment on attachment 244623 [details] [diff] [review]
Fix v2b for 1.8.0 branch

r=me, thanks for the interdiff.

/be
Comment 32 Daniel Veditz [:dveditz] 2006-11-07 11:00:44 PST
Comment on attachment 239098 [details] [diff] [review]
Fix v2b

Approved for 1.8 branch, a=dveditz for drivers
Comment 33 Daniel Veditz [:dveditz] 2006-11-07 11:00:56 PST
Comment on attachment 244623 [details] [diff] [review]
Fix v2b for 1.8.0 branch

Approved for 1.8.0 branch, a=dveditz for drivers
Comment 34 Igor Bukanov 2006-11-07 11:30:42 PST
I committed the patch from comment 21 to MOZILLA_1_8_BRANCH:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.93.2.10; previous revision: 3.93.2.9
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.58.2.20; previous revision: 3.58.2.19
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.14; previous revision: 3.80.4.13
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.23; previous revision: 3.104.2.22
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.33.4.7; previous revision: 3.33.4.6
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.108.2.10; previous revision: 3.108.2.9
done
Comment 35 Igor Bukanov 2006-11-07 11:38:27 PST
Comment on attachment 244623 [details] [diff] [review]
Fix v2b for 1.8.0 branch

The patch does not include the fix for regression from dependant bug 355029. I will include it here.
Comment 36 Igor Bukanov 2006-11-07 11:45:38 PST
I took out the previous commit to include into the patch the regression fix from bug 355029.
Comment 37 Igor Bukanov 2006-11-07 11:49:46 PST
Created attachment 244914 [details] [diff] [review]
For 1.8.1: Fix v2b + regression fix from bug 355029

This is just a the previous patch + the fix from bug 355029 applied on top of it.
Comment 38 Igor Bukanov 2006-11-07 11:57:42 PST
Created attachment 244915 [details] [diff] [review]
Fix v2b for 1.8.0 branch + regression fix from bug 355029

This is again the previous version of the patch + the regression fix from bug 355029 applied on top of it.
Comment 39 Daniel Veditz [:dveditz] 2006-11-07 14:47:30 PST
Comment on attachment 244915 [details] [diff] [review]
Fix v2b for 1.8.0 branch + regression fix from bug 355029

approved for 1.8.0 branch, a=dveditz for drivers
Comment 40 Daniel Veditz [:dveditz] 2006-11-07 14:47:49 PST
Comment on attachment 244914 [details] [diff] [review]
For 1.8.1: Fix v2b + regression fix from bug 355029

approved for 1.8 branch, a=dveditz for drivers
Comment 41 Igor Bukanov 2006-11-22 00:30:04 PST
I committed the patch from comment 37 to MOZILLA_1_8_BRANCH:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.93.2.13; previous revision: 3.93.2.12
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.58.2.22; previous revision: 3.58.2.21
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.16; previous revision: 3.80.4.15
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.25; previous revision: 3.104.2.24
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.33.4.9; previous revision: 3.33.4.8
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.108.2.13; previous revision: 3.108.2.12
done
Comment 42 Igor Bukanov 2006-11-22 01:05:16 PST
I committed the patch from comment 38 to MOZILLA_1_8_0_BRANCH:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.93.2.2.2.5; previous revision: 3.93.2.2.2.4
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.58.2.10.2.10; previous revision: 3.58.2.10.2.9
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.2.2.9; previous revision: 3.80.4.2.2.8
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.3.2.14; previous revision: 3.104.2.3.2.13
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.33.12.3; previous revision: 3.33.12.2
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.108.2.5.2.5; previous revision: 3.108.2.5.2.4
done
Comment 43 Bob Clary [:bc:] 2006-11-23 01:49:24 PST
verified fixed 20061122 1.8.0.9 windows/linux/mac*, 1.8.1.1 windows/linux/mac*, 1.9 windows/linux
Comment 44 Bob Clary [:bc:] 2007-02-08 16:19:35 PST
/cvsroot/mozilla/js/tests/e4x/extensions/regress-352846-01.js,v  <--  regress-352846-01.js

/cvsroot/mozilla/js/tests/e4x/extensions/regress-352846-02.js,v  <--  regress-352846-02.js

/cvsroot/mozilla/js/tests/e4x/extensions/regress-352846-03.js,v  <--  regress-352846-03.js

moved to extensions/ due to toSource

Note You need to log in before you can comment on or make changes to this bug.