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

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: 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?

Comment 2

[(not currently active) Ted Mielczarek]

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?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 4

[(not currently active) Ted Mielczarek]

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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Don't use auto-detected symbols_url if 404 v1.1

This patch should practically do what I mentioned in the last comments.

Comment 8

[Jordan Lund (:jlund)]

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

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 10

[(not currently active) Ted Mielczarek]

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!

Comment 11

[Andrew Halberstadt [:ahal]]

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?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 13

[Carsten Book [:Tomcat]]

jordan, do you want to handle this checkin or shall we sheriff check this in during the normal checkin-needed stuff ?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61fa62d120b6

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61fa62d120b6

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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!

Comment 18

[(not currently active) Ted Mielczarek]

Okay. This approach seems fine, I don't think backporting it should be a problem.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e7ab2023afa

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 22

[(not currently active) Ted Mielczarek]

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.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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?

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d21b40099aa0

Comment 26

[(not currently active) Ted Mielczarek]

(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

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Ted! I filed bug 1251196 to let mozcrash support that.