Closed Bug 759020 Opened 9 years ago Closed 9 years ago swallows some seg faults


(Core :: JavaScript Engine, defect)

Not set





(Reporter: n.nethercote, Assigned: terrence)



(2 files)

I landed the patch from bug 754181 and then had to back it out because it caused a seg fault on ecma_5/misc/future-reserved-words.js.  But I'd run the test suite!  So what went wrong?

Let's see... with that patch applied, if I run it in

  [bayou:~/moz/mi6/js/src/tests] python ../d64/js ecma_5/misc/future-reserved-words.js

But if I use's -s option to get the command and run that by itself:

  [bayou:~/moz/mi6/js/src/tests] /home/njn/moz/mi6/js/src/d64/js -f shell.js -f ecma_5/shell.js -f ecma_5/misc/shell.js -f ./ecma_5/misc/future-reserved-words.js
   <...lots of output snipped...>
   PASSED! Implement FutureReservedWords per-spec: implements: strict function expression argument, SyntaxError: implements is a reserved identifier 
   PASSED! Implement FutureReservedWords per-spec: implements: function expression argument retroactively strict, SyntaxError: implements is a reserved identifier 
  Segmentation fault (core dumped)

Hmm, that's not good.
What platform?  I was not able to reproduce this by inserting a trivial segfault.
Inserting a trivial segfault isn't enough;  only some seg faults are swallowed.

Steps to reproduce:

- Apply the attached patch.

- Run ecma_5/misc/future-reserved-words.js.

- The seg fault is swallowed.

I've reproduced this on Linux64 and Mac64 debug builds.  It's a totally reliable NULL deref crash, so I'm confident there's no non-determinism involved.
Attachment #628552 - Flags: feedback?(terrence)
I broke this by fixing a bug, which was previously masked by yet another bug :-P.

The bug I inadvertently fixed is to correctly post-process the child status code.  "Real" process exit codes are in [0..128), with information about how the process exited, whether it generated a core dump, and what signal killed it, etc stored in higher bits.

I opened Bug 754691 recently to try to fix the other problem, which is that we only consider a process as crashed if (simplifying a bit here) the test prints FAIL and the error code is not 0.

The right fix here is to use the real process crash status provided by the OS to decide whether a process has crashed.
Attached patch v0Splinter Review
On deeper inspection, it turns out I was wrong.  The subprocess modules does indeed return a baked error code, but a strange one where the signal is returned as a -signum.  With this, the iscrashed tests in make more sense (although an explicit sign check would still be clearer).

This patch updates the returncode we pass to TestOutput to have the same format in the unix runner as in the windows runner.
Assignee: n.nethercote → terrence
Attachment #628975 - Flags: review?(dmandelin)
Attachment #628552 - Flags: feedback?(terrence)
Comment on attachment 628975 [details] [diff] [review]

Review of attachment 628975 [details] [diff] [review]:

Yay adapting for random OS corners. :-)
Attachment #628975 - Flags: review?(dmandelin) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.