1524142 - MOZ_CRASHREPORTER=1 not defined in default aarch64 build but is in default i686 build

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm trying to get an x86 plugin-container.exe to host an x86 GMP for use from an aarch64 parent process. One of the problems I encountered was that if you don't specify --disable-crashreporter in an i686 build, MOZ_CRASHREPORTER gets defined to 1. Whereas if you don't specify --disable-crashreporter in an aarch64 build, MOZ_CRASHREPORTER is defined to ''.

This discrepency caused my parent aarch64 process to not append the command line args used by plugin-container to init the crash reporter in the GMP process [1], but the x86 plugin-container was expecting them and so consumed command line args that it wasn't supposed to and which were required later [2] and so we crashed on startup.

[1] https://searchfox.org/mozilla-central/rev/78cd247b5d7a08832f87d786541d3e2204842e8e/toolkit/xre/nsEmbedFunctions.cpp#509
[2] https://searchfox.org/mozilla-central/rev/78cd247b5d7a08832f87d786541d3e2204842e8e/toolkit/xre/nsEmbedFunctions.cpp#599

Comment 1

[(Away)]

This will likely be addressed by bug 1513284. In particular see bug 1513284 comment 4.

Comment 2

[Eric Rahm [:erahm]]

Gabriele, is there a reason to keep the crashreporter disabled locally at this point?

Comment 3

[Gabriele Svelto [:gsvelto]]

No, there should be no difference between the two platforms. We're probably missing some bit in the default configuration. I'll check.

Comment 4

[Gabriele Svelto [:gsvelto]]

We're missing a bit in old-configure.in. I'll fix it.

Comment 5

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1524142 - Ensure that MOZ_CRASHREPORTER is properly defined in Windows/AArch64 builds

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d30d431b5d8
Ensure that MOZ_CRASHREPORTER is properly defined in Windows/AArch64 builds r=glandium

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6d30d431b5d8

Comment 8

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9040401 [details]
Bug 1524142 - Ensure that MOZ_CRASHREPORTER is properly defined in Windows/AArch64 builds

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1517730

User impact if declined

The crash reporter is not enabled by default in builds

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

See comment 0

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

We're only defining an environment variable during the build

String changes made/needed

None

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9040401 [details]
Bug 1524142 - Ensure that MOZ_CRASHREPORTER is properly defined in Windows/AArch64 builds

Let's take this now for beta 6 and verify the results in beta.

Comment 10

[Stefan Hindli [:stefan_hindli]]

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

Comment 11

[Daniel Bodea [:danibodea]]

I have attempted to validate this fix, but these builds don't seem to run on Lenovo Yoga C630-13Q50 with Windows 10 Home (v1803).

I have attempted to install this installer:
http://archive.mozilla.org/pub/firefox/nightly/2019/02/2019-02-07-21-44-23-mozilla-central/firefox-67.0a1.en-US.win64-aarch64.installer.exe
which installed successfully, but simply did not open.

I have also tried these:
http://archive.mozilla.org/pub/firefox/nightly/2019/02/2019-02-07-21-44-23-mozilla-central/firefox-67.0a1.en-US.win64-aarch64.zip
http://archive.mozilla.org/pub/firefox/nightly/2019/02/2019-02-07-09-48-41-mozilla-central/firefox-67.0a1.en-US.win64-aarch64.zip

Comment 12

[Gabriele Svelto [:gsvelto]]

I believe that we're running into bug 1526250 which makes it hard to verify this, Chris is the problem fixed for you in recent builds?

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

I updated my tree, and I see MOZ_CRASHREPORTER being set by configure in both my aarch64 and i686 builds:

$ grep MOZ_CRASHREPORTER obj-i686-dbg/config.status obj-aarch64-dbg/config.status
obj-i686-dbg/config.status:    'MOZ_CRASHREPORTER': '1',
obj-i686-dbg/config.status:    'MOZ_CRASHREPORTER_INJECTOR': '1',
obj-i686-dbg/config.status:    'MOZ_CRASHREPORTER': '1',
obj-i686-dbg/config.status:    'MOZ_CRASHREPORTER_INJECTOR': '1',
obj-aarch64-dbg/config.status:    'MOZ_CRASHREPORTER': '1',
obj-aarch64-dbg/config.status:    'MOZ_CRASHREPORTER_INJECTOR': '',
obj-aarch64-dbg/config.status:    'MOZ_CRASHREPORTER': '1',

Note that MOZ_CRASHREPORTER_INJECTOR is not making it into the defines section on the aarch64 build, but is on the i686 build. Is that expected?

My mozconfigs:

$ cat i686.dbg.mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-i686-dbg

ac_add_options --enable-debug
ac_add_options --enable-debug-symbols
ac_add_options --disable-optimize
ac_add_options --target=i686

mk_add_options AUTOCLOBBER=1

ac_add_options --disable-tests

#opt!
export WIN32_REDIST_DIR="C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Redist\MSVC\14.16.27012\x86\Microsoft.VC141.CR"T

ac_add_options --disable-launcher-process

$ cat aarch64.dbg.mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-aarch64-dbg

#ac_add_options --disable-optimize
ac_add_options --enable-debug
ac_add_options --enable-debug-symbols
ac_add_options --disable-tests

mk_add_options AUTOCLOBBER=1
ac_add_options --target=aarch64

export WIN32_REDIST_DIR="C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Redist\MSVC\14.16.27012\arm64\Microsoft.VC141.CRT"

ac_add_options --disable-launcher-process

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to Chris Pearce [:cpearce (GMT+13)] from comment #13)

Note that MOZ_CRASHREPORTER_INJECTOR is not making it into the defines section on the aarch64 build, but is on the i686 build. Is that expected?

MOZ_CRASHREPORTER_INJECTOR is an x86-only thing for use with the Flash plugin so that sounds OK and shouldn't cause problems.

Comment 15

[Gabriele Svelto [:gsvelto]]

As Ted said, I think we can mark this as verified then.

Comment 16

[Daniel Bodea [:danibodea]]

Considering that the Aarch64 builds can now run and considering the fact that this uplift is marked so that is should be verified manually, can you please tell me how I should verify it?

I have attempted to verify this fix in Nightly first, by confirming that the "--enable-crashreporter" property is displayed in a build with the fix, while it doesn't display in a build prior to the fix (in the "about:buildconfig" page). The problem is that the property is displayed in builds both prior and after the fix. I assume my steps to reproduce are incorrect.

Builds tested:

1. Prior to fix: Nightly v67.0a1 (2019-02-01) (64-bit) (Aarch64 build)
2. After the fix: nightly v67.0a1 (2019-02-11) (64-bit) (Aarch64 build)

How should I verify this fix? Thanks.

Comment 17

[Daniel Bodea [:danibodea]]

After talking with :gsvelto on slack, it appears that this issue was verified by the reporter and should be marked as such.
Thank you!

Comment 18

[Daniel Bodea [:danibodea]]

Removing "qe-verify+" flag and setting version flags as verified.