Closed
Bug 1206987
Opened 9 years ago
Closed 9 years ago
SpiderMonkey on Windows unable to create crash dumps when they crash
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: gkw, Assigned: terrence)
References
Details
(Keywords: regression, Whiteboard: [fuzzblocker])
Attachments
(1 file, 2 obsolete files)
2.86 KB,
patch
|
sfink
:
review+
gkw
:
feedback+
|
Details | Diff | Splinter Review |
In Q1 of this year, I implemented fuzzing harness support for using crash dumps on Windows [1]. This worked fine till May/June, when I noticed that we were no longer getting the dumps. I dismissed this as something transitional until I started investigating more recently. I realised that on one of the fuzzing computers on my desk, it was still generating crash dumps for non-SpiderMonkey binaries even till July, so I began suspecting something that landed in SM. I went into hgweb and found that we were suppressing error dialogs from the SM side [2] because jit-tests were timing out when crashing, causing intermittent failures (bug 1170665 and its patch landing [3]). Then, I did a test where I #ifndef'ed that code, and sure enough, the crash dumps came back when I ran through it through a known crash (e.g. bug 1205870). Thus, the regressor is bug 1170665. I have attached a patch which disables that code when we are compiling more-deterministic builds; however, Terrence suggested on IRC that we should #ifndef this to use --fuzzing-safe terminology instead, which might be a better solution. Setting [fuzzblocker] because this prevents us from getting crash dumps on Windows. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/bb787181%28v=vs.85%29.aspx [2] http://hg.mozilla.org/mozilla-central/annotate/197af2fb7e29/js/src/shell/js.cpp#l6172 [3] http://hg.mozilla.org/mozilla-central/rev/617e217524a2
Flags: needinfo?(terrence)
Reporter | ||
Updated•9 years ago
|
Keywords: regression
Reporter | ||
Updated•9 years ago
|
Attachment #8663963 -
Attachment is patch: true
Reporter | ||
Comment 1•9 years ago
|
||
We will also have to backport this to the other branches probably as NPOTB.
Assignee | ||
Comment 2•9 years ago
|
||
Literally 5 lines below my SetErrorMode call, there is a second one that I have no idea *at all* how I missed when adding the new one. The existing SetErrorMode is guarded by the environment variable XRE_NO_WINDOWS_CRASH_DIALOG. This variable appears to be set by most of the test suites in our tree except for jstests, jit-tests, and jsapi-tests. The right solution here is to merge our duplicate SetErrorMode calls with the environment check and add that environment variable to our test suites.
Assignee: nobody → terrence
Attachment #8663963 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(terrence)
Attachment #8664486 -
Flags: review?(sphink)
Attachment #8664486 -
Flags: feedback?(gary)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8664486 [details] [diff] [review] fix_crash_dialog_suppression-v0.diff This works great, thanks!
Attachment #8664486 -
Flags: feedback?(gary) → feedback+
Comment 5•9 years ago
|
||
Comment on attachment 8664486 [details] [diff] [review] fix_crash_dialog_suppression-v0.diff Review of attachment 8664486 [details] [diff] [review]: ----------------------------------------------------------------- Sure, seems ok to me.
Attachment #8664486 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Slurped it to my windows machine and fixed the build there.
Comment 10•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6dddf8043e8
Looks like you also managed to bust some spidermonkey jobs: https://treeherder.mozilla.org/logviewer.html#?job_id=14602539&repo=mozilla-inbound
Flags: needinfo?(terrence)
Reporter | ||
Comment 12•9 years ago
|
||
Seems like we're missing an "import os" line in https://hg.mozilla.org/integration/mozilla-inbound/annotate/748829f0a151/js/src/tests/lib/tasks_win.py
Assignee | ||
Comment 13•9 years ago
|
||
That, and the environment entries need to 'str' and not 'unicode' typed. Appears to work now. Will check in in after I test some more locally.
Flags: needinfo?(terrence)
Windows XP jit tests have been timing out like https://treeherder.mozilla.org/logviewer.html#?job_id=14720960&repo=mozilla-inbound since this landed. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/27e9d9dfc369
Flags: needinfo?(terrence)
Assignee | ||
Comment 16•9 years ago
|
||
Now we can be fairly sure that we're getting the right environment in the test suite, so let's use it to do a minimal fix. Try seems to be green on at least one run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b3f009541dd
Attachment #8664486 -
Attachment is obsolete: true
Flags: needinfo?(terrence)
Attachment #8672815 -
Flags: review?(sphink)
Reporter | ||
Comment 17•9 years ago
|
||
Comment on attachment 8672815 [details] [diff] [review] only_disable_gpf_dialog_during_testing-v0.diff Tested that this patch also causes crash dumps to appear as intended.
Attachment #8672815 -
Flags: feedback+
Updated•9 years ago
|
Attachment #8672815 -
Flags: review?(sphink) → review+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a419ffb1b62f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•