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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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.

Comment 1

[Jesse Ruderman]

Attachment: some gdb output

f->root is 0x55555555

Comment 2

[Robert Sayre]

I can't reproduce this so far

Comment 3

[Robert Sayre]

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$

Comment 4

[Jesse Ruderman]

Weird.  I can reproduce with clobbered shells on two machines.  One of them even has a completely clean TM source tree.

Comment 5

[Brendan Eich [:brendan]]

Which Mac OS X version(s) are y'all on?

/be

Comment 6

[Robert Sayre]

I'm on 10.5.7

Comment 7

[Jesse Ruderman]

same

Comment 8

[Jesse Ruderman]

same

Comment 9

[David Mandelin [:dmandelin]]

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.

Comment 10

[David Mandelin [:dmandelin]]

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.

Comment 11

[Graydon Hoare :graydon]

Guh. I introduced this recently in bug 495362, in an attempt to fix a related error in that loop.

Solution (1): two loops, please.

Comment 12

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 13

[Jesse Ruderman]

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.

Comment 14

[David Mandelin [:dmandelin]]

Attachment: Patch v2

mrbkap likes this version better because it's got a minimum of extra source gunk to hurt programmers' eyes.

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Patch v3

Fixed an out of date comment in the new iterator func.

Comment 16

[Graydon Hoare :graydon]

Comment on attachment 381666 [details] [diff] [review]
Patch v2

Looks good. Sorry for the mess.

Comment 17

[David Mandelin [:dmandelin]]

Pushed to TM as e7c61fb767c7.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?

Comment 19

[David Mandelin [:dmandelin]]

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.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

(marking fixed-in-tracemonkey as per comment 17)

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e7c61fb767c7

Comment 22

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b94f6e65e1e8

Comment 23

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.