Closed Bug 496391 Opened 13 years ago Closed 13 years ago

TM: "Assertion failure: f == f->root" with MallocScribble and simple testcase

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: dmandelin)

Details

(Keywords: assertion, fixed1.9.1, testcase, Whiteboard: [sg:critical?] fixed-in-tracemonkey)

Attachments

(2 files, 2 obsolete files)

export MallocScribble=1
js -j a.js
Assertion failure: f == f->root, at ../jstracer.cpp:3500

a.js:

var z = 0;
for (var a = 0; a < 3; ++a) {
    for (var b = 0; b < 3; ++b) {
    }
}
"" + z;

When I run it under Valgrind, I don't get any Valgrind errors and the assertion does not fire.  But I can reproduce under gdb.

Happens on 1.9.1 branch too.
Flags: blocking1.9.1?
Attached file some gdb output
f->root is 0x55555555
Flags: blocking1.9.1? → blocking1.9.1+
I can't reproduce this so far
still nothing.

robert-sayres-macbook-pro:build-release sayrer$ cat a.js 
var z = 0;
for (var a = 0; a < 3; ++a) {
    for (var b = 0; b < 3; ++b) {
    }
}
"" + z;
robert-sayres-macbook-pro:build-release sayrer$ export MallocScribble=1
robert-sayres-macbook-pro:build-release sayrer$ ./js -j -f a.js 
js(34800) malloc: enabling scribbling to detect mods to free blocks
robert-sayres-macbook-pro:build-release sayrer$ valgrind --auto-run-dsymutil=yes ./js -j -f a.js
valgrind(34801) malloc: enabling scribbling to detect mods to free blocks
==34801== Memcheck, a memory error detector.
==34801== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==34801== Using LibVEX rev 1897, a library for dynamic binary translation.
==34801== Copyright (C) 2004-2009, and GNU GPL'd, by OpenWorks LLP.
==34801== Using valgrind-3.5.0.SVN, a dynamic binary instrumentation framework.
==34801== Copyright (C) 2000-2009, and GNU GPL'd, by Julian Seward et al.
==34801== For more details, rerun with: -v
==34801== 
==34801== 
==34801== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==34801== malloc/free: in use at exit: 184,366 bytes in 1,391 blocks.
==34801== malloc/free: 4,119 allocs, 2,728 frees, 513,967 bytes allocated.
==34801== For counts of detected errors, rerun with: -v
==34801== searching for pointers to 1,391 not-freed blocks.
==34801== checked 10,125,668 bytes.
==34801== 
==34801== LEAK SUMMARY:
==34801==    definitely lost: 0 bytes in 0 blocks.
==34801==    indirectly lost: 0 bytes in 0 blocks.
==34801==      possibly lost: 44 bytes in 1 blocks.
==34801==    still reachable: 184,322 bytes in 1,390 blocks.
==34801==         suppressed: 0 bytes in 0 blocks.
==34801== Rerun with --leak-check=full to see details of leaked memory.
robert-sayres-macbook-pro:build-release sayrer$
Weird.  I can reproduce with clobbered shells on two machines.  One of them even has a completely clean TM source tree.
Which Mac OS X version(s) are y'all on?

/be
I'm on 10.5.7
same
same
I get the same results as sayrer. If you have a machine here where you can reproduce reliably, I wouldn't mind doing some pair debugging.
We just debugged it on Jesse's machine. The flaw is in the interaction of js_PurgeScriptFragments and js_TrashTree and occurs on shutdown (via JS_DestroyScript, so it wouldn't have to be shutdown in the browser).

js_PurgeScriptFragments processes the trees in arbitrary order. For each, first it calls js_TrashTree, and then it calls clearFragment on the fragment, which deletes it (and in Jesse's setup, scribbles over it). So far, this looks good, because js_TrashTree reads the fragment, so it should be called before clearFragment.

But js_TrashTree recursively calls js_TrashTree on dependent trees of the argument. Thus, if we have two trees X and Y, where X depends on Y (X is in the depend list of Y), we can have this happen:

  purge visits X first
    trash X
    delete fragments of X
  purge visits Y second
    trash Y
      recursively trash X
      assert trying to read freed fragment of X

Two obvious solutions are (1) run two loops in js_PurgeScriptFragments, the first trashing, the second freeing, or (2) visit trees in topological order in js_PurgeScriptFragments.
Guh. I introduced this recently in bug 495362, in an attempt to fix a related error in that loop.

Solution (1): two loops, please.
Assignee: general → dmandelin
Attached patch Patch (obsolete) — Splinter Review
I tested this patch and it fixes the bug for me.  Fuzzing with MallocScribble now, mostly to see if it catches other bugs with this one fixed.
Attached patch Patch v2 (obsolete) — Splinter Review
mrbkap likes this version better because it's got a minimum of extra source gunk to hurt programmers' eyes.
Attachment #381661 - Attachment is obsolete: true
Attachment #381666 - Flags: review?
Attached patch Patch v3Splinter Review
Fixed an out of date comment in the new iterator func.
Attachment #381666 - Attachment is obsolete: true
Attachment #381667 - Flags: review?
Attachment #381666 - Flags: review?
Comment on attachment 381666 [details] [diff] [review]
Patch v2

Looks good. Sorry for the mess.
Attachment #381667 - Flags: review? → review+
Pushed to TM as e7c61fb767c7.
Do the identical signatures mean the template will only be instantiated once?  Or should we just have a typedef for that signature and pass function pointers of that type?
I would think it is only instantiated once, because there is only one type. But on my machine, GCC inlined the iterator function into js_PurgeScopeFragments both times, so it seems not to matter. Of course, it's possible we don't want that inlining, but that would appear to be a separate issue and not under our direct control.
(marking fixed-in-tracemonkey as per comment 17)
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/e7c61fb767c7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Group: core-security
Flags: in-testsuite?
Whiteboard: fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey
Flags: wanted1.9.0.x-
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.