1564900 - [s390x] Addressbar not working (Wrong size for type statvfs)

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect, P1)

Description

[msirringhaus]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Build and start Firefox 68 on s390x.

Actual results:

Addressbar is only partly usable. No suggestions, no search engines, no auto-complete. Full addresses (starting with https://) work, as well as about:config and others. But www.mozilla.org does not work (hitting return or Go-Button has no effect at all).

In the console, I see:

JavaScript error: resource://gre/modules/workers/require.js line 140 > Function line 3 > eval, line 933: Error: Error: Wrong size for type statvfs: expected 88, found 96 (ctypes.StructType("statvfs", [{ "padding_0": ctypes.uint8_t.array(4) }, { "f_frsize": ctypes.int64_t }, { "padding_1": ctypes.uint8_t.array(12) }, { "f_bavail": ctypes.uint64_t }, { "padding_2": ctypes.uint8_t.array(56) }]))

If I remove the check in toolkit/components/osfile/modules/osfile_shared_allthreads.jsm:932 it seems to work as expected (but thats obviously not a real solution).

The struct from the sys-headers has indeed a size of 88, the definition also adds up to 88, but for some reason converting that to a StructType with ctypes.StructType(this.name, struct) gives 96.

I am not sure if this is a big endian problem, or a "the sys-headers are slightly different"-problem.

From a first glance into js/src/ctypes/CTypes.* I did not find any parts that might cause endian-problems.

Expected results:

Address bar works with all checks in place

Comment 1

[msirringhaus]

Phew, found it.

The code in dom/system/OSFileConstants.cpp is using not sys/statvfs.h as I was expecting, but sys/vfs.h and "renaming" the structs to be called statvfs. Otherwise I would have caught the problem way earlier.

Comparing struct statfs from /usr/include/bits/statfs.h on a machine that fails and a machine that works, shows that the structs are defined differently.

On the one that works, fields of interest are of type __fsword_t with size 8. On the machine that fails they are unsigned int with size 4.

And in toolkit/components/osfile/module/osfile_unix_back.jsm:234 the field is defined as:

         statvfs.add_field_at(Const.OSFILE_OFFSETOF_STATVFS_F_FRSIZE,
                        "f_frsize", Type.unsigned_long.implementation);

which is size 8.

I have no idea how to solve this in a general way. Maybe something like "if sizeof(struct statvfs) == 88, then use Type.unsigned_int instead of Type.unsigned_long".

Comment 2

[Adrian Florinescu [:aflorinescu]]

I'm not that sure that this report actually belongs to Firefox::Address bar or there's a better component for it, but I think it's a good triage starting point.

msirringhaus, could you please add as additional information if this is a regression from fx67?

Comment 3

[msirringhaus]

We only do ESR-version, so I can't tell you for certain. And the FF60esr didn't work on s390x at all, so I don't even know if its a regression from 60esr.

But looking at the code snippets in question: They have all been there for a long time (around 2014).

Currently, I don't even think its a Firefox bug, but might be a glibc-bug.
Reached out to the glibc-maintainers and waiting for a response there.

Comment 4

[msirringhaus]

Ok, I got a response from glibc-maintainers.

According to them, it is not a glibc-bug but a Firefox-bug:
A userspace application has no guarantee about the type or order of struct-members.
Firefox always assumes f_frsize is "unsigned long".

It works for the other members of that struct, as they have their own type defined (fsblkcnt_t), which size is calculated at compile time.

Compare:

         // Types fsblkcnt_t and fsfilcnt_t, used by structure |statvfs|
         Type.fsblkcnt_t = Type.uintn_t(Const.OSFILE_SIZEOF_FSBLKCNT_T).withName("fsblkcnt_t");
       // ....snip....
         statvfs.add_field_at(Const.OSFILE_OFFSETOF_STATVFS_F_BAVAIL,
                        "f_bavail", Type.fsblkcnt_t.implementation);

vs.

         statvfs.add_field_at(Const.OSFILE_OFFSETOF_STATVFS_F_FRSIZE,
                        "f_frsize", Type.unsigned_long.implementation);

The solution would be to have a similar construct for f_frsize, where Const.OSFILE_SIZEOF_F_RSIZE would be defined in OSFileConstants.cpp, using something like #define MEMBER_SIZE_MACRO(t,m) sizeof(((t*)0)->m) (define from skia)

Comment 5

[Marco Bonardo [:mak]]

If you wish to contribute a patch, we can find a reviewer for it.

Comment 6

[msirringhaus]

Attachment: mozilla-bmo1564900.patch

As mentioned in the patch, this is not perfect. There is no guarantee about the signedness either, so some platforms currently have signed integers, others have unsigned integers.

This patch always assumes unsigned, as the previous code did so, too and that seems to have worked for a few years now.

Comment 7

[Marco Bonardo [:mak]]

Yoric, any idea who may be a good reviewer for this?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9078389 [details] [diff] [review]
mozilla-bmo1564900.patch

I'll try and review it this week.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(for some reason, I cannot place a r?)

Comment 10

[Marco Bonardo [:mak]]

splinter reviews are disabled, because now we are supposed to use phabricator.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Hey, msirringhaus, I'd be happy to review this, but these days, we use phabricator for review requests. Could you move your patch over there?

The instructions are here: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html .

Comment 12

[msirringhaus]

This looks like a great way to reduce the number of incoming patches.
I'll try to get this going, but it looks like this will take a while.

Comment 13

[Marco Bonardo [:mak]]

./mach bootstrap should setup most of the requirements once you have a local tree clone.

Comment 14

[msirringhaus]

Attachment: Bug 1564900 - [OS.File] Fix size of statvfs::f_frsize;r?Yoric

Make Firefox not assume type of member statvfs::f_frsize from sys head, which may vary

Comment 15

[msirringhaus]

I apologize for my snarky comment above. Shouldn't post while hungry.

./mach bootstrap doesn't work for my distribution, but hopefully this worked anyways

Comment 16

[David Teller [:Yoric] - still alive but not very active]

(In reply to msirringhaus from comment #12)

This looks like a great way to reduce the number of incoming patches.

I know. Hopefully, Bugzilla will integrate better with Phabricator soon.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cce1e378da8
[OS.File] Fix size of statvfs::f_frsize;r=Yoric

Comment 18

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7cce1e378da8

Comment 19

[David Teller [:Yoric] - still alive but not very active]

...and it's done!

Thank you very much for the patch, msirringhaus!

Comment 20

[msirringhaus]

Cool!

Could this be uplifted to ESR by any chance?

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Note: in the future, don't hesitate to add "Request information from...", to ensure that I receive your question :)

I personally very seldom request uplifts, so I'm not entirely sure if it can. Regardless, before any uplift, we want to make sure that it stays at least 1-2 weeks on Nightly and doesn't seem to cause a big problem. After this, we can request uplift. Note that I'll be on PTO during the next ~2 weeks, so you'll probably have to ask :mak (for instance) to handle this.

Comment 22

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: This bug may be of interest for enterprises.

yes, I think it may be worth trying to uplift to ESR because of the possible enterprise interest in this.
We may evaluate that next week, enough days should have passed at that point.
I'll leave a needinfo on myself as a reminder.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 9078662 [details]
Bug 1564900 - [OS.File] Fix size of statvfs::f_frsize;r?Yoric

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Urlbar (and likely other components) not working on certain hardware/software configurations, for example Ubuntu Server for IBM Z and LinuxONE. May be of interest to enterprises.
• User impact if declined: urlbar and probably other components using OS.File are broken.
• Fix Landed on Version: 70
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): It's a cpp change, with potential to increase crash risk, but it's a simple patch that should have no negative effect on Tier1 platforms (the final result should not change at all there). The patch has been in Nightly for more than 10 days, no relevant problems reported.
• String or UUID changes made by this patch: none

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9078662 [details]
Bug 1564900 - [OS.File] Fix size of statvfs::f_frsize;r?Yoric

Fixes the awesomebar for some Tier 3 builds where statvfs::f_frsize isn't the expected size. Patch doesn't change anything for builds we ship. Approved for 68.1esr.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9078662 [details]
Bug 1564900 - [OS.File] Fix size of statvfs::f_frsize;r?Yoric

Might as well take this on Beta for Fx69 as well.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/ba3fb8da261b

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b6b59d7d915