osfile_unix_back.jsm wrongly assumes that stat() is macro when _STAT_VER is defined

UNCONFIRMED
Unassigned

Status

()

Toolkit
OS.File
UNCONFIRMED
4 years ago
4 years ago

People

(Reporter: Natanael Copa, Unassigned)

Tracking

26 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131211162850

Steps to reproduce:

Built firefox-19 (and later?) on Alpine Linux which uses uClibc.


Actual results:

Back button stopped working. Restore Previous Session stopped working. (see bug #843513 for more details).

The restore of previous session broke again with firefox-26. (see bug #949375)

I decided to clean up the fix and report it as requested on bug 843513#c12


What happens is that if _STAT_VER is defined, the assumption is that stat/lstat/fstat are macros.
http://hg.mozilla.org/mozilla-central/file/1de977085578/toolkit/components/osfile/modules/osfile_unix_back.jsm#l505

This is not true on uClibc and stat function binding breaks.


Expected results:

The code should use #if defined(stat) to test if stat is a macro or not.
(Reporter)

Updated

4 years ago
Summary: xstat_name wrongly assumes that stat() is macro when _STAT_VER is defined → osfile_unix_back.jsm wrongly assumes that stat() is macro when _STAT_VER is defined
(Reporter)

Comment 1

4 years ago
Created attachment 8346535 [details] [diff] [review]
detect-if-stat-is-macro.patch

The patch tests if stat/fstat/lstat are macros and set a constant, OSFILE_STAT_MACROS to indicate this.

This also lets us handle NetBSD's __stat50 like we handle glibc's __xstat.
(Reporter)

Updated

4 years ago
Attachment #8346535 - Flags: review?(dteller)
Comment on attachment 8346535 [details] [diff] [review]
detect-if-stat-is-macro.patch

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

::: toolkit/components/osfile/modules/osfile_unix_back.jsm
@@ +521,5 @@
>             fxstat_name = "_fxstat";
> +         } else if (OS.Constants.Sys.Name == "NetBSD") {
> +	   xstat_name = "__stat50";
> +           lxstat_name = "__lstat50";
> +           fxstat_name = "__fstat50";

I'm almost sure that __stat50 & co. only take two arguments, while __xstat & co. take three. So this patch probably won't work.
Attachment #8346535 - Flags: review?(dteller)
(Reporter)

Comment 3

4 years ago
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #2)
 
> I'm almost sure that __stat50 & co. only take two arguments, while __xstat &
> co. take three. So this patch probably won't work.

You are of course right about that. I will make new patch. Sorry.
(Reporter)

Comment 4

4 years ago
Created attachment 8347186 [details] [diff] [review]
detect-if-stat-is-macro.patch

Updated patch. I think the nested if/else is a bit ugly. Maybe should make a small declareStat() helper function?
Attachment #8346535 - Attachment is obsolete: true
(Reporter)

Comment 5

4 years ago
(In reply to Natanael Copa from comment #4)
> Created attachment 8347186 [details] [diff] [review]
> detect-if-stat-is-macro.patch
> 
> Updated patch. I think the nested if/else is a bit ugly. Maybe should make a
> small declareStat() helper function?

I just realized this patch wont work either. ver is undefined. I'll clean it up...
(Reporter)

Comment 6

4 years ago
Created attachment 8347203 [details] [diff] [review]
detect-if-stat-is-macro.patch

I believe this should work.

Note that it removes more lines than it adds ;)
Attachment #8347186 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.