If symbols URL does not exist, don't add it to the run_command call

RESOLVED FIXED

Status

Release Engineering
Mozharness
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

unspecified

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed, firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
While running Fx UI Tests for localized builds of Firefox, the automatic symbol URL detection of mozharness fails because the folder name is different to the en-US build, and localized builds don't come with any symbols. Which means the en-US should be used. 

We should not do the latter in mozharness, but simply ignoring non-existent URLs is pretty fine. By that we can trigger mozcrash to auto-download the correct symbols in case there are some available for the type of build. It's better than the current behavior, because mozcrash will not get such a logic and simply fails over and over again to fetch the symbols. See the following job:

https://treeherder.mozilla.org/logviewer.html#?job_id=749410&repo=mozilla-beta
(Assignee)

Comment 1

a year ago
Created attachment 8713114 [details] [diff] [review]
Don't use 404 symbols_url

First step before doing any work for bug 1225124. This patch ensures that the symbols URL exists and only use it in those cases. Otherwise mozcrash auto-detects the symbols for testing and use them if available.

This patch would allow us to automatically pickup new auto-detect features from mozcrash without having to modify mozharness again.

Ted, is all that correct?
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(ted)
Attachment #8713114 - Flags: review?(jlund)
I'm not really wild about having this be the default behavior, because it could easily result in other test runs not having symbols. Most of the tests we run in automation run against inbound builds where we do need to download the symbols.zip, so having those fail would be bad. Can we make this opt-in, and have some configuration setting like `self.symbol_failure = True` that the Fx UI tests could set?
Flags: needinfo?(ted)
(Assignee)

Comment 3

a year ago
Ted, don't we always specify --symbols-url when running those tests e.g. for inbound? I thought that is by default. Maybe we could restrict this check when we try to get the symbols from the installer url? In that case it is not explicitly set and could always fail.
Flags: needinfo?(ted)
I don't actually know, but if that's the case then your suggestion sounds fine. Falling back silently if we're trying to guess the symbols URL seems ok. Falling back silently if we were given a symbols URL and failed to fetch is is not.
Flags: needinfo?(ted)
(Assignee)

Updated

a year ago
Attachment #8713114 - Flags: review?(jlund)
(Assignee)

Comment 5

a year ago
I will have a look at various test logs to check for which tests we actually pass in the symbols URL.
(Assignee)

Comment 6

a year ago
Desktop unit tests (via config we set: symbols-path):

['/tools/buildbot/bin/python', 'scripts/scripts/desktop_unittest.py', '--cfg', 'unittests/*', '--*-suite', '*', '--blob-upload-branch', 'mozilla-central', '--download-symbols', 'ondemand']

Marionette Unit tests (via config we set download_symbols=ondemand, and symbols-path):

['/tools/buildbot/bin/python', 'scripts/scripts/marionette.py', '--cfg', 'marionette/prod_config.py', '--blob-upload-branch', 'mozilla-central']

Web platform tests:

['/tools/buildbot/bin/python', 'scripts/scripts/web_platform_tests.py', '--cfg', 'web_platform_tests/prod_config.py', '--test-type=reftest', '--blob-upload-branch', 'mozilla-central', '--download-symbols', 'ondemand']

All the tests run via buildbot get the symbols url from that line as part of the buildbot properties:

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/testbase.py#287

> elif f['name'].endswith('crashreporter-symbols.zip'):  # yuk
>     self.symbols_url = str(f['name'])

That means query_symbols_url() will directly return this value and due to the ondemand setting we assign the URL to symbols_path for use in mozcrash.

So as proposed in comment 3 and agreed on by Ted in comment 4, I will move the extra check for existence to the guessing code path only.
(Assignee)

Comment 7

a year ago
Created attachment 8713329 [details] [diff] [review]
Don't use auto-detected symbols_url if 404 v1.1

This patch should practically do what I mentioned in the last comments.
Attachment #8713114 - Attachment is obsolete: true
Attachment #8713329 - Flags: review?(jlund)
Attachment #8713329 - Flags: feedback?(ted)
Comment on attachment 8713329 [details] [diff] [review]
Don't use auto-detected symbols_url if 404 v1.1

Review of attachment 8713329 [details] [diff] [review]:
-----------------------------------------------------------------

again, chmanchester or ahal would be better for this test specific stuff as they have a much better grasp of all the branches and tests that use this code today.

If they can't help, I'll dive into this deeper once Ted is in agreement with your strategy
Attachment #8713329 - Flags: review?(jlund)
(Assignee)

Comment 9

a year ago
Comment on attachment 8713329 [details] [diff] [review]
Don't use auto-detected symbols_url if 404 v1.1

So flipping over the review request to Andrew.
Attachment #8713329 - Flags: review?(ahalberstadt)
Comment on attachment 8713329 [details] [diff] [review]
Don't use auto-detected symbols_url if 404 v1.1

Review of attachment 8713329 [details] [diff] [review]:
-----------------------------------------------------------------

That looks sensible, thanks!
Attachment #8713329 - Flags: feedback?(ted) → feedback+
Comment on attachment 8713329 [details] [diff] [review]
Don't use auto-detected symbols_url if 404 v1.1

Review of attachment 8713329 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +187,2 @@
>          else:
> +            self.fatal("Can't figure out symbols_url without an installer_url!")

As long as symbols_url is allowed to be None, should this also be a warning? Or is this the only place we ensure installer_url is set?
Attachment #8713329 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 12

a year ago
Comment on attachment 8713329 [details] [diff] [review]
Don't use auto-detected symbols_url if 404 v1.1

Review of attachment 8713329 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +187,2 @@
>          else:
> +            self.fatal("Can't figure out symbols_url without an installer_url!")

The behavior has not been changed in my patch, and I don't want to do it here. There is bug 1225124 which I can look at next, where we can make it not a fatal failure.
(Assignee)

Updated

a year ago
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
Keywords: checkin-needed
jordan, do you want to handle this checkin or shall we sheriff check this in during the normal checkin-needed stuff ?
Flags: needinfo?(jlund)
(Assignee)

Comment 14

a year ago
(In reply to Carsten Book [:Tomcat] from comment #13)
> jordan, do you want to handle this checkin or shall we sheriff check this in
> during the normal checkin-needed stuff ?

This is a normal check-in to inbound. Not sure why this question is coming up. I will land it myself then.
Flags: needinfo?(jlund)

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/61fa62d120b6
Keywords: checkin-needed

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61fa62d120b6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 17

a year ago
Ted, given that we run our fx ui tests against different locales for Aurora, Beta, and Release, it would be good for me to have this backported for 46 and 45 (especially because this becomes the next ESR). It's not that easy for us to retrieve a downloadable URL for symbols given that Pulse message do not contain a build nor a symbol URL. We should definitely wait some days to see how this change works and that nothing regresses. But want to have some feedback early on. Thanks!
(Assignee)

Updated

a year ago
Flags: needinfo?(ted)
Okay. This approach seems fine, I don't think backporting it should be a problem.
Flags: needinfo?(ted)
(Assignee)

Comment 19

a year ago
Please land those testing only changes on mozilla-aurora. Given that we have some crashes there for l10n builds I can verify the functionality before requesting a check-in for mozilla-beta.
Whiteboard: [checkin-needed-aurora]

Comment 20

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e7ab2023afa
status-firefox46: affected → fixed
Whiteboard: [checkin-needed-aurora]
(Assignee)

Comment 21

a year ago
Ted, I have a question about the logic in mozcrash. I have seen a crash in a zh-TW build from yesterday:

https://treeherder.mozilla.org/logviewer.html#?job_id=1925606&repo=mozilla-aurora#L874

Sadly the crash has not been processed because the symbol path has not been given. Does it mean if we do not supply a symbol_url we would have to set a fake local path for symbols?

 14:28:50     INFO - PROCESS-CRASH | runner.py | application crashed [None]
 14:28:50     INFO - Crash dump filename: c:\jenkins\workspace\mozilla-aurora_functional\build\tmpazt4ub.mozrunner-1455229702\minidumps\6d998caa-c418-475d-bd8a-c629df6d0391.dmp
 14:28:50     INFO - No symbols path given, can't process dump.
Flags: needinfo?(ted)
Yes, you need to at least give mozcrash an empty local directory. If you're using the minidump_stackwalk binaries that fetch symbols over HTTP, that's where they will store the symbols they fetch. It can be a temp dir.
Flags: needinfo?(ted)
(Assignee)

Comment 23

a year ago
We would also need this testonly change for the upcoming 45 release. Flagging for checkin so it can be handled once the branches have been opened again.
Whiteboard: [checkin-needed-beta]
(Assignee)

Comment 24

a year ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> Yes, you need to at least give mozcrash an empty local directory. If you're
> using the minidump_stackwalk binaries that fetch symbols over HTTP, that's
> where they will store the symbols they fetch. It can be a temp dir.

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?
Flags: needinfo?(ted)

Comment 25

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d21b40099aa0
status-firefox45: affected → fixed
Whiteboard: [checkin-needed-beta]
(In reply to Henrik Skupin (:whimboo) from 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
Flags: needinfo?(ted)
(Assignee)

Comment 27

a year ago
Thanks Ted! I filed bug 1251196 to let mozcrash support that.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1232347
You need to log in before you can comment on or make changes to this bug.