If the a jit-test's magic comment says, e.g., error:SyntaxError, the harness will accept a program that has a syntax error and then crashes. Presumably, the harness just needs to check the error code in addition to the output. An example is the jit-test added by bug 775807.

This seems serious; unless I'm missing something, we could be ignoring some crashes in the jit-test harnesses *right now*!

I didn't get any anomaly with any test after applying this patch, maybe at this time no existing tests throw a SyntaxError *and* crash at the same time? Tested with: python -u js/src/jit-test/jit_test.py --no-slow <a js binary from debug 64-bit and rev c21ef3664c67>

I wanted to test if this change would work for /all/ errors, not just SyntaxError, so I duped your patch. I figured that since I updated your patch with the changes I want, it would probably be clearer to just provide my review in patch form.

Comment on attachment 752665 [details] [diff] [review] patch v0 Superceded.

Comment on attachment 753016 [details] [diff] [review] v2 Spoke to Terrence in-person, some more comments might be better, but the idea behind this sounds good to me. r=me

Thanks!

Awesome! Looks like it stuck with just the one test change. Crisis averted.

Multiprocessing appears to hand back incorrect return codes at a small, but too-large-for-tbpl rate. We should try turning this back on once we move to using os.exec/os.waitpid directly.

Terrence, is there a bug for that? I just noticed that the way we use subprocess.Popen.communicate(), it kicks off a thread for each of {stdout, stderr}. Combined with but 725500 comment 0 that makes me twitch a little bit.

(In reply to Jason Orendorff [:jorendorff] from comment #11) > Terrence, is there a bug for that? Probably bug 745237, but it depends how aggressive we want to be with this. > I just noticed that the way we use subprocess.Popen.communicate(), it kicks > off a thread for each of {stdout, stderr}. Combined with but 725500 comment > 0 that makes me twitch a little bit. Me too. Our saving grace here is that subprocess does a bunch of syscall heavy work before execing anything: the parent thread is extremely likely to have gone quiescent before the exec actually happens. I don't think I was ever able to get the freeze to occur with -j1. Also, the extra layer of indirection through multiprocessing means that at least the test runner itself should not freeze. I'm sorry about how busted this all is. I knew something like this was probably going to happen at some point when I allowed multiprocessing in. We didn't have much alternative at the time; maybe now is finally the right moment to take a few days and just make bug 745237 happen.

I'm not actively working on this. I don't know if it's still a problem. Nor what the cause could be. We're finally off multiprocessing though, so I'm cautiously optimistic that it's already fixed?