Created attachment 8663963 [details] [diff] [review] patch v1 In Q1 of this year, I implemented fuzzing harness support for using crash dumps on Windows . 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  because jit-tests were timing out when crashing, causing intermittent failures (bug 1170665 and its patch landing ). 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.  https://msdn.microsoft.com/en-us/library/windows/desktop/bb787181%28v=vs.85%29.aspx  http://hg.mozilla.org/mozilla-central/annotate/197af2fb7e29/js/src/shell/js.cpp#l6172  http://hg.mozilla.org/mozilla-central/rev/617e217524a2
Attachment #8663963 - Attachment is patch: true
We will also have to backport this to the other branches probably as NPOTB.
Created attachment 8664486 [details] [diff] [review] fix_crash_dialog_suppression-v0.diff 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.
Comment on attachment 8664486 [details] [diff] [review] fix_crash_dialog_suppression-v0.diff This works great, thanks!
Attachment #8664486 - Flags: feedback?(gary) → feedback+
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+
Slurped it to my windows machine and fixed the build there.
Looks like you also managed to bust some spidermonkey jobs: https://treeherder.mozilla.org/logviewer.html#?job_id=14602539&repo=mozilla-inbound
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
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.
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
Created attachment 8672815 [details] [diff] [review] only_disable_gpf_dialog_during_testing-v0.diff 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
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+
Attachment #8672815 - Flags: review?(sphink) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.