Closed Bug 1251196 Opened 7 years ago Closed 7 years ago

[mozcrash] Do not force the existence of a symbols path but use tmp if not specified

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox46 fixed, firefox47 fixed, firefox-esr45 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox-esr45 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

Details

Attachments

(2 files)

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
ted
: review+
Details
MozReview Request: Bug 1251196 - Bump mozcrash to 0.17. r?ted
58 bytes, text/x-review-board-request
ted
: review+
Details 
(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.
Status: NEW → ASSIGNED
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.
Keywords: leave-open
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?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox46: --- → affected
status-firefox-esr45: --- → affected
Flags: needinfo?(ted)
Resolution: --- → FIXED
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?
Flags: needinfo?(ted)
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?
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
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.
status-firefox47: --- → fixed
Whiteboard: [checkin-needed-beta]
Target Milestone: --- → mozilla47
Whiteboard: [checkin-needed-beta]
Similar to the backport to mozilla-beta, please land this test-only change on mozilla-esr45.
Whiteboard: [checkin-needed-esr45]
You need to log in before you can comment on or make changes to this bug.