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

Status: ASSIGNED

(Toolkit :: Crash Reporting, defect, P1)

Description

[:Gijs (he/him)]

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.

Comment 1

[:Gijs (he/him)]

This looks fairly straightforward...

Comment 2

[Mandy Cheang]

Attachment: Bug 1541603 - remove creation of minidumps folder on startup, create it lazily in crash reporter. r?mconley,gsvelto

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b59c9bfa328d
remove creation of minidumps folder on startup, create it lazily in crash reporter. r=gsvelto

Comment 4

[Stefan Hindli [:stefan_hindli]]

Backed out changeset b59c9bfa328d (Bug 1541603) for causing xpcshell failure in toolkit/crashreporter/test/unit_ipc/test_content_annotation.js CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=e3ffa96d1b94da9f88a94c88710fc4eb6bc26b2c&selectedJob=255361257

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=255361257&repo=autoland&lineNumber=2627

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16ee100ad0c9
Backed out changeset b59c9bfa328d for causing xpcshell failure in toolkit/crashreporter/test/unit_ipc/test_content_annotation.js CLOSED TREE

Comment 6

[Cristian Brindusan [:cbrindusan]]

There are also some mochitest failures:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=255369501&repo=autoland&lineNumber=7332

Comment 7

[Gabriele Svelto [:gsvelto]]

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.

Comment 8

[Mandy Cheang]

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

Comment 9

[Mike Conley (:mconley) (:⚙️) (Catching up on needinfos)]

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

Comment 10

[Mandy Cheang]

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: https://phabricator.services.mozilla.com/D36370
My most recent push to TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=def246ee2a53c5adbd9dd3a2563a728179517150

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.

Comment 11

[Gabriele Svelto [:gsvelto]]

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.