Get rid of local patches, sync back up with Breakpad upstream

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
Crash Reporting
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44+ wontfix, firefox45+ wontfix, firefox46+ wontfix, firefox47 fixed, fennec+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

4 years ago
We took a bunch of local patches to support Breakpad-in-SPS and never got them upstreamed (to be fair they were pretty invasive). Breakpad has been replaced by LUL in SPS now, so we should be able to ditch the local patches and get back to a stock upstream Breakpad. First we need to get rid of the Breakpad-SPS integration code, which is bug 1007145.

There are three local patches we're carrying that we'll still need to get upstreamed:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/breakpad-patches/12-bug863475.patch - bug 863475, ARM EXIDX unwind parsing. There's a patch that was up for review upstream for this but it died on the vine. Need to resurrect that. In the short term we can rebase this on top of Breakpad tip and keep it as a local patch.
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/breakpad-patches/19-bug942290-breakpad-exidx-merge.patch - bug 942290 - This patch depends on the previous patch, but it's trivial
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/breakpad-patches/17-bug942407-usersig.patch - bug 942407 - This patch got put up for upstream review but got ignored. Need to poke someone: https://breakpad.appspot.com/984002/
(Assignee)

Updated

4 years ago
Depends on: 1069558
(Assignee)

Comment 1

4 years ago
A wrinkle, putting a note here so I don't forget. Upstream just landed a patch that requires Android NDK version r10c to build:
https://code.google.com/p/google-breakpad/source/detail?r=1396
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1167305
Ted, it sounds like we may need to update breakpad for bug 1164052. Is this something you can look into?
Flags: needinfo?(ted)
(Assignee)

Comment 4

3 years ago
No, sorry, there's non-trivial work in bug 1069558 to do to make this feasible. If you can get someone to fix that then I can probably do the rest.
Flags: needinfo?(ted)
Assignee: nobody → nchen
tracking-fennec: --- → ?
tracking-fennec: ? → +
Priority: -- → P1
Ted what do you think about moving the current breakpad aside, which will continue to be used for BHR, and then pasting in the new breakpad for crash reports? Jim could then update BHR to use something else at his leisure.
Flags: needinfo?(ted)
(Assignee)

Comment 6

3 years ago
No, I think that's an even worse idea. That's how we wind up with two copies of chromium in the tree and never fix it.
Flags: needinfo?(ted)
Depends on: 1196381
I think it'd be fine if we rip out the Breakpad code in ThreadStackHelper first, then work on a replacement for later. The BHR native stack hasn't been of much use anyways. I filed bug 1196381 for this.
(Assignee)

Updated

3 years ago
Blocks: 1196981
This bug is no longer blocked on bug 1069558 because of bug 1196381. Ted, is that all you need to continue working on this bug?
Assignee: nchen → nobody
No longer depends on: 1069558
Flags: needinfo?(ted)
(Assignee)

Comment 9

3 years ago
That's great, thanks!
Flags: needinfo?(ted)
(Assignee)

Comment 11

3 years ago
That try push was busted in several different ways, but I think I've got those all sorted now. I just need to rebase the two local patches we rely on that haven't been upstreamed and I think we'll be in good shape.
(Assignee)

Comment 12

3 years ago
I've got our local patches rebased and applied, and everything seems good. I just need to tweak one more thing that I forgot about to match upstream (The dump_syms binary on Mac changed its commandline arguments slightly.)
Tracking this for 44 since it was brought up in the channel meeting today and seems like a good thing to stay on top of. It sounded like uplifting may be a bit too risky so it would be better riding with 44 to release.
status-firefox44: --- → affected
tracking-firefox44: --- → +
(Assignee)

Comment 14

3 years ago
Created attachment 8676746 [details]
MozReview Request: bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)
(Assignee)

Comment 15

3 years ago
Created attachment 8676747 [details]
MozReview Request: bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium

bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r?glandium
Attachment #8676747 - Flags: review?(mh+mozilla)
(Assignee)

Comment 16

3 years ago
Created attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

bug 1069556 - local build changes to match up with upstream Breakpad. r?glandium,benwa

This commit contains a few things:
* Misc build fixup to sync with upstream--adding a few new moz.build files,
  source files
* The final bits of unhooking Breakpad from the profiler:
** Revert to only building toolkit/crashreporter if MOZ_CRASHREPORTER.
** Stop building bits of Breakpad that we only needed for the profiler.
** Remove a few bits of profiler code that were used to interface with Breakpad.
** Remove toolkit/crashreporter/breakpad-logging, which was only used to
   suppress Breakpad logging for the in-process stackwalker.
* Upstream removed their Android-compat sys/ucontext.h because the Android NDK
  added it, but the bionic we're using for Gonk builds is too old, so add a
  copy of sys/ucontext.h from bionic master to keep Gonk building.
Attachment #8676748 - Flags: review?(mh+mozilla)
Attachment #8676748 - Flags: review?(bgirard)
(Assignee)

Comment 17

3 years ago
I got hung up for a bit because the dump_syms output was different on Mac, but after investigation it looks like the symbol dumper stopped outputting PUBLIC lines for symbols that already had a FUNC entry, so it's not harmful.

I'll push this to try again when try is open...

Updated

3 years ago
Attachment #8676748 - Flags: review?(bgirard) → review+
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

https://reviewboard.mozilla.org/r/22803/#review20345

Profiler changes look good, we shouldn't be using breakpad to symbolicate anymore.
https://reviewboard.mozilla.org/r/22799/#review20411

::: toolkit/crashreporter/update-breakpad.sh:8
(Diff revision 1)
> +# Usage: update-breakpad.sh <path to breakpad git clone> [rev, defaults to HEAD]

FWIW, we tend not to rely on already existing clones.
Comment on attachment 8676747 [details]
MozReview Request: bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium

https://reviewboard.mozilla.org/r/22801/#review20413

If I run update-breakpad.sh /path/to/breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c, I get some weird CRLF related differences from what applying the patch gives. Apart from that, looks good.

::: toolkit/crashreporter/update-breakpad.sh:26
(Diff revision 1)
>  # remove some extraneous bits
> -rm -rf ${crashreporter_dir}/google-breakpad/src/third_party/protobuf ${crashreporter_dir}/google-breakpad/src/testing/ ${crashreporter_dir}/google-breakpad/src/tools/gyp/
> +rm -rf \
> +  ${crashreporter_dir}/google-breakpad/docs/ \
> +  ${crashreporter_dir}/google-breakpad/src/third_party/protobuf \
> +  ${crashreporter_dir}/google-breakpad/src/testing/ \
> +  ${crashreporter_dir}/google-breakpad/src/tools/gyp/ \
> +  ${crashreporter_dir}/google-breakpad/src/processor/testdata/ \
> +  ${crashreporter_dir}/google-breakpad/src/tools/windows/dump_syms/testdata/

Maybe this should be part of the update-breakpad.sh patch?
Attachment #8676747 - Flags: review?(mh+mozilla) → review+
Attachment #8676748 - Flags: review?(mh+mozilla)
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

https://reviewboard.mozilla.org/r/22803/#review20415

::: toolkit/crashreporter/crashreporter.mozbuild:17
(Diff revision 1)
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['OS_TARGET'] == 'Android':

do we ever have MOZ_WIDGET_TOOLKIT=gonk with OS_TARGET != Android? That would be surprising.

::: toolkit/crashreporter/google-breakpad/src/client/linux/dump_writer_common/moz.build:12
(Diff revision 1)
> +Library('breakpad_linux_dump_writer_common_s')

You're not using this library directly, so you don't need to give it a name.

::: toolkit/crashreporter/google-breakpad/src/common/Makefile.in:11
(Diff revision 1)
> +# memory.h in this dir breaks things if -I$(srcdir) gets added.

Can you be more specific?

::: toolkit/crashreporter/google-breakpad/src/common/linux/moz.build:35
(Diff revision 1)
> -    ]
> +]

You've been birotted by bug 1175857

::: toolkit/crashreporter/google-breakpad/src/common/moz.build
(Diff revision 1)
> -        'arm_ex_reader.cc',
> -        'arm_ex_to_module.cc',

Those two files come from the 00-arm-exidx-rollup.patch patch. If you're not building them, what does the patch do?

This obviously needs to be folded with the previous patch.
(Assignee)

Comment 23

3 years ago
https://reviewboard.mozilla.org/r/22801/#review20413

Odd, I produced this changeset by running the script.

> Maybe this should be part of the update-breakpad.sh patch?

Good call, don't know how that snuck into this patch.
(Assignee)

Comment 24

3 years ago
https://reviewboard.mozilla.org/r/22803/#review20415

> do we ever have MOZ_WIDGET_TOOLKIT=gonk with OS_TARGET != Android? That would be surprising.

I guess not, I can remove that last bit.

> Can you be more specific?

Some system headers `#include <memory.h>` (a totally reasonable thing to do) and they wind up including *this* memory.h, which obviously breaks things. I'll update the comment to note that.

> Those two files come from the 00-arm-exidx-rollup.patch patch. If you're not building them, what does the patch do?

Uh, oops? I generated this patch series iteratively by first updating to stock Breakpad and making things build, and then rebasing that patch, and I guess I missed the part where I actually needed to make these files build again.
(Assignee)

Comment 25

3 years ago
https://reviewboard.mozilla.org/r/22799/#review20411

> FWIW, we tend not to rely on already existing clones.

Cloning Breakpad is kind of a hassle. It requires you to clone depot_tools first and run their `fetch` script because they're using gclient to handle dependent subrepos.
(Assignee)

Comment 26

3 years ago
Comment on attachment 8676746 [details]
MozReview Request: bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8676747 [details]
MozReview Request: bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium

bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium
Attachment #8676747 - Attachment description: MozReview Request: bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r?glandium → MozReview Request: bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium
(Assignee)

Updated

3 years ago
Attachment #8676748 - Flags: review?(mh+mozilla)
(Assignee)

Comment 28

3 years ago
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

bug 1069556 - local build changes to match up with upstream Breakpad. r?glandium,benwa

This commit contains a few things:
* Misc build fixup to sync with upstream--adding a few new moz.build files,
  source files
* The final bits of unhooking Breakpad from the profiler:
** Revert to only building toolkit/crashreporter if MOZ_CRASHREPORTER.
** Stop building bits of Breakpad that we only needed for the profiler.
** Remove a few bits of profiler code that were used to interface with Breakpad.
** Remove toolkit/crashreporter/breakpad-logging, which was only used to
   suppress Breakpad logging for the in-process stackwalker.
* Upstream removed their Android-compat sys/ucontext.h because the Android NDK
  added it, but the bionic we're using for Gonk builds is too old, so add a
  copy of sys/ucontext.h from bionic master to keep Gonk building.
Attachment #8676748 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

https://reviewboard.mozilla.org/r/22803/#review20837
Duplicate of this bug: 1217857
Oh good this looks close to landing, new b2g devices Will need this for the aarch64 support.
(Assignee)

Comment 37

3 years ago
So I thought I was done, but I had to wrangle B2G builds some more. I just submitted a Try run to check all the various B2G flavors, but this patch builds with B2G for me locally, as well as 64-bit Linux and arm/x86 Android.
(Assignee)

Comment 38

3 years ago
Comment on attachment 8676746 [details]
MozReview Request: bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22799/diff/2-3/
(Assignee)

Comment 39

3 years ago
Comment on attachment 8676747 [details]
MozReview Request: bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22801/diff/2-3/
(Assignee)

Comment 40

3 years ago
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22803/diff/2-3/
Attachment #8676748 - Attachment description: MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r?glandium,benwa → MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=glandium,benwa
Glandium, Ted: I noticed this bug is marked tracking for 44 and that two patches were r+d but don't see a Sheriff landed these patches. Do these patches need to be uplifted to Aurora44 or not?
Flags: needinfo?(ted)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 42

3 years ago
These patches never landed. They got r+ but I got hung up on making them build for B2G.
Flags: needinfo?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #42)
> These patches never landed. They got r+ but I got hung up on making them
> build for B2G.

Do you want me to get this landed on m-c with a sheriffs help? Should I set the checkin-needed keyword? Or will the patches not apply cleanly now?
(Assignee)

Comment 44

3 years ago
They apply cleanly but they don't build on all the B2G variants. I need to spend some more time on it unfortunately.
Given that we are halfway through the beta44 cycle, I would like to suggest wontfixing this for fx44. Tracked for 45.
status-firefox44: affected → wontfix
status-firefox45: --- → affected
tracking-firefox45: --- → +
Blocks: 1187826
(Assignee)

Comment 46

2 years ago
Sorry, I got hung up on making this work on B2G and didn't have time to fix it. I spent some time on this yesterday and I made some progress, I'll give it another spin and see how it goes.

Comment 50

2 years ago
Ted, where are we on this?
We are bad enough in Android stability that we consider not shipping, and there's a firm belief that this Breakpad update will gives us more actionable data on what to fix to get us back to a better state.
Flags: needinfo?(ted)
(Assignee)

Comment 52

2 years ago
Comment on attachment 8676746 [details]
MozReview Request: bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22799/diff/3-4/
(Assignee)

Comment 53

2 years ago
Comment on attachment 8676747 [details]
MozReview Request: bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22801/diff/3-4/
(Assignee)

Updated

2 years ago
Attachment #8676748 - Attachment description: MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=glandium,benwa → MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium
(Assignee)

Comment 54

2 years ago
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22803/diff/3-4/
(Assignee)

Comment 55

2 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #50)
> Ted, where are we on this?
> We are bad enough in Android stability that we consider not shipping, and
> there's a firm belief that this Breakpad update will gives us more
> actionable data on what to fix to get us back to a better state.

Sorry, fixing the B2G build turned out to be more work than I anticipated, but I believe I have it all working now. I've pushed to try again and asked for re-review on the changes to make that work. If everything looks good on Try this should be landable immediately.
Flags: needinfo?(ted)
(Assignee)

Comment 57

2 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #51)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7d63e854256

The Windows builds on this push are burning due to bug 1240993, so I re-pushed just Windows builds.
(Assignee)

Comment 58

2 years ago
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

I'd like a once-over on the changes here, but Mozreview won't let me unset the r+ there.
Attachment #8676748 - Flags: review+ → review?(mh+mozilla)
Comment on attachment 8676746 [details]
MozReview Request: bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

https://reviewboard.mozilla.org/r/22799/#review28811

::: toolkit/crashreporter/update-breakpad.sh:6
(Diff revision 4)
> -# Usage: update-breakpad.sh <path to breakpad SVN>
> +set -v -e -x

-v -x is nice when debugging, but when simply using, it's kind of overkill.

::: toolkit/crashreporter/update-breakpad.sh:15
(Diff revision 4)
> -crashreporter_dir=`dirname $0`
> +crashreporter_dir=`realpath $(dirname $0)`

You can nest $() instead of mixing with ``: $(realpath $(dirname $0))

::: toolkit/crashreporter/update-breakpad.sh:61
(Diff revision 4)
> -find ${crashreporter_dir}/google-breakpad -name "*.orig" -print0 | xargs -0 rm
> +find ${crashreporter_dir}/google-breakpad -name "*.orig" -exec rm '{}' \;

-print0 | xargs -0 rm tends to be faster because rm is forked less, but meh.
Attachment #8676746 - Flags: review+
(Assignee)

Comment 60

2 years ago
Mozreview is sort of confusing here, but I really just want a once-over on the local build changes patch:
https://reviewboard.mozilla.org/r/22803/diff/4#index_header
Comment on attachment 8676748 [details]
MozReview Request: bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa, r?glandium

https://reviewboard.mozilla.org/r/22803/#review29025

Maybe move the gonk headers to /other-licenses/gonk?
Attachment #8676748 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 62

2 years ago
https://reviewboard.mozilla.org/r/22803/#review29025

I actually changed tack on this for the final patch and just copied the old version of the Breakpad headers that were removed or changed, so I think they're OK there since they originate from Breakpad.
(Assignee)

Comment 63

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e628eff02f5cf6fdc118ef0e61f3e17f627e7d2
bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

https://hg.mozilla.org/integration/mozilla-inbound/rev/09fd4f3ab6e764016fe073efb226f03b5969af59
bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/31c98f5e107b9271be88e7c8543c4dbb4a2b6526
bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa,glandium
backed out for multiple test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=20487667&repo=mozilla-inbound
Flags: needinfo?(ted)
Following our conversation on irc, the problem is that the breakpad code used by fileid is using the wrapped STL, which means it uses moz_xmalloc, which is not linked in fileid. In any case, the DISABLE_STL_WRAPPING workaround in 
toolkit/crashreporter/google-breakpad/src/common/linux/Makefile.in is not enough anymore, and trying to expand it is worthless because the STL *is* used, and we don't want unwrapped code in libxul. That workaround can go away, independently of anything else.

There are several solutions to this:
- build fileid as a GeckoProgram with linkage=None, but that means on mac there will be a dependency on libmozglue.dylib.
- statically link mozalloc to fileid (which requires to assign a LIBRARY_NAME to mozalloc)
- build another set of breakpad libraries (only those listed in testing/tools/fileid/moz.build) entirely built without STL wrapping, and use it for fileid, instead of sharing the same as for libxul.

The second one is probably the least effort, assuming it works. A cursory test suggests that it does.

Comment 69

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7b86ad8cd86
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 years ago
Status: RESOLVED → REOPENED
Flags: needinfo?(ted)
Resolution: FIXED → ---
(Assignee)

Comment 70

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a319694b46d54d9240842f0c525cf5570cf4649a
bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

https://hg.mozilla.org/integration/mozilla-inbound/rev/16f9d3a6d2ef6a6efd088e3b8eff0a4723daef8f
bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/24dbe7da737057008fd62dafc1783aedb840dc23
bug 1069556 - local build changes to match up with upstream Breakpad. r=benwa,glandium
(Assignee)

Comment 71

2 years ago
This broke on Android and Mac, so I give up.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #71)
> This broke on Android and Mac, so I give up.

WAIT WHAT
Created attachment 8713894 [details] [diff] [review]
Fix fileid on mac and android

That was a fileid problem too, so I pushed glandium's option one from comment 68 to try (based on when this was on inbound) and it looks ok:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12c0e8322cf0
Attachment #8713894 - Flags: review?(ted)
FYI, I know you are doing your best. We really want this to land asap in m-c. We have a few bugs we cannot act because of the lack of relevant backtrace. 
We hope the new version of breakpad will help with this.
status-firefox46: --- → affected
status-firefox47: fixed → affected
(Assignee)

Comment 76

2 years ago
Comment on attachment 8713894 [details] [diff] [review]
Fix fileid on mac and android

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

This looks sensible enough, but we ought to verify that fileid still works. Maybe we can tweak testAssertStack in selftest.py to pass a symbols path? It should be as simple as doing:
```
path = os.path.join(build_obj.distdir, 'crashreporter-symbols')
if os.path.isdir(path):
  ...
```
Attachment #8713894 - Flags: review?(ted) → review+
Created attachment 8715477 [details] [diff] [review]
Fix fileid to build on mac and android and add a selftest

Patch with updated selftest.
Assignee: nobody → cmanchester
Status: REOPENED → ASSIGNED
Assignee: cmanchester → ted
(In reply to Chris Manchester [:chmanchester] from comment #77)
> Created attachment 8715477 [details] [diff] [review]
> Fix fileid to build on mac and android and add a selftest
> 
> Patch with updated selftest.

Try, in case the automation build environment breaks that for some reason:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7900de044b55
(Assignee)

Comment 79

2 years ago
You're the best! I owe you a beer.
[Tracking Requested - why for this release]:
We might want to uplift that to 46 when it is ready (and if the developers agree)
tracking-firefox46: --- → ?
(Assignee)

Comment 82

2 years ago
Ok, with chmanchester's patch applied, that latest try run looks good!
(Assignee)

Comment 83

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bf35bec8aec0ea8e151097434875f0db0559fa
bug 1069556 - change update-breakpad.sh to work with Breakpad's git repo (NPOTB)

https://hg.mozilla.org/integration/mozilla-inbound/rev/62a0d8c93da2ca24f3f87642a2989c4741f214d4
bug 1069556 - sync to Breakpad c53ed143108948eb7e2d7ee77dc8c0d92050ce7c. r=glandium, benwa

Comment 84

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9bf35bec8ae
https://hg.mozilla.org/mozilla-central/rev/62a0d8c93da2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Congratulations guys, this will really help us (especially on mobile). Many thanks!
(In reply to Pulsebot from comment #86)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6c98cc47620d

This reads: "Followup for bug 1069556: Remove what was removed in bug 1245055. r=me"
(Assignee)

Comment 89

2 years ago
Oops, sorry for undoing your changes there!
We probably don't want to uplift this in beta.
However, having it in aurora might help. Ted, do you agree? If so, could you fill the uplift request to aurora?
status-firefox45: affected → wontfix
Flags: needinfo?(ted)
(Assignee)

Comment 91

2 years ago
I don't really know, I wasn't fixing this to take care of any particular problem, although I know there was hope it would help with some bad Android crash reports. We probably need to take a look at the crash stats for the past few days to see if it made any noticeable improvement.
Flags: needinfo?(ted)
OK, thanks.
Kairo, could you verify if it has a positive impact before we uplift it? Danke!
Flags: needinfo?(kairo)
Ted, any thoughts here? Are you still working on this or shall I not worry about it for 46? 
Tracking for now so that I will come back to it before merge day.
tracking-firefox46: ? → +
Flags: needinfo?(ted)
(Assignee)

Comment 95

2 years ago
Like I said in comment 91, this needs Kairo to weigh in on whether this made any difference in Android signatures. From some unittest log investigation in another bug it doesn't look like this fixed the situation.
Flags: needinfo?(ted)
We believed that this would help with bug 1218850 (Crash at hex address on x86 Android). We still see these signatures after the upgrade. I think we should just let this ride the trains.

Comment 97

2 years ago
Kevin is following the signatures more closely than I am, so I trust his judgement on this.
Flags: needinfo?(kairo)
OK, thanks!
status-firefox46: affected → wontfix
(Assignee)

Updated

2 years ago
Blocks: 791107
You need to log in before you can comment on or make changes to this bug.