Closed Bug 1251196 Opened 5 years ago Closed 5 years ago
[mozcrash] Do not force the existence of a symbols path but use tmp if not specified
MozReview Request: Bug 1251196 - [mozcrash] If no symbols path is specified let stackwalk download the symbols. r?ted
58 bytes, text/x-review-board-request
58 bytes, text/x-review-board-request
(In reply to Henrik Skupin (:whimboo) from bug 1243684 comment #24) > I don't understand why that has to be done given that if a symbols_url is > present and symbols will be downloaded ondemand you don't have to specify > such a local folder. Why can't mozcrash automatically pick the temp folder > in case no symbols_url is present? It could! The code in mozcrash predates the existence of these minidump_stackwalk binaries that have the symbol server URL baked in, so this wasn't a use case we envisioned. We've already got code that creates a temp dir to extract symbols when you pass in a URL to a symbols.zip, so we could just let you pass in None and create an empty directory to use: https://dxr.mozilla.org/mozilla-central/rev/5b2baa5e9356644a7ed0b73e422eaff62e159ffb/testing/mozbase/mozcrash/mozcrash/mozcrash.py#176
Given that this blocks crash analysis for l10n builds of Firefox UI Tests I'm going to take this bug.
Assignee: nobody → hskupin
Here an example of the current behavior of mozcrash with a de locale build. No symbols_url nor symbols_path has been specified: https://treeherder.mozilla.org/logviewer.html#?job_id=2020778&repo=mozilla-aurora#L879 06:24:15 INFO - PROCESS-CRASH | runner.py | application crashed [None] 06:24:15 INFO - Crash dump filename: c:\jenkins\workspace\mozilla-aurora_functional\build\tmpuutlym.mozrunner-1456496626\minidumps\99370cd3-32fd-4f20-9a05-56c79636b98e.dmp 06:24:15 INFO - No symbols path given, can't process dump.
Review commit: https://reviewboard.mozilla.org/r/37941/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37941/
Attachment #8726383 - Flags: review?(ted)
Review commit: https://reviewboard.mozilla.org/r/37943/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37943/
Attachment #8726384 - Flags: review?(ted)
Comment on attachment 8726384 [details] MozReview Request: Bug 1251196 - Bump mozcrash to 0.17. r?ted https://reviewboard.mozilla.org/r/37943/#review34553
Attachment #8726384 - Flags: review?(ted) → review+
Comment on attachment 8726383 [details] MozReview Request: Bug 1251196 - [mozcrash] If no symbols path is specified let stackwalk download the symbols. r?ted https://reviewboard.mozilla.org/r/37941/#review34555 ::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:169 (Diff revision 1) > + self.symbols_path = tempfile.mkdtemp() You need to set `self.remove_symbols = True` like the url case below.
Attachment #8726383 - Flags: review?(ted) → review+
Comment on attachment 8726383 [details] MozReview Request: Bug 1251196 - [mozcrash] If no symbols path is specified let stackwalk download the symbols. r?ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37941/diff/1-2/
Comment on attachment 8726384 [details] MozReview Request: Bug 1251196 - Bump mozcrash to 0.17. r?ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37943/diff/1-2/
https://reviewboard.mozilla.org/r/37941/#review34555 > You need to set `self.remove_symbols = True` like the url case below. Oh, right. I will add this line below line 169 so we do not fail in case `mkdtemp()` fails.
Lets keep the bug open after landing on mozilla-central so that I can release the version to pypi.
Version 0.17 has been uploaded to pypi. Ted, given that our firefox-ui-tests make use of the in-tree version of mozcrash I would like to get this patch backported to at least 46.0 and even better 45.0esr. I don't think that we can land anything on the 45.0 release branch. What do you think?
It's a pretty inoffensive patch, should be fine to branch-land. Are you intending to backport the firefox-ui-tests in-tree to those branches, or what?
Our tests are already existent in-tree down to the current release branch (45.o) and mozilla-esr45. Due to crashes we have a lot of test jobs for which mozcrash doesn't work because of locale builds. So we could 1) backport this patch (i assume without the version bump) or 2) we set a fake folder for symbols-url. Both would work for me. Maybe the latter is better?
Either way is fine with me. This patch is pretty inoffensive, and presumably you're satisfied with how it works, so backporting it is probably less work than writing a new patch just for the branches.
So this patch works great with localized Aurora builds without specifying any symbols path: https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&filter-searchStr=Firefox%20UI&filter-tier=2&filter-tier=3&selectedJob=2097106 > PROCESS-CRASH | runner.py | application crashed [@ RtlAllocateMemoryZone + 0x2a6281] Sheriffs please backport only attachment 8726383 [details] to mozilla-beta, which is a test/tools only change. Thanks.
Similar to the backport to mozilla-beta, please land this test-only change on mozilla-esr45.
You need to log in before you can comment on or make changes to this bug.