Closed Bug 1284167 Opened 3 years ago Closed 3 years ago

Don't poke /proc on non-Linux systems

Categories

(Toolkit :: Telemetry, defect, P4)

All
FreeBSD
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

Attachments

(1 file)

While it's possible to retrieve some information about running CPU on BSD kernels via dmesg(8) or handpicking sysctls on x86 one is supposed to use CPUID instruction instead.

On FreeBSD there're procfs(5) and linprocfs(5). Only the latter provides "cpuinfo" file usually under /compat/linux/proc. linprocfs(5) is only available on x86 architectures and doesn't know about anthing more recent than SSE2.
https://reviewboard.mozilla.org/r/61990/#review58870

::: xpcom/base/nsSystemInfo.cpp:37
(Diff revision 1)
>  #ifdef MOZ_WIDGET_GTK
>  #include <gtk/gtk.h>
>  #include <dlfcn.h>
> +#endif
> +
> +#ifdef XP_LINUX

To not change things on Android, this can just be nested inside of #ifdef MOZ_WIDGET_GTK, i.e.,

#ifdef MOZ_WIDGET_GTK
...
#ifdef XP_LINUX
...
#endif
#endif

::: xpcom/base/nsSystemInfo.cpp:79
(Diff revision 1)
>  // system call to discover the appropriate value is not thread-safe,
>  // so we must call it before going multithreaded, but nsSystemInfo::Init
>  // only happens well after that point.
>  uint32_t nsSystemInfo::gUserUmask = 0;
>  
> -#if defined (MOZ_WIDGET_GTK)
> +#if defined (XP_LINUX)

We don't want this for Android, so the way to maintain the original intent and exclude something like FreeBSD, this should be:

#if defined (MOZ_WIDGET_GTK) && defined (XP_LINUX)

instead.

::: xpcom/base/nsSystemInfo.cpp:503
(Diff revision 1)
>    if (!sysctlbyname("machdep.cpu.stepping", &sysctlValue32, &len, NULL, 0)) {
>      cpuStepping = static_cast<int>(sysctlValue32);
>    }
>    MOZ_ASSERT(sizeof(sysctlValue32) == len);
>  
> -#elif defined (MOZ_WIDGET_GTK)
> +#elif defined (XP_LINUX)

Same as the above, we want:

#elif defined (MOZ_WIDGET_GTK) && defined (XP_LINUX)

instead.
https://reviewboard.mozilla.org/r/61990/#review58870

> To not change things on Android, this can just be nested inside of #ifdef MOZ_WIDGET_GTK, i.e.,
> 
> #ifdef MOZ_WIDGET_GTK
> ...
> #ifdef XP_LINUX
> ...
> #endif
> #endif

But it's a platform-specific, not widget-specific code. I think, on Desktop it may work even with MOZ_WIDGET_QT.
Comment on attachment 8767540 [details]
Bug 1284167 - /proc is unreliable on non-Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61990/diff/1-2/
Attachment #8767540 - Flags: review?(milan) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/cec1ab33c0d6
/proc is unreliable on non-Linux. r=milan
Keywords: checkin-needed
Assignee: nobody → jbeich
Priority: -- → P4
https://hg.mozilla.org/mozilla-central/rev/cec1ab33c0d6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.