1304962 - incorrect argument processing in Aarch64 Xptcall causes obscure SIGSEGV

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Laszlo Ersek]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160817112116

Steps to reproduce:

This issue has been encountered with the "firefox-45.3.0-1.el7_2.aarch64" package, but it applies equally to current gecko-dev master (at git commit c31ad35f39c6). 

$ git diff origin/esr45..master -- \
  xpcom/reflect/xptcall/md/unix/xptcinvoke_aarch64.cpp \
  xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_aarch64.s
[prints nothing]

The issue is also tracked by <https://bugzilla.redhat.com/show_bug.cgi?id=1358441>.

Steps to reproduce: simply start Firefox on aarch64, open a static page (or just stick with the blank start page), then wait about 60 seconds.


Actual results:

Firefox crashes with a SIGSEGV. The backtraces vary somewhat, but they always contain the following snippet:

> #6  0x000003ff94648cb8 in nsIOService::NewChannel2(nsACString_internal
>     const&, char const*, nsIURI*, nsIDOMNode*, nsIPrincipal*,
>     nsIPrincipal*, unsigned int, unsigned int, nsIChannel**) () from
>     /usr/lib64/firefox/libxul.so
> #7  0x000003ff945de83c in _NS_InvokeByIndex () from
>     /usr/lib64/firefox/libxul.so
> #8  0x000003ff94a78abc in XPCWrappedNative::CallMethod(XPCCallContext&,
>     XPCWrappedNative::CallMode) () from /usr/lib64/firefox/libxul.so


Expected results:

Firefox shouldn't crash.

Comment 1

[Laszlo Ersek]

This issue was introduced in the initial patch that ported Xptcall to aarch64, namely in

https://github.com/mozilla/gecko-dev/commit/8bc52be8d14eb3d68af3088de6291d85d71faf87

which addressed

https://bugzilla.mozilla.org/show_bug.cgi?id=963024

Comment 2

[Laszlo Ersek]

Attachment: [PATCH] xpcom: fix argument processing in AARCH64 xptcall

Patch that fixes the bug.

Comment 3

[Laszlo Ersek]

Flipping Severity to Critical, since this bug makes Firefox practically unusable on Aarch64.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8794059 [details] [diff] [review]
[PATCH] xpcom: fix argument processing in AARCH64 xptcall

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

+2 for the excellent commit message.

Comment 5

[Laszlo Ersek]

Thank you Nathan for the quick review!

Is there anything I should do now? I received an automated message saying

On 09/23/16 18:10, [e-mail] wrote:
> [...]
> 
> Once the patch is correctly formatted and all the nits are addressed,
> you should ask the person who reviewed your patch to push it to the
> try server for testing.
> 
> Once it comes back passing all our tests, your patch will be ready
> to check in!  To get your patch checked in you can add "checkin-needed"
> to the Keywords field on the bug summary page, and one of our helpful
> Code Sheriffs will come by to merge it into the Mozilla codebase.
> 
> [...]

So... should I ask you to push the patch to the try server for testing? :) Thank you.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3a85f50cff
fix argument processing in AARCH64 xptcall; r=froydnj

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Laszlo Ersek (RH) from comment #5)
> So... should I ask you to push the patch to the try server for testing? :)
> Thank you.

That would be the procedure for most normal patches that affect our Tier 1 platforms (Windows, Mac, Linux x86/x86-64, Android), but since this patch doesn't touch any of those areas, I've gone ahead and pushed it with only light modifications to the commit message.

Thanks for the patch!

Comment 8

[Laszlo Ersek]

Awesome, thanks!

(Another newbie question:) Should we change the Status field on this BZ? I'm unfamiliar with the Mozilla workflow. The Bug Fields help page mentions RESOLVED/FIXED -- "A fix for this bug is checked into the tree and tested". However, the FIXED resolution doesn't seem to be available to me, only INVALID / WONTFIX / DUPLICATE / WORKSFORME / INCOMPLETE are.

Hm, maybe if I flip it first to ASSIGNED... let me see.

Comment 9

[Andrew McCreight [:mccr8]]

NEW vs. ASSIGNED doesn't matter much in practice. Currently the patch is located on an integration branch, mozilla-inbound. Once it is landed to the main repository for Nightly, mozilla-central, the bug will be automatically marked RESOLVED FIXED. That should happen in the next day or two.

Comment 10

[Laszlo Ersek]

Great! Thank you guys for the education & patience. Cheers!

Comment 11

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/9f3a85f50cff

Comment 12

[Laszlo Ersek]

[Tracking Requested - why for this release]:

Nominating this patch for Firefox ESR45. The issue was originally encountered in ESR45 (see comment 0). The affected source code files are identical (pre-patch) between ESR45 and current master; the patch applies to both branches identically. Thanks.

(Sorry if this is not the right process -- I tried to follow <https://wiki.mozilla.org/Release_Management/ESR_Landing_Process>.)

Comment 13

[Laszlo Ersek]

And, I think this patch qualifies for ESR45 under both:
- "major stability fixes when they're landed/merged onto mozilla-beta", and
- NPOTB.

Comment 14

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

Thanks Laszlo! I admire your digging through our wiki pages. 
I would suggest this also could use uplift to our aurora and beta channels. If it's not part of the build, you can land it or ask for someone to land it for you. Nathan, maybe you would do that? 

Here's an even more fun wiki page that explains when to request uplift:  https://wiki.mozilla.org/Release_Management/Uplift_rules

Comment 15

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

nfroyd: forgot to needinfo to you for comment 14. Can you push this to aurora, beta, esr45? Thanks!

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> nfroyd: forgot to needinfo to you for comment 14. Can you push this to
> aurora, beta, esr45? Thanks!

Can do!  Sorry for dropping this one on the floor. :(

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8794059 [details] [diff] [review]
[PATCH] xpcom: fix argument processing in AARCH64 xptcall

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: NPTOB fix that makes life easier for distributors of Firefox.
User impact if declined: Firefox won't work 
Fix Landed on Version: 52
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: Bug 963024
[User impact if declined]: Firefox won't work correctly on a tier3 platform.
[Describe test coverage new/current, TreeHerder]: NPTOB
[Risks and why]: None
[String/UUID change made/needed]: None.

Comment 18

[Laszlo Ersek]

Thanks Guys!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/67759b4345ae

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/f61049d5f373