Open Bug 1541603 Opened 4 years ago Updated 2 months ago

MakeOrSetMinidumpPath shouldn't bother doing IO and just set the env var, leaving the crash reporter to create the minidumps folder if necessary


(Toolkit :: Crash Reporting, defect, P1)




Performance Impact medium


(Reporter: Gijs, Assigned: mcheang)


(Blocks 2 open bugs)


(4 keywords, Whiteboard: [fxperfsize:S])


(1 file)

As in summary. The code adds insult to injury by doing an exists() check followed by a create() - but I think the correct solution is to just do this work in the crashreporter, and just update the env var when we have a profile without doing any IO.

Keywords: main-thread-io, perf
Whiteboard: [fxperf]
Whiteboard: [fxperf] → [fxperf:p2]

This looks fairly straightforward...

Whiteboard: [fxperf:p2] → [fxperf:p2] [fxperfsize:S]
Assignee: nobody → mcheang
Pushed by
remove creation of minidumps folder on startup, create it lazily in crash reporter. r=gsvelto
Flags: needinfo?(mcheang)
Backout by
Backed out changeset b59c9bfa328d for causing xpcshell failure in toolkit/crashreporter/test/unit_ipc/test_content_annotation.js CLOSED TREE

Mandy, it looks like the minidump & extra files are not being moved correctly in the test environment. You'll have to run the tests locally and see what's going on. The only difference between a normal run and a test run is that ShouldReport() will return false and the target minidump directory will be different. I find it odd that your change doesn't work under those conditions but we might have missed some corner-case that comes up in tests.

Priority: -- → P1

Thanks Gabriele, I will take a look at why those tests are failing.

Flags: needinfo?(mcheang)

Adding bug 1546460, since we'll no longer be creating the minidump folder for brand new user profiles right at startup anymore.

Blocks: 1546460

My patch moved the creation of the minidumps directory out of start-up. Instead we can create it lazily (in the content process). This change caused many test failures and I have attempted to fix these failures over the last several weeks. The test infrastructure heavily relies on the minidumps directory to be available at start-up.

While fixing this problem, I ran into several issues. I fixed a subset of tests to correctly find the minidump file, while there are other tests that will fail due to this fix. Some tests want to find the dump file in the "profile/minidumps" folder, while other tests want to find the dump file in "temp" directory.

All the failing tests have a similar theme. The problem is that it cannot find the dump file in the directory it wants to find it. The directory where the dump file is created is inconsistent across different test suites.

I've fixed most of the test failures by adding a few things:

  • I've added creating it lazily in the parent and plugin process to fix test failures.
  • I've added an externallySetMinidumpPath boolean to keep track of whether to or not the crashReporter.minidumpPath has been set externally by a test. If it has, then let's use the externally set path for the test.

I've pushed my most recent patch on phabricator:
My most recent push to TRY:

This is the version with the least test failures (from few hundred down to less than 10 test failures):

There are 4 remaining test failures I'm aware of:

  • Windows 2012 build is failing because the test was not able to log "PROCESS CRASH".
  • There is another set of XPCshell X2 test failures:
    • toolkit/crashreporter/test/unit/test_crash_with_memory_report.js and
    • toolkit/crashreporter/test/unit/test_crash_with_memory_report.js
  • An android Xpcshell test failure: toolkit/components/extensions/test/xpcshell/test_ext_webRequest_cached.js

These are my findings as of August 28, 2019. My internship ends on August 30, 2019. I plan to continue working on this after I receive volunteer contributor access. I'll be around in IRC and slack.

Mandy, you've done some excellent work on a very complex issue. I'm always available to help out if you want to finish this. As for your last try run I suspect that the failures in toolkit/crashreporter/test/unit/test_crash_with_memory_report.js are due to the memory report file which should "ride along" with the minidump and it might have been created in the wrong directory instead. The Windows issue on the other hand is quite odd and I can't think of an obvious cause.

Assignee: mcheang → mandycheang135
Assignee: mandycheang135+personal → mandycheang135

Mandy, looks like through a series of bugzilla account changes this is still assigned to you - are you likely to come back to it? :-)

Performance Impact: --- → P2
Flags: needinfo?(mcheang)
Whiteboard: [fxperf:p2] [fxperfsize:S] → [fxperfsize:S]

Hi Gijs, thanks for following up on this one.

Yes - I'd love to get back to this bug when I have a chance in the next upcoming weeks. I've saved all my notes from my intern days on it and I roughly remember the solution I was implementing. I got stuck because it failed a bunch of tests and I couldn't figure out why. Next steps are to track back my steps of my original solution and investigate the test failures.

I'll aim to get it in for Fx 101 if not, then Fx 102.

Flags: needinfo?(mcheang)

Oh and - I apologize for not getting back to this earlier. School and part-time work got really busy for me in the last two years! But now I'm back at Mozilla, I'll love to take a look at this bug again. :)

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.