Last Comment Bug 561359 - "Assertion failure: &shape.methodObject() == &prev.toObject(),"
: "Assertion failure: &shape.methodObject() == &prev.toObject(),"
Status: RESOLVED FIXED
[softblocker][js-triage-done][inbound]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla9
Assigned To: Jason Orendorff [:jorendorff]
:
:
Mentors:
: 618575 635598 667108 (view as bug list)
Depends on: 617609 684178 684489 684779 684972 685864
Blocks: jsfunfuzz 558058
  Show dependency treegraph
 
Reported: 2010-04-23 07:44 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-14 07:54 PST (History)
16 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
.x+


Attachments
hack (2.33 KB, patch)
2010-11-18 10:32 PST, Luke Wagner [:luke]
jorendorff: feedback-
Details | Diff | Splinter Review
v1 (18.19 KB, patch)
2011-02-24 17:39 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v1, diff -b (11.10 KB, patch)
2011-02-24 17:40 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 - Only the one change mentioned in comments 25-26. (11.36 KB, patch)
2011-02-28 11:55 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2, diff -b (9.81 KB, patch)
2011-02-28 11:56 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v3 (18.32 KB, patch)
2011-03-04 12:44 PST, Jason Orendorff [:jorendorff]
brendan: review+
Details | Diff | Splinter Review
b3, diff -b (10.53 KB, patch)
2011-03-04 12:46 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v4 - same as v3, rebased to m-c tip (14.88 KB, patch)
2011-07-19 13:51 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
dump the GC heap every time the CC invokes the GC (1.38 KB, patch)
2011-08-17 16:07 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
added logging to trace ChromeWindow leaks (3.67 KB, patch)
2011-08-21 20:00 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
v5 (52.84 KB, patch)
2011-08-23 09:38 PDT, Jason Orendorff [:jorendorff]
dvander: review+
Details | Diff | Splinter Review
v5, diff -b (ignoring whitespace) (15.16 KB, patch)
2011-08-23 09:41 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2010-04-23 07:44:33 PDT
for (let z = 0; z < 2; z++) {
    with({
        x: function() {}
    }) {
        for (l in [x]) {}
    }
}

asserts js debug shell on TM tip without -j at Assertion failure: sprop->methodValue() == prev, at ../jsscope.cpp:1167
Comment 1 Gary Kwong [:gkw] [:nth10sd] 2010-04-23 08:05:10 PDT
autoBisect shows this is probably related to bug 558058:

The first bad revision is:
changeset:   40454:61de331861af
user:        Andreas Gal
date:        Thu Apr 08 07:53:09 2010 -0700
summary:     No need to lookup parent/proto for iterator objects, and cache the last free one (bug 558058, r=brendan).
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2010-07-16 19:57:54 PDT
Comment #0 now asserts in Assertion failure: &sprop->methodObject() == &prev.toObject(), at ../jsscope.cpp:1215
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2010-08-31 02:08:59 PDT
Comment #0 has further morphed to Assertion failure: &shape.methodObject() == &prev.toObject(),
Comment 4 Luke Wagner [:luke] 2010-11-17 18:40:53 PST
If I put the assert

  JS_ASSERT(&methodObject() == &pobj->nativeGetSlot(slot).toObject());

in Shape::get(), in the isMethod() branch, I get an assert with just:

  for (let z = 0; z < 2; z++)
      with({ x: function() {} }) { x; }

I can trace this assert back to JSOP_INITMETHOD on the second iteration where testForInit hits on the offending shape (whose methodObject is different than the value assigned to the obj slot).
Comment 5 Luke Wagner [:luke] 2010-11-18 10:32:20 PST
Created attachment 491583 [details] [diff] [review]
hack

This patch fixes comment 0 and comment 4 (with the tighter assert), but only for the interpreter and I'm not sure if this is even the right fix since I'm not very familiar with propcache invariants.

The bug is this:
 1. the first time around the loop, JSOP_INITMETHOD caches the transition from an empty object to an object with a single method property (function() {}).
 2. the with creates a call object
 3. the second time around the loop, JSOP_LAMBDA clones the compiled function object (b/c of the call object)
 4. the propcache hits and we transition from an empty object to the same shape as (1), whose methodObject is the compiled object and whose slot value is the clone, hence the assert on read.
Comment 6 Luke Wagner [:luke] 2010-11-23 18:22:02 PST
Jason said there are some big changes to INITMETHOD/SETMETHOD coming in so I think this should wait and reevaluate after those land.
Comment 7 Jason Orendorff [:jorendorff] 2010-11-24 15:35:58 PST
Comment on attachment 491583 [details] [diff] [review]
hack

I was wrong; the main changes landed. There's still a bit left in bug 614051, but it doesn't affect this.

If possible, filling more carefully would be better than this hack. But that depends on knowing ahead of time if the INITMETHOD optimization will fly or not. The distance in time between LAMBDA and SETMETHOD/INITMETHOD is an issue. I need to look closer at what JSOP_LAMBDA is doing, and understand the root cause of this bug better. Setting f?me.
Comment 8 Jason Orendorff [:jorendorff] 2010-11-29 09:05:34 PST
Comment on attachment 491583 [details] [diff] [review]
hack

This patch should be a last resort. More explanation coming in a moment.
Comment 9 Jason Orendorff [:jorendorff] 2010-11-29 09:11:04 PST
Brendan, please take a look.

To paraphrase Luke's comment 5:
  - First time through JSOP_LAMBDA, fp->scopeChain() is the global, so
    the method optimization is enabled.
  - JSOP_INITMETHOD fills the property cache with the method shape.
  - The with-statement forces the enclosing Block to be reified.
  - Second time through, fp->scopeChain() is the Block object, so the
    method optimization is disabled.

The bug is that the method optimization depends on whether or not an enclosing Block happens to have been reified. That's not deterministic enough: for cache correctness, JSOP_LAMBDA to apply the method optimization based solely on criteria known at jsemit time and/or covered by the recipient object's shape.

(JSStackFrame::scopeChain doesn't actually get the scope chain. It exposes the optimization that we reify scope objects lazily. JSStackFrame::notTheScopeChain would be a better name for it.)

That we setMethodAtom at run time in JSOP_LAMBDA, instead of doing it statically in the emitter, also seems weird.

One approach is to get rid of that `if (obj->parent == parent)` test and use `script->compileAndGo` instead. But it's not quite that simple:

 1. There's a case where a JSFunction can be a null closure, or claim to be
    one, but its behavior actually depends on Call objects and such:

        function f(s) {
            var obj = {m: function () { return a; }};
            eval(s);
            return obj;
        }
        var obj = f("var a = 'right';");
        var a = 'wrong';
        assertEq(obj.m(), 'right');

    Here the function obj.m is a null closure, and we emit JSOP_INITMETHOD;
    but JSOP_LAMBDA sees a Call object and so the method optimization is
    inhibited: we pass the test. The change I propose would break this.

    But it seems to me obj.m shouldn't be a null closure.

 2. Brendan mentioned on IRC that the `if (obj->parent == parent)` test in
    JSOP_LAMBDA protects against API abuse, where a compile-n-go script is
    incorrectly executed in a different scope from the one where it was
    compiled. But such abuse is an expressway to crashville. We should assert
    against it in JS_Execute*. We currently don't, because the scope object
    passed to JS_Compile* is discarded after compilation. In DEBUG builds, we
    should hang on to it.

Luke, do you still want this one? I can take.
Comment 10 Luke Wagner [:luke] 2010-11-29 09:18:04 PST
You got it partner; I was just blocker hunting.
Comment 11 Luke Wagner [:luke] 2010-12-11 14:47:25 PST
*** Bug 618575 has been marked as a duplicate of this bug. ***
Comment 12 Brendan Eich [:brendan] 2011-01-03 20:44:05 PST
Bug 494637 seems relevant -- if we fixed it, would we have anything to do here?

/be
Comment 13 Jason Orendorff [:jorendorff] 2011-01-12 15:42:22 PST
Yes, I think we'd still have work here. (Though, bug 494637 is not a good thing to have lying around either.)

Fixing now.
Comment 14 Jason Orendorff [:jorendorff] 2011-01-12 16:44:20 PST
This depends on the bug where some functions are marked as null closures when they shouldn't be (because they are inside a with-statement or a function that calls eval directly). Does anyone know that bug#? I can't find it.

I think it'll be pretty easy to fix that bug as far as direct eval is concerned. To detect enclosing with-statements, I think I need a new tcflag or something, to be set in Parser::functionDef. Also fairly straightforward.

Brendan, you are the likely reviewer here. Let me know if I'm off target!
Comment 15 Jason Orendorff [:jorendorff] 2011-01-12 16:45:38 PST
methodjit's treatment of JSOP_LAMBDA could be better, I think -- more of the branches can be decided statically, leaving less to be done at run time -- but we can tidy that up in a follow-up bug.
Comment 16 David Anderson [:dvander] 2011-01-12 17:03:33 PST
(In reply to comment #14)
> This depends on the bug where some functions are marked as null closures when
> they shouldn't be (because they are inside a with-statement or a function that
> calls eval directly). Does anyone know that bug#? I can't find it.


bug 617609 maybe?
Comment 17 Jason Orendorff [:jorendorff] 2011-01-13 10:07:11 PST
That's the one. Thanks.
Comment 18 Gary Kwong [:gkw] [:nth10sd] 2011-01-22 20:13:52 PST
(In reply to comment #17)
> That's the one. Thanks.

bug 617609 is now fixed-on-TM. :)
Comment 19 Gary Kwong [:gkw] [:nth10sd] 2011-02-16 18:22:42 PST
In the wake of all softblocker blocking2.0-final+ bugs soon-to-be switched to blocking2.0-, renominating this one in the hope of getting a ".x" flag.

The seemingly endless switching of the assert message in this bug every now and then is annoying but nothing too severe for the fuzzers to handle.
Comment 20 Jason Orendorff [:jorendorff] 2011-02-23 12:15:29 PST
*** Bug 635598 has been marked as a duplicate of this bug. ***
Comment 21 Brendan Eich [:brendan] 2011-02-23 15:16:59 PST
I've been stalling review to work on blockers. Let me know if that's wrong.

/be
Comment 22 Jason Orendorff [:jorendorff] 2011-02-24 17:39:36 PST
Created attachment 514963 [details] [diff] [review]
v1

There are basically three things going on here.

1. fun->joinable() should be false if we were not compiled with the COMPILE_N_GO flag. So fix that.

2. Don't emit JSOP_{INIT,SET}METHOD after a JSOP_LAMBDA for a function that isn't joinable. Duh.

3. Tidy up afterwards. Use fun->joinable() instead of wordier ways of saying the same thing. Eliminate a few branches. This part changed a lot of indentation, so I'll post a diff -b version in a sec.
Comment 23 Jason Orendorff [:jorendorff] 2011-02-24 17:40:28 PST
Created attachment 514964 [details] [diff] [review]
v1, diff -b
Comment 24 Brendan Eich [:brendan] 2011-02-25 14:32:33 PST
Why the parent to scope chain (unreified) removal in the JSOP_LAMBDA-interp case?

That seems desirable to detect a joined function object and avoid the further overhead, but if it is a bogus test (it may be already due to scope chain variability for a given op in a loop) a better test still seems worth doing.

/be
Comment 25 Jason Orendorff [:jorendorff] 2011-02-25 15:18:35 PST
It is definitely a bogus test. That's the line of code that causes the bug.

(comment 9)
> The bug is that the method optimization depends on whether or not an enclosing
> Block happens to have been reified. That's not deterministic enough[...]

But yes, a better test can be done. I can test fun->joinable() instead. How about that?
Comment 26 Brendan Eich [:brendan] 2011-02-25 15:21:02 PST
Seems like exactly the right test! Sorry, slow here due to cold.

/be
Comment 27 Brendan Eich [:brendan] 2011-02-26 18:25:14 PST
Mentioned on IRC: compile-time "joinable" makes sense but once we've decided, "joined" for function objects is better.

/be
Comment 28 Jason Orendorff [:jorendorff] 2011-02-28 11:55:49 PST
Created attachment 515698 [details] [diff] [review]
v2 - Only the one change mentioned in comments 25-26.
Comment 29 Jason Orendorff [:jorendorff] 2011-02-28 11:56:18 PST
Created attachment 515700 [details] [diff] [review]
v2, diff -b
Comment 30 Brendan Eich [:brendan] 2011-02-28 12:19:43 PST
Comment on attachment 515698 [details] [diff] [review]
v2 - Only the one change mentioned in comments 25-26.

>diff --git a/js/src/jscompartment.cpp b/js/src/jscompartment.cpp
>--- a/js/src/jscompartment.cpp
>+++ b/js/src/jscompartment.cpp
>@@ -213,18 +213,17 @@ JSCompartment::wrap(JSContext *cx, Value
>             JS_ASSERT(str->asCell()->compartment() == cx->runtime->atomsCompartment);
>             return true;
>         }
>     }
> 
>     /*
>      * Wrappers should really be parented to the wrapped parent of the wrapped
>      * object, but in that case a wrapped global object would have a NULL
>-     * parent without being a proper global object (JSCLASS_IS_GLOBAL). Instead
>-,
>+     * parent without being a proper global object (JSCLASS_IS_GLOBAL). Instead,
>      * we parent all wrappers to the global object in their home compartment.
>      * This loses us some transparency, and is generally very cheesy.
>      */
>     JSObject *global;
>     if (cx->hasfp()) {
>         global = cx->fp()->scopeChain().getGlobal();
>     } else {
>         global = cx->globalObject;
>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -5598,17 +5598,17 @@ BEGIN_CASE(JSOP_LAMBDA)
>     JSObject *obj = FUN_OBJECT(fun);
> 
>     /* do-while(0) so we can break instead of using a goto. */
>     do {
>         JSObject *parent;
>         if (FUN_NULL_CLOSURE(fun)) {
>             parent = &regs.fp->scopeChain();
> 
>-            if (obj->getParent() == parent) {
>+            if (fun->joinable()) {
>                 jsbytecode *pc2 = AdvanceOverBlockchainOp(regs.pc + JSOP_LAMBDA_LENGTH);
>                 JSOp op2 = JSOp(*pc2);
> 
>                 /*
>                  * Optimize var obj = {method: function () { ... }, ...},
>                  * this.method = function () { ... }; and other significant
>                  * single-use-of-null-closure bytecode sequences.
>                  *
>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>--- a/js/src/jsparse.cpp
>+++ b/js/src/jsparse.cpp
>@@ -304,17 +304,19 @@ Parser::newFunctionBox(JSObject *obj, JS
>         funbox->tcflags |= TCF_IN_WITH;
>     return funbox;
> }
> 
> bool
> JSFunctionBox::joinable() const
> {
>     return FUN_NULL_CLOSURE((JSFunction *) object) &&
>-           !(tcflags & (TCF_FUN_USES_ARGUMENTS | TCF_FUN_USES_OWN_NAME));
>+           (tcflags & (TCF_FUN_USES_ARGUMENTS |
>+                       TCF_FUN_USES_OWN_NAME |
>+                       TCF_COMPILE_N_GO)) == TCF_COMPILE_N_GO;
> }
> 
> bool
> JSFunctionBox::inAnyDynamicScope() const
> {
>     for (const JSFunctionBox *funbox = this; funbox; funbox = funbox->parent) {
>         if (funbox->tcflags & (TCF_IN_WITH | TCF_FUN_CALLS_EVAL))
>             return true;
>@@ -4713,18 +4715,16 @@ CloneParseTree(JSParseNode *opn, JSTreeC
> 
> #undef NULLCHECK
>     }
>     return pn;
> }
> 
> #endif /* JS_HAS_DESTRUCTURING */
> 
>-extern const char js_with_statement_str[];
>-
> static JSParseNode *
> ContainsStmt(JSParseNode *pn, TokenKind tt)
> {
>     JSParseNode *pn2, *pnt;
> 
>     if (!pn)
>         return NULL;
>     if (PN_TYPE(pn) == tt)
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -15580,17 +15580,17 @@ TraceRecorder::record_JSOP_LAMBDA()
>      * via identity and mutation. But don't clone if our result is consumed by
>      * JSOP_SETMETHOD or JSOP_INITMETHOD, since we optimize away the clone for
>      * these combinations and clone only if the "method value" escapes.
>      *
>      * See jsinterp.cpp, the JSOP_LAMBDA null closure case. The JSOP_SETMETHOD and
>      * JSOP_INITMETHOD logic governing the early ARECORD_CONTINUE returns below
>      * must agree with the corresponding break-from-do-while(0) logic there.
>      */
>-    if (FUN_NULL_CLOSURE(fun) && FUN_OBJECT(fun)->getParent() == &cx->fp()->scopeChain()) {
>+    if (fun->joinable()) {
>         jsbytecode *pc2 = AdvanceOverBlockchainOp(cx->regs->pc + JSOP_LAMBDA_LENGTH);
>         JSOp op2 = JSOp(*pc2);
> 
>         if (op2 == JSOP_INITMETHOD) {
>             stack(0, w.immpObjGC(FUN_OBJECT(fun)));
>             return ARECORD_CONTINUE;
>         }
> 
>diff --git a/js/src/methodjit/StubCalls.cpp b/js/src/methodjit/StubCalls.cpp
>--- a/js/src/methodjit/StubCalls.cpp
>+++ b/js/src/methodjit/StubCalls.cpp
>@@ -1415,70 +1415,78 @@ stubs::RegExp(VMFrame &f, JSObject *rege
>     if (!obj)
>         THROWV(NULL);
>     return obj;
> }
> 
> JSObject * JS_FASTCALL
> stubs::LambdaForInit(VMFrame &f, JSFunction *fun)
> {
>+    JS_ASSERT(fun->joinable());
>+    JS_ASSERT(FUN_NULL_CLOSURE(fun));
>+    JS_ASSERT(f.fp()->script()->compileAndGo);
>     JSObject *obj = FUN_OBJECT(fun);
>-    if (FUN_NULL_CLOSURE(fun) && obj->getParent() == &f.fp()->scopeChain()) {
>+    JS_ASSERT(obj->getParent() != NULL);
>+
>+    fun->setMethodAtom(f.fp()->script()->getAtom(GET_SLOTNO(f.regs.pc)));
>+    return obj;
>+}
>+
>+JSObject * JS_FASTCALL
>+stubs::LambdaForSet(VMFrame &f, JSFunction *fun)
>+{
>+    JS_ASSERT(fun->joinable());
>+    JS_ASSERT(FUN_NULL_CLOSURE(fun));
>+    JS_ASSERT(f.fp()->script()->compileAndGo);
>+    JSObject *obj = FUN_OBJECT(fun);
>+    JS_ASSERT(obj->getParent() != NULL);
>+
>+    const Value &lref = f.regs.sp[-1];
>+    if (lref.isObject() && lref.toObject().canHaveMethodBarrier()) {
>         fun->setMethodAtom(f.fp()->script()->getAtom(GET_SLOTNO(f.regs.pc)));
>         return obj;
>     }
>     return Lambda(f, fun);
> }
> 
> JSObject * JS_FASTCALL
>-stubs::LambdaForSet(VMFrame &f, JSFunction *fun)
>-{
>-    JSObject *obj = FUN_OBJECT(fun);
>-    if (FUN_NULL_CLOSURE(fun) && obj->getParent() == &f.fp()->scopeChain()) {
>-        const Value &lref = f.regs.sp[-1];
>-        if (lref.isObject() && lref.toObject().canHaveMethodBarrier()) {
>-            fun->setMethodAtom(f.fp()->script()->getAtom(GET_SLOTNO(f.regs.pc)));
>-            return obj;
>-        }
>-    }
>-    return Lambda(f, fun);
>-}
>-
>-JSObject * JS_FASTCALL
> stubs::LambdaJoinableForCall(VMFrame &f, JSFunction *fun)
> {
>+    JS_ASSERT(fun->joinable());
>+    JS_ASSERT(FUN_NULL_CLOSURE(fun));
>+    JS_ASSERT(f.fp()->script()->compileAndGo);
>     JSObject *obj = FUN_OBJECT(fun);
>-    if (FUN_NULL_CLOSURE(fun) && obj->getParent() == &f.fp()->scopeChain()) {
>-        /*
>-         * Array.prototype.sort and String.prototype.replace are
>-         * optimized as if they are special form. We know that they
>-         * won't leak the joined function object in obj, therefore
>-         * we don't need to clone that compiler- created function
>-         * object for identity/mutation reasons.
>-         */
>-        int iargc = GET_ARGC(f.regs.pc);
>+    JS_ASSERT(obj->getParent() != NULL);
> 
>-        /*
>-         * Note that we have not yet pushed obj as the final argument,
>-         * so regs.sp[1 - (iargc + 2)], and not regs.sp[-(iargc + 2)],
>-         * is the callee for this JSOP_CALL.
>-         */
>-        const Value &cref = f.regs.sp[1 - (iargc + 2)];
>-        JSObject *callee;
>+    /*
>+     * Array.prototype.sort and String.prototype.replace are
>+     * optimized as if they are special form. We know that they
>+     * won't leak the joined function object in obj, therefore
>+     * we don't need to clone that compiler- created function
>+     * object for identity/mutation reasons.
>+     */
>+    int iargc = GET_ARGC(f.regs.pc);
> 
>-        if (IsFunctionObject(cref, &callee)) {
>-            JSFunction *calleeFun = callee->getFunctionPrivate();
>-            Native native = calleeFun->maybeNative();
>+    /*
>+     * Note that we have not yet pushed obj as the final argument,
>+     * so regs.sp[1 - (iargc + 2)], and not regs.sp[-(iargc + 2)],
>+     * is the callee for this JSOP_CALL.
>+     */
>+    const Value &cref = f.regs.sp[1 - (iargc + 2)];
>+    JSObject *callee;
> 
>-            if (native) {
>-                if (iargc == 1 && native == array_sort)
>-                    return obj;
>-                if (iargc == 2 && native == str_replace)
>-                    return obj;
>-            }
>+    if (IsFunctionObject(cref, &callee)) {
>+        JSFunction *calleeFun = callee->getFunctionPrivate();
>+        Native native = calleeFun->maybeNative();
>+
>+        if (native) {
>+            if (iargc == 1 && native == array_sort)
>+                return obj;
>+            if (iargc == 2 && native == str_replace)
>+                return obj;
>         }
>     }
>     return Lambda(f, fun);
> }
> 
> JSObject * JS_FASTCALL
> stubs::LambdaJoinableForNull(VMFrame &f, JSFunction *fun)
> {
>diff --git a/js/src/tests/js1_8_5/regress/jstests.list b/js/src/tests/js1_8_5/regress/jstests.list
>--- a/js/src/tests/js1_8_5/regress/jstests.list
>+++ b/js/src/tests/js1_8_5/regress/jstests.list
>@@ -16,16 +16,20 @@ script regress-553778.js
> script regress-555246-0.js
> script regress-555246-1.js
> script regress-559402-1.js
> script regress-559402-2.js
> script regress-559438.js
> script regress-560101.js
> script regress-560998-1.js
> script regress-560998-2.js
>+script regress-561359-1.js
>+script regress-561359-2.js
>+script regress-561359-3.js
>+script regress-561359-4.js
> script regress-563210.js
> script regress-563221.js
> script regress-566549.js
> script regress-566914.js
> script regress-567152.js
> script regress-569306.js
> script regress-569464.js
> script regress-571014.js
>diff --git a/js/src/tests/js1_8_5/regress/regress-561359-1.js b/js/src/tests/js1_8_5/regress/regress-561359-1.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/js1_8_5/regress/regress-561359-1.js
>@@ -0,0 +1,13 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+// Contributor: Gary Kwong <gary@rumblingedge.com>
>+
>+for (let z = 0; z < 2; z++) {
>+    with({
>+        x: function() {}
>+    }) {
>+        for (l in [x]) {}
>+    }
>+}
>+
>+reportCompare(0, 0, 'ok');
>diff --git a/js/src/tests/js1_8_5/regress/regress-561359-2.js b/js/src/tests/js1_8_5/regress/regress-561359-2.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/js1_8_5/regress/regress-561359-2.js
>@@ -0,0 +1,14 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+// Contributor: Jason Orendorff <jorendorff@mozilla.com>
>+
>+function f(s) {
>+    var obj = {m: function () { return a; }};
>+    eval(s);
>+    return obj;
>+}
>+var obj = f("var a = 'right';");
>+var a = 'wrong';
>+assertEq(obj.m(), 'right');
>+
>+reportCompare(0, 0, 'ok');
>diff --git a/js/src/tests/js1_8_5/regress/regress-561359-3.js b/js/src/tests/js1_8_5/regress/regress-561359-3.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/js1_8_5/regress/regress-561359-3.js
>@@ -0,0 +1,13 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+// Contributor: Jason Orendorff <jorendorff@mozilla.com>
>+
>+function f(s) {
>+    with (s)
>+        return {m: function () { return a; }};
>+}
>+var obj = f({a: 'right'});
>+var a = 'wrong';
>+assertEq(obj.m(), 'right');
>+
>+reportCompare(0, 0, 'ok');
>diff --git a/js/src/tests/js1_8_5/regress/regress-561359-4.js b/js/src/tests/js1_8_5/regress/regress-561359-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/js1_8_5/regress/regress-561359-4.js
>@@ -0,0 +1,10 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+// Contributor: Jason Orendorff <jorendorff@mozilla.com>
>+
>+var x;
>+for (let i = 0; i < 2; i++)
>+    x = {m: function () {}, n: function () { i++; }};
>+x.m;
>+
>+reportCompare(0, 0, 'ok');
Comment 31 Brendan Eich [:brendan] 2011-02-28 17:33:40 PST
Argh, how did that happen? Chrome bug, I think (my Minefield was rebuilding and I could not get Chrome to switch to edit the patch). Sorry for this mess.

I think I wrote something like this in the small textarea, which was trumped by the (invisible) switch to patch-editing mode:

>-    if (FUN_NULL_CLOSURE(fun) && FUN_OBJECT(fun)->getParent() == &cx->fp()->scopeChain()) {
>+    if (fun->joinable()) {

Don't we need to keep the FUN_NULL_CLOSURE(fun) test to match the interpreter?

/be
Comment 32 Jason Orendorff [:jorendorff] 2011-03-01 09:56:09 PST
No, because only null closures are joinable.
Comment 33 Brendan Eich [:brendan] 2011-03-01 14:11:33 PST
(In reply to comment #32)
> No, because only null closures are joinable.

Right (I knew that, I swear! ;-)) but this suggests making the interpreter case for JSOP_LAMBDA test only fun->joinable() in the outer if that currently tests FUN_NULL_CLOSURE(fun), and get rid of the inner if. Right?

The #ifdef DEBUG code that updates rt->unjoinedFunctionCountMap would kick in only for joined function objects that could not be optimized via the method barrier, but that seems better too.

/be
Comment 34 Jason Orendorff [:jorendorff] 2011-03-02 15:02:20 PST
Yes, that totally makes sense.
Comment 35 Jason Orendorff [:jorendorff] 2011-03-04 12:44:44 PST
Created attachment 516976 [details] [diff] [review]
v3
Comment 36 Jason Orendorff [:jorendorff] 2011-03-04 12:46:25 PST
Created attachment 516977 [details] [diff] [review]
b3, diff -b
Comment 37 Brendan Eich [:brendan] 2011-03-04 18:18:44 PST
Comment on attachment 516976 [details] [diff] [review]
v3

Thanks, r=me.

/be
Comment 38 Gary Kwong [:gkw] [:nth10sd] 2011-03-14 21:20:07 PDT
(In reply to comment #35)
> Created attachment 516976 [details] [diff] [review]
> v3

Can this pls be landed soon? (Just hoping to prevent bitrot)
Comment 39 Jason Orendorff [:jorendorff] 2011-03-16 13:59:41 PDT
I landed it Monday: http://hg.mozilla.org/tracemonkey/rev/39de74c74b20
but it bounced:     http://hg.mozilla.org/tracemonkey/rev/11c970a9eacc

It reliably causes windows to leak here when I start the browser. :-|
Comment 40 Brendan Eich [:brendan] 2011-05-24 13:22:38 PDT
Is this bug going to get fixed? Can I help? Just LMK, thanks.

/be
Comment 41 Robert Sayre 2011-05-24 14:22:55 PDT
nominate with a reason for tracking-6
Comment 42 Jason Orendorff [:jorendorff] 2011-07-19 13:50:23 PDT
A patch in bug 666713 is hitting this. Updating the patch.
Comment 43 Jason Orendorff [:jorendorff] 2011-07-19 13:51:04 PDT
Created attachment 546893 [details] [diff] [review]
v4 - same as v3, rebased to m-c tip
Comment 44 Jason Orendorff [:jorendorff] 2011-07-25 23:31:39 PDT
This patch still causes DOMWindows to leak when I open and close the browser.
How do you figure out this kind of problem? I can only think of two approaches:

 1. Figure out where the printf is that says we leaked windows, and dump
    the JS heap at that point, then figure out what path through the graph
    is keeping windows alive and try and figure out what it might have to do
    with this patch.

 2. Reduce the patch and figure out the minimum change that causes the leak.
    Try to figure out what extra edges that change adds.

#2 sounds more appealing. Anyone else have any ideas?
Comment 45 David Mandelin [:dmandelin] 2011-07-26 10:24:20 PDT
peterv, what other tricks do you have for debugging leaking DOMWindows?
Comment 46 Andrew McCreight [:mccr8] 2011-07-26 10:27:14 PDT
You can dump the cycle collector graph, then use it to see which object is making the CC hold on to the window.  That object will be something where the number of known references the CC sees is less than the ref count.  Sometimes it will just be the window itself that has an unknown reference, but it can be useful.

Once you have that, you can do refcount logging on the object holding on to the window, to see what is holding onto it.
Comment 47 Andrew McCreight [:mccr8] 2011-07-26 10:51:50 PDT
I've been meaning to write this up in detail, as I've had to explain it to a number of people so far, but I haven't gotten around to it yet...
Comment 48 Jason Orendorff [:jorendorff] 2011-08-06 08:10:05 PDT
This bug needs an owner. Please steal it. It's important to get bug 666713 landed quickly, and that means either fixing this bug or finding some kind of temporary hack so that it doesn't crash (see bug 666713 comment 16 through 19).

Comment 46 and option 2 in comment 44 still seem like the best things to try next.
Comment 49 Andrew McCreight [:mccr8] 2011-08-16 14:19:32 PDT
I'll look into the leak caused by this patch.
Comment 50 Andrew McCreight [:mccr8] 2011-08-17 11:57:39 PDT
There are two nsGlobalWindows that leak with this patch, just from opening and closing the browser.  One is just the inner window, so this is really just one that is leaking.

Here is what the cycle collector graph says is rooting this object:

    0x118de3628 [JS Object (ChromeWindow) (global=118de3628)]
        --[xpc_GetJSPrivate(obj)]-> 0x118e66a00 [XPCWrappedNative (ChromeWindow)]
        --[mIdentity]-> 0x105694fe8 [nsGlobalWindow]

In other words, there's a JS Object holding on to an XPCWrappedNative, which is holding onto the window.  The A --[FOO]-> B notation means that object A holds onto object B via a field or whatever named FOO.

The JS Object (ChromeWindow) is a root because the GC marked it, which means it is reachable from a JS root.  I'll try a JS GC dump to figure out why the GC is rooting the  JS Object (ChromeWindow).
Comment 51 Andrew McCreight [:mccr8] 2011-08-17 15:40:05 PDT
I set up XPConnect to do a JS_DumpHeap every time the cycle collector invokes the GC.  The JS Object (ChromeWindow) shows up exactly once on the left side (the address is different because this is a different run, but it is the same object):

0x11a814a60 ChromeWindow 1055b4570 via mScopeObject

I assume this means that the ChromeWindow is being stored directly on mScopeObject, and that's why it is kept alive.  I don't know what mScopeObject is, so I'm not sure what that means.  The string mScopeObject doesn't appear in the patch anywhere.

sfink said in IRC: "all I can tell is that it ends up as the aScope for CallEventHandler at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1827"

In summary, the nsGlobalWindow that is leaking is being held alive in the cycle collector by a ChromeWindow.  The ChromeWindow is (maybe) being held alive in the garbage collector because it is being held by mScopeObject.

Unfortunately, I think this is all I will really be able to do, not knowing anything about the context.  If anybody else wants to look into this, I can let you know how to reproduce at least what I have found so far.
Comment 52 Andrew McCreight [:mccr8] 2011-08-17 16:07:57 PDT
Created attachment 553941 [details] [diff] [review]
dump the GC heap every time the CC invokes the GC

This patch will dump a JS GC heap log to a file gc-edges-X.log every time the CC invokes the GC.  The CC does this every time at shutdown, which is what we are interested in for shutdown leaks.

The easiest way to dump a CC log at every CC is to apply the patch in Bug 679779, then uncomment the line //#define ALWAYS_LOG_CC_GRAPH

Here's a rough guide to what I did.

Apply these two patches, rebuild Firefox, then start up Firefox, and exit it.  This will produce a number of cc-edges-X.log and gc-edges-X.log files.  You are interested in the last ones produced, as they are from the final GC and CC before the CC gave up because it stopped finding anything.  Look at the last cc-edges file, for the nsGlobalWindow.  There will be two.  One has no children.  This is the inner window, and is not interesting.  The other one is the one being held onto.  Write down the address of this object.  If you trace back a couple of steps, you'll find the Chrome window I describe.  Write down the address of that window, then look it up in the GC log file.
Comment 53 Andrew McCreight [:mccr8] 2011-08-18 09:40:42 PDT
jorendorff found what looks like the source of the mScopeObject rooting:
  http://mxr.mozilla.org/mozilla-central/source/dom/src/events/nsJSEventListener.cpp#118

I'll try to hunt down somebody who knows that code.
Comment 54 Andrew McCreight [:mccr8] 2011-08-18 11:13:25 PDT
The ChromeWindow in question is the mScopeObject for a huge number of nsJSEventListeners.  I'm not sure why nsJSEventListeners are acting as a black root (as opposed to a grey one) on the JS side.
Comment 55 Andrew McCreight [:mccr8] 2011-08-18 18:22:21 PDT
I'm pretty sure the nsJSEventListener is just a red herring.  I need to get JS_DumpHeap to dump all paths to the ChromeWindow, not just the first one...
Comment 56 Andrew McCreight [:mccr8] 2011-08-19 17:04:51 PDT
Okay, after multiple rounds of hacking on things, I made JS_DumpHeap not display anything reachable only from grey roots, which helped clear out red herrings like nsJSEventListener.  The remaining paths (of length <= 10) from black roots to the ChromeWindow look like this (after deleting addresses):

ChromeWindow via machine stack(BackstagePass).PlacesUtils(Object)._shutdownFunctions(Array).element[0](Function).parent(Block).parent(Call).XYZ

XYZ varies, but starts with either aURI, aCallback, or aScope.  Here's an example: aURI(XPCWrappedNative_NoHelper).proto(XPC_WN_NoMods_NoCall_Proto_JSClass).parent

So, it looks like a function is being stuck into the shutdown functions array of BackstagePass, and that function seems to be dragging along its entire context, which includes the ChromeWindow, which in turn drags along the nsGlobalWindow.  Hopefully that helps somebody who understands the patch.
Comment 57 Jason Orendorff [:jorendorff] 2011-08-21 13:35:11 PDT
Andrew, thank you so much. Victory! Can you easily get the fun->script()->filename and fun->script()->lineno of the function or functions at issue here? If so, I could then look at the source and produce a small test case. If not, I'll ponder some more and come back to you if I get stuck.

Before I forget: it sounds like your hacks (1) nailed the problem (2) involved a nontrivial amount of work to create. We should check that stuff in (enabled by a parameter to JS_DumpHeap or something, not an #ifdef) ...unless you can think of something even better we could do instead. I'll be happy to file a follow-up bug for that if you like.

Taking this bug back. If I recall correctly, the patch was only intended to affect null closures, and it wasn't intended ever to turn a null closure into a "full" closure (parented to the enclosing Call). If so, the leak must stem from a bug in the patch. I will look at it next week.
Comment 58 Andrew McCreight [:mccr8] 2011-08-21 14:27:58 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #57)
> Andrew, thank you so much. Victory! Can you easily get the
> fun->script()->filename and fun->script()->lineno of the function or
> functions at issue here? If so, I could then look at the source and produce
> a small test case. If not, I'll ponder some more and come back to you if I
> get stuck.
Sure, that sounds doable.  Would that be from the "Function" part of the path I showed above?  Or something deeper in?

> Before I forget: it sounds like your hacks (1) nailed the problem (2)
> involved a nontrivial amount of work to create. We should check that stuff
> in (enabled by a parameter to JS_DumpHeap or something, not an #ifdef)
> ...unless you can think of something even better we could do instead. I'll
> be happy to file a follow-up bug for that if you like.
It wasn't too bad in the end, it was mostly a matter of figuring out what to do.  I filed Bug 680482 about a replacement for JS_DumpHeap, but a few smaller tweaks might be helpful in the mean time.
Comment 59 Andrew McCreight [:mccr8] 2011-08-21 19:45:57 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #57)
> Andrew, thank you so much. Victory! Can you easily get the
> fun->script()->filename and fun->script()->lineno of the function or
> functions at issue here?

The filename is resource://gre/modules/PlacesUtils.jsm and lineno is 2088.

Which does not look very helpful to me:
      +     "SELECT 1 FROM moz_items_annos a "
Comment 60 Andrew McCreight [:mccr8] 2011-08-21 20:00:16 PDT
Created attachment 554783 [details] [diff] [review]
added logging to trace ChromeWindow leaks

With this patch, every time the CC calls the GC, it will dump a heap to a file with a name like "gc-edges-1.log".  This heap dump will not visit any grey roots.  It also prints out the file and line number of (some) functions.  (It also sets the thingToFind to the first ChromeWindow it finds, but I don't think that really matters as it turns out.)

To reproduce what I've shown here with both my patch and jorendorff's patches applied:
1. start the browser and close it right away.
2. open the last gc-edges file, which should be gc-edges-3.log
3. search in the file for " ChromeWindow ".  All lines with this string are paths from black roots to the leaking ChromeWindow.
4. Within one of those lines, look for the part of the path that looks like ".element[0](0x123066a28 Function).parent".  0x123066a28 (or whatever) is the address of the function.
5. Search for the line containing "0x123066a28 Function ", it will contain the file and line number like so:
0x123066a28 Function 11ae0d180 resource://gre/modules/PlacesUtils.jsm:2088
Comment 61 Jason Orendorff [:jorendorff] 2011-08-22 13:10:21 PDT
(In reply to Andrew McCreight [:mccr8] from comment #59)
> (In reply to Jason Orendorff [:jorendorff] from comment #57)
> > Andrew, thank you so much. Victory! Can you easily get the
> > fun->script()->filename and fun->script()->lineno of the function or
> > functions at issue here?
> 
> The filename is resource://gre/modules/PlacesUtils.jsm and lineno is 2088.
> 
> Which does not look very helpful to me:
>       +     "SELECT 1 FROM moz_items_annos a "

There are some #ifdefs in this source file. In my build directory, the preprocessed .jsm file has this at line 2088:

      this.registerShutdownFunction(function () {
        this._asyncGetBookmarksStmt.finalize();
      });

And note that there are indeed arguments named aURI, aCallback, and aScope in the enclosing function. This is the one.
Comment 62 Jason Orendorff [:jorendorff] 2011-08-23 07:48:20 PDT
Oh, I see. This patch was only supposed to affect the method optimization. The leak happens because it also completely disables the null closure optimization in non-compile-and-go code. Oops.

This will be easy to fix, I think.
Comment 63 Jason Orendorff [:jorendorff] 2011-08-23 09:38:25 PDT
Created attachment 555117 [details] [diff] [review]
v5

The -b version will be a lot easier to review, give me a sec...
Comment 64 Jason Orendorff [:jorendorff] 2011-08-23 09:41:44 PDT
Created attachment 555118 [details] [diff] [review]
v5, diff -b (ignoring whitespace)

OK. In the methodjit, this moves a few of the branches in JSOP_LAMBDA from runtime back to compile time. Nothing too tricky.

LambdaJoinableForNull becomes a no-op; I don't have enough methodjit-fu to eliminate the stub call in that case. Maybe you can suggest a mini-patch that I can fold into this one.
Comment 65 David Anderson [:dvander] 2011-08-29 14:58:13 PDT
Comment on attachment 555117 [details] [diff] [review]
v5

Sorry for the delay here, and thanks for the whitespace-adusted diff!
Comment 66 Jason Orendorff [:jorendorff] 2011-08-30 17:42:02 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/9eaca4ef5880
Comment 67 Marco Bonardo [::mak] 2011-08-31 02:08:38 PDT
http://hg.mozilla.org/mozilla-central/rev/9eaca4ef5880
Comment 68 Gary Kwong [:gkw] [:nth10sd] 2011-08-31 04:22:24 PDT
Getting this fixed does help, thanks all!
Comment 69 Jason Orendorff [:jorendorff] 2011-08-31 07:57:06 PDT
*** Bug 667108 has been marked as a duplicate of this bug. ***
Comment 70 Christian Holler (:decoder) 2013-01-14 07:54:16 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug561359-1.js.

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