MakeOrSetMinidumpPath shouldn't bother doing IO and just set the env var, leaving the crash reporter to create the minidumps folder if necessary
Categories
(Toolkit :: Crash Reporting, defect, P1)
Tracking
()
Performance | P2 |
People
(Reporter: Gijs, Assigned: mcheang)
References
(Blocks 2 open bugs)
Details
(4 keywords, Whiteboard: [fxperfsize:S])
Attachments
(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.
Reporter | ||
Updated•3 years ago
|
Updated•3 years ago
|
Reporter | ||
Comment 1•3 years ago
|
||
This looks fairly straightforward...
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Pushed by mconley@mozilla.com: 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•3 years ago
|
||
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/logviewer.html#/jobs?job_id=255361257&repo=autoland&lineNumber=2627
Backout by shindli@mozilla.com: 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•3 years ago
|
||
There are also some mochitest failures:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=255369501&repo=autoland&lineNumber=7332
Comment 7•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
•
|
||
Thanks Gabriele, I will take a look at why those tests are failing.
Comment 9•3 years ago
|
||
Adding bug 1546460, since we'll no longer be creating the minidump folder for brand new user profiles right at startup anymore.
Assignee | ||
Comment 10•3 years ago
•
|
||
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•3 years ago
|
||
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 | ||
Updated•3 years ago
|
Updated•3 years ago
|
Reporter | ||
Comment 12•3 months ago
|
||
Mandy, looks like through a series of bugzilla account changes this is still assigned to you - are you likely to come back to it? :-)
Assignee | ||
Comment 13•3 months ago
|
||
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.
Assignee | ||
Comment 14•3 months ago
•
|
||
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. :)
Description
•