Last Comment Bug 1243684 - If symbols URL does not exist, don't add it to the run_command call
: If symbols URL does not exist, don't add it to the run_command call
Status: RESOLVED FIXED
:
Product: Release Engineering
Classification: Other
Component: Mozharness (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: Henrik Skupin (:whimboo) [away 02/18 - 02/27]
: Jordan Lund (:jlund)
:
Mentors:
: 1232347 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-28 01:45 PST by Henrik Skupin (:whimboo) [away 02/18 - 02/27]
Modified: 2016-03-10 04:00 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
Don't use 404 symbols_url (2.52 KB, patch)
2016-01-28 02:44 PST, Henrik Skupin (:whimboo) [away 02/18 - 02/27]
no flags Details | Diff | Splinter Review
Don't use auto-detected symbols_url if 404 v1.1 (2.29 KB, patch)
2016-01-28 13:08 PST, Henrik Skupin (:whimboo) [away 02/18 - 02/27]
ahalberstadt: review+
ted: feedback+
Details | Diff | Splinter Review

Description User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-01-28 01:45:52 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-01-28 02:44:32 PST
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?
Comment 2 User image Ted Mielczarek [:ted.mielczarek] 2016-01-28 06:01:50 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-01-28 06:30:24 PST
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 User image Ted Mielczarek [:ted.mielczarek] 2016-01-28 10:01:01 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-01-28 12:23:29 PST
I will have a look at various test logs to check for which tests we actually pass in the symbols URL.
Comment 6 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-01-28 12:48:45 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-01-28 13:08:34 PST
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.
Comment 8 User image Jordan Lund (:jlund) 2016-01-28 14:25:45 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-01-28 14:32:00 PST
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 User image Ted Mielczarek [:ted.mielczarek] 2016-01-29 03:07:10 PST
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 User image Andrew Halberstadt [:ahal] 2016-01-29 06:27:30 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-01 14:28:57 PST
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 User image Carsten Book [:Tomcat] 2016-02-02 01:45:22 PST
jordan, do you want to handle this checkin or shall we sheriff check this in during the normal checkin-needed stuff ?
Comment 14 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-02 01:50:10 PST
(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 16 User image Carsten Book [:Tomcat] 2016-02-03 03:27:36 PST
https://hg.mozilla.org/mozilla-central/rev/61fa62d120b6
Comment 17 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-03 06:52:04 PST
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 User image Ted Mielczarek [:ted.mielczarek] 2016-02-03 12:21:11 PST
Okay. This approach seems fine, I don't think backporting it should be a problem.
Comment 19 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-05 06:26:09 PST
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 21 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-12 01:20:18 PST
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 User image Ted Mielczarek [:ted.mielczarek] 2016-02-12 03:41:05 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-23 06:44:25 PST
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-23 06:50:53 PST
(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 User image Carsten Book [:Tomcat] 2016-02-24 07:54:08 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/d21b40099aa0
Comment 26 User image Ted Mielczarek [:ted.mielczarek] 2016-02-24 09:15:37 PST
(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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-02-25 03:01:28 PST
Thanks Ted! I filed bug 1251196 to let mozcrash support that.
Comment 28 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2016-03-10 04:00:10 PST
*** Bug 1232347 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.