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) (:⚙️)]

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