Closed Bug 1304962 Opened 3 years ago Closed 3 years ago

incorrect argument processing in Aarch64 Xptcall causes obscure SIGSEGV

Categories

(Core :: XPCOM, defect, critical)

ARM
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox-esr45 50+ fixed
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: lersek, Assigned: lersek)

Details

Attachments

(1 file)

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.
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
Patch that fixes the bug.
Attachment #8794059 - Flags: review?(nfroyd)
Flipping Severity to Critical, since this bug makes Firefox practically unusable on Aarch64.
Severity: normal → critical
Component: Untriaged → XPCOM
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → ARM
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #8794059 - Flags: review?(nfroyd) → review+
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, bugzilla-daemon@mozilla.org 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.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3a85f50cff
fix argument processing in AARCH64 xptcall; r=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!
Assignee: nobody → lersek
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.
Status: NEW → ASSIGNED
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.
Great! Thank you guys for the education & patience. Cheers!
https://hg.mozilla.org/mozilla-central/rev/9f3a85f50cff
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
[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>.)
And, I think this patch qualifies for ESR45 under both:
- "major stability fixes when they're landed/merged onto mozilla-beta", and
- NPOTB.
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
nfroyd: forgot to needinfo to you for comment 14. Can you push this to aurora, beta, esr45? Thanks!
Flags: needinfo?(nfroyd)
(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. :(
Flags: needinfo?(nfroyd)
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.
Attachment #8794059 - Flags: approval-mozilla-esr45?
Attachment #8794059 - Flags: approval-mozilla-beta?
Attachment #8794059 - Flags: approval-mozilla-aurora?
Thanks Guys!
Attachment #8794059 - Flags: approval-mozilla-esr45?
Attachment #8794059 - Flags: approval-mozilla-esr45+
Attachment #8794059 - Flags: approval-mozilla-beta?
Attachment #8794059 - Flags: approval-mozilla-beta+
Attachment #8794059 - Flags: approval-mozilla-aurora?
Attachment #8794059 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.