Closed Bug 1206987 Opened 5 years ago Closed 5 years ago

SpiderMonkey on Windows unable to create crash dumps when they crash


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox44 --- fixed


(Reporter: gkw, Assigned: terrence)



(Keywords: regression, Whiteboard: [fuzzblocker])


(1 file, 2 obsolete files)

Attached patch patch v1 (obsolete) — 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.

Flags: needinfo?(terrence)
Attachment #8663963 - Attachment is patch: true
We will also have to backport this to the other branches probably as NPOTB.
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
Flags: needinfo?(terrence)
Attachment #8664486 - Flags: review?(sphink)
Attachment #8664486 - Flags: feedback?(gary)
Duplicate of this bug: 1206990
Comment on attachment 8664486 [details] [diff] [review]

This works great, thanks!
Attachment #8664486 - Flags: feedback?(gary) → feedback+
Comment on attachment 8664486 [details] [diff] [review]

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.
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)
Depends on: 1213104
Depends on: 1213111
Depends on: 1213127
Depends on: 1213129
Depends on: 1213133
Depends on: 1213365
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:
Attachment #8664486 - Attachment is obsolete: true
Flags: needinfo?(terrence)
Attachment #8672815 - Flags: review?(sphink)
Comment on attachment 8672815 [details] [diff] [review]

Tested that this patch also causes crash dumps to appear as intended.
Attachment #8672815 - Flags: feedback+
Attachment #8672815 - Flags: review?(sphink) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1263857
You need to log in before you can comment on or make changes to this bug.