From 0d83c6fc1e45162aea0dfd30640fbc4b8a89e387 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 8 Dec 2011 14:51:35 -0800 Subject: [PATCH] Bug 708838: Use findReferences instead of countHeap to make js1_8/extensions/regress-422269.js fail less randomly. r=jwalden The SpiderMonkey test js/src/tests/js1_8/extensions/regress-422269.js fails randomly, because it requires a certain object to be garbage-collected to pass. With a conservative stack scanner, an object being retained is not necessarily a bug, and in general, the engine makes no promises about which objects it retains and which it doesn't. The JavaScript shell's new findReferences function allows us to find all GC edges referring to the object, and filter out ones we know to be benign. This patch changes the test to use findReferences. --- js/src/shell/js.cpp | 23 +++++++++- js/src/shell/jsheaptools.cpp | 27 +----------- .../tests/js1_8/extensions/regress-422269.js | 42 +++++++++++-------- 3 files changed, 48 insertions(+), 44 deletions(-) diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp index 66145d20c35..1e526279f96 100644 --- a/js/src/shell/js.cpp +++ b/js/src/shell/js.cpp @@ -4121,7 +4121,28 @@ static const char *const shell_help_messages[] = { "notes([fun]) Show source notes for functions", "stats([string ...]) Dump 'arena', 'atom', 'global' stats", "findReferences(target)\n" -" Walk the heap and return an object describing all references to target", +" Walk the entire heap, looking for references to |target|, and return a\n" +" \"references object\" describing what we found.\n" +"\n" +" Each property of the references object describes one kind of reference. The\n" +" property's name is the label supplied to MarkObject, JS_CALL_TRACER, or what\n" +" have you, prefixed with \"edge: \" to avoid collisions with system properties\n" +" (like \"toString\" and \"__proto__\"). The property's value is an array of things\n" +" that refer to |thing| via that kind of reference. Ordinary references from\n" +" one object to another are named after the property name (with the \"edge: \"\n" +" prefix).\n" +"\n" +" Garbage collection roots appear as references from 'null'. We use the name\n" +" given to the root (with the \"edge: \" prefix) as the name of the reference.\n" +"\n" +" Note that the references object does record references from objects that are\n" +" only reachable via |thing| itself, not just the references reachable\n" +" themselves from roots that keep |thing| from being collected. (We could make\n" +" this distinction if it is useful.)\n" +"\n" +" If any references are found by the conservative scanner, the references\n" +" object will have a property named \"edge: machine stack\"; the referrers will\n" +" be 'null', because they are roots.", #endif "dumpStack() Dump the stack as an array of callees (youngest first)", #ifdef TEST_CVTARGS diff --git a/js/src/shell/jsheaptools.cpp b/js/src/shell/jsheaptools.cpp index c2714be4228..945aef1a1e5 100644 --- a/js/src/shell/jsheaptools.cpp +++ b/js/src/shell/jsheaptools.cpp @@ -551,32 +551,7 @@ ReferenceFinder::findReferences(JSObject *target) return result; } -/* - * findReferences(thing) - * - * Walk the entire heap, looking for references to |thing|, and return a - * "references object" describing what we found. - * - * Each property of the references object describes one kind of reference. The - * property's name is the label supplied to MarkObject, JS_CALL_TRACER, or what - * have you, prefixed with "edge: " to avoid collisions with system properties - * (like "toString" and "__proto__"). The property's value is an array of things - * that refer to |thing| via that kind of reference. Ordinary references from - * one object to another are named after the property name (with the "edge: " - * prefix). - * - * Garbage collection roots appear as references from 'null'. We use the name - * given to the root (with the "edge: " prefix) as the name of the reference. - * - * Note that the references object does record references from objects that are - * only reachable via |thing| itself, not just the references reachable - * themselves from roots that keep |thing| from being collected. (We could make - * this distinction if it is useful.) - * - * If any references are found by the conservative scanner, the references - * object will have a property named "edge: machine stack"; the referrers will - * be 'null', because they are roots. - */ +/* See help(findReferences). */ JSBool FindReferences(JSContext *cx, uintN argc, jsval *vp) { diff --git a/js/src/tests/js1_8/extensions/regress-422269.js b/js/src/tests/js1_8/extensions/regress-422269.js index c150f94773c..1dde8bbb849 100644 --- a/js/src/tests/js1_8/extensions/regress-422269.js +++ b/js/src/tests/js1_8/extensions/regress-422269.js @@ -38,8 +38,8 @@ //----------------------------------------------------------------------------- var BUGNUMBER = 422269; var summary = 'Compile-time let block should not capture runtime references'; -var actual = 'No leak'; -var expect = 'No leak'; +var actual = 'referenced only by stack and closure'; +var expect = 'referenced only by stack and closure'; //----------------------------------------------------------------------------- @@ -56,31 +56,39 @@ function test() function f() { let m = {sin: Math.sin}; - (function() { m.sin(1); })(); + (function holder() { m.sin(1); })(); return m; } - if (typeof countHeap == 'undefined') + if (typeof findReferences == 'undefined') { expect = actual = 'Test skipped'; - print('Test skipped. Requires countHeap function.'); + print('Test skipped. Requires findReferences function.'); } else { var x = f(); - f(); // overwrite the machine stack with new objects - gc(); - var n = countHeap(); - x = null; - // When running with the method JIT, null may not get stored to memory right away. - // Calling eval ensures that all values are stored out so that the old x is no - // longer rooted from the stack. - eval(""); - gc(); + var refs = findReferences(x); - var n2 = countHeap(); - if (n2 >= n) - actual = "leak is detected, something roots the result of f"; + // At this point, x should only be referenced from the stack --- the + // variable 'x' itself, and any random things the conservative scanner + // finds --- and possibly from the 'holder' closure, which could itself + // be held alive for random reasons. Remove those from the refs list, and + // then complain if anything is left. + for (var edge in refs) { + // Remove references from roots, like the stack. + if (refs[edge].every(function (r) r === null)) + delete refs[edge]; + // Remove references from the closure, which could be held alive for + // random reasons. + else if (refs[edge].length === 1 && + typeof refs[edge][0] === "function" && + refs[edge][0].name === "holder") + delete refs[edge]; + } + + if (Object.keys(refs).length != 0) + actual = "unexpected references to the result of f: " + Object.keys(refs).join(", "); } reportCompare(expect, actual, summary);