SpiderMonkey on Windows unable to create crash dumps when they crash

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gkw, Assigned: terrence)

Tracking

({regression})

Trunk
mozilla44
All
Windows
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8663963 [details] [diff] [review]
patch v1

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

2 years ago
Keywords: regression
(Reporter)

Updated

2 years ago
Attachment #8663963 - Attachment is patch: true
(Reporter)

Updated

2 years ago
See Also: → bug 1206990
(Reporter)

Comment 1

2 years ago
We will also have to backport this to the other branches probably as NPOTB.
(Assignee)

Comment 2

2 years ago
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.
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)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1206990
(Reporter)

Comment 4

2 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 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

2 years ago
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
Flags: needinfo?(terrence)
(Assignee)

Comment 13

2 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)

Updated

2 years ago
Depends on: 1213104
(Assignee)

Updated

2 years ago
Depends on: 1213111
(Assignee)

Updated

2 years ago
Depends on: 1213127
(Assignee)

Updated

2 years ago
Depends on: 1213129
(Assignee)

Updated

2 years ago
Depends on: 1213133
(Assignee)

Updated

2 years ago
Depends on: 1213365
(Assignee)

Comment 16

2 years ago
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
Attachment #8664486 - Attachment is obsolete: true
Flags: needinfo?(terrence)
Attachment #8672815 - Flags: review?(sphink)
(Reporter)

Comment 17

2 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+
Attachment #8672815 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/a419ffb1b62f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1263857
You need to log in before you can comment on or make changes to this bug.