Firefox 20.0.1 on Solaris doesn't restore tabs on startup

RESOLVED FIXED in mozilla23

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: brent.paulson, Assigned: Ginn Chen)

Tracking

20 Branch
mozilla23
x86
Solaris
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; SunOS i86pc; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130308163327

Steps to reproduce:

When using Firefox 20.0.1 for Solaris from the following URL:

http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest/contrib/solaris_tarball/

any open tabs in the browser will be lost when the browser is exited and
started again later.  This doesn't occur on Firefox 19.0.2, 19.0.1,
Firefox 18.0, or Firefox 17.0 which were previously downloaded from the
above URL and which I still have copies of on this Solaris system.

The problem is 100% reproducible using these simple steps:

01. Start Firefox 20.0.1
02. Open three tabs with content in each, say cnn.com, news.bbc.co.uk,
    and apple.com.
03. Exit Firefox 20.0.1 (File->Quit)
04. Start Firefox 20.0.1 again.

Expected result:

See all three tabs with their respective content.

Actual result:

A single, empty tab.

I've confirmed the problem exists with Firefox 20.0.1 using 'firefox
-safe-mode' as well.  Reverting to Firefox 19.0.2 allows things to work
as expected.
(Assignee)

Comment 1

5 years ago
Looks like a regression of Bug 532150.

Something doesn't work on Solaris.
Assignee: nobody → ginn.chen
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → Session Restore
Ever confirmed: true
Brent, is this the only problem with Firefox 20.0.1 on Solaris? In particular, does the search bar work?
Flags: needinfo?(brent.paulson)
(Assignee)

Comment 3

5 years ago
If I type something in localtion bar, it searches fine.
History also works fine.
Flags: needinfo?(brent.paulson)
(Assignee)

Comment 4

5 years ago
It looks like the problem is in reading sessionstore.js, not writing.

If I open several tabs in Firefox 20 close it and open Firefox 19.0.2, I can restore tabs without a problem.
(Assignee)

Comment 5

5 years ago
Solaris ABI is
_xstat, _lxstat, _fxstat
not 
__xstat, __lxstat, __fxstat (Linux)


Also on Solaris dirent doesn't have d_type.
Ah, that might very well be the issue.
Are you working on a patch?

If so, you'll need to fix around the following line:
http://dxr.mozilla.org/mozilla-central/toolkit/components/osfile/osfile_unix_back.jsm#l483

Use OS.Constants.Sys.Name to detect Solaris.
(Assignee)

Comment 7

5 years ago
I'm working on it.
(Assignee)

Comment 8

5 years ago
Created attachment 738354 [details] [diff] [review]
patch

Solaris port
Attachment #738354 - Flags: review?(dteller)
(Assignee)

Updated

5 years ago
Component: Session Restore → OS.File
Product: Firefox → Toolkit
Comment on attachment 738354 [details] [diff] [review]
patch

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

Looks good, but let's go a little bit further and perform feature detection instead of platform detection. With any luck, this will also solve issues for other platforms.

::: toolkit/components/osfile/osfile_unix_back.jsm
@@ +117,5 @@
>         //    char[...]   d_name;
>         //    };
>         {
> +         let dirent;
> +         if (OS.Constants.Sys.Name == "SunOS") {

Rather than detecting the OS name, it would be more generic to check whether |OS.Constants.libc.OSFILE_OFFSETOF_DIRENT_D_TYPE| is |undefined|.

@@ +120,5 @@
> +         let dirent;
> +         if (OS.Constants.Sys.Name == "SunOS") {
> +           // On Solaris, |dirent| doesn't have d_type,
> +           // and d_name is defined as "char d_name[1];".
> +           let name_max = 256;

Doesn't |OS.Constants.libc.OSFILE_SIZEOF_DIRENT_D_NAME| provide an acceptable bound?

@@ +515,5 @@
>                       );
>         } else if (OS.Constants.libc._STAT_VER != undefined) {
>           const ver = OS.Constants.libc._STAT_VER;
> +         let xstat_name, lxstat_name, fxstat_name
> +         if (OS.Constants.Sys.Name == "SunOS") {

In this case, this is probably unescapable. Sigh. The endless wonders of FFI.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +636,5 @@
>              entry = UnixFile.readdir(this._dir)) {
>           let contents = entry.contents;
> +         let isDir;
> +         if (OS.Constants.Sys.Name == "SunOS") {
> +           // On Solaris, |dirent| doesn't have d_type

You should rather perform feature detection:
  if ("d_type" in contents) { ... }

@@ +643,5 @@
> +           isDir = (gStatData.st_mode & OS.Constants.libc.S_IFMT) == OS.Constants.libc.S_IFDIR;
> +         }
> +         else {
> +           isDir = contents.d_type == OS.Constants.libc.DT_DIR;
> +         }  

Nit: whitespace

@@ +696,5 @@
>         let name = unix_entry.d_name.readString();
>         this._parent = parent;
>         let path = OS.Unix.Path.join(this._parent, name);
> +       if (OS.Constants.Sys.Name == "SunOS") {
> +         // On Solaris, |dirent| doesn't have d_type

As above, feature detection rules.
Attachment #738354 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 10

5 years ago
Created attachment 738593 [details] [diff] [review]
patch v2

I agree "feature detection" is better.
Attachment #738354 - Attachment is obsolete: true
Attachment #738593 - Flags: review?(dteller)
Comment on attachment 738593 [details] [diff] [review]
patch v2

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

::: toolkit/components/osfile/osfile_unix_back.jsm
@@ +117,5 @@
>         //    char[...]   d_name;
>         //    };
>         {
> +         let d_name_extra_size = 0;
> +         if (OS.Constants.libc.OSFILE_SIZEOF_DIRENT_D_NAME < 8) {

Nit: "on some platforms (e.g. Solaris)"

@@ +127,2 @@
>           let dirent = new OS.Shared.HollowStructure("dirent",
> +           OS.Constants.libc.OSFILE_SIZEOF_DIRENT + d_name_extra_size);

That sounds a little surprising. The structure is larger than its |sizeof|?

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +635,5 @@
>              entry != null && !entry.isNull();
>              entry = UnixFile.readdir(this._dir)) {
>           let contents = entry.contents;
> +         let isDir;
> +         if ("d_type" in contents) {

Looks like a forgotten negation.

@@ +636,5 @@
>              entry = UnixFile.readdir(this._dir)) {
>           let contents = entry.contents;
> +         let isDir;
> +         if ("d_type" in contents) {
> +           // |dirent| doesn't have d_type

Nit: "e.g. Solaris"

@@ +638,5 @@
> +         let isDir;
> +         if ("d_type" in contents) {
> +           // |dirent| doesn't have d_type
> +           let dir_fd = UnixFile.dirfd(this._dir);
> +           throw_on_negative("stat", UnixFile.fstat(dir_fd, gStatDataPtr));

It's a shame we need this, as |stat| is quite expensive. I don't think we can do any better, though.

However, we want to avoid calling |stat| twice in a row. You should find a way to share the result.

@@ +641,5 @@
> +           let dir_fd = UnixFile.dirfd(this._dir);
> +           throw_on_negative("stat", UnixFile.fstat(dir_fd, gStatDataPtr));
> +           isDir = (gStatData.st_mode & OS.Constants.libc.S_IFMT) == OS.Constants.libc.S_IFDIR;
> +         }
> +         else {

Nit: in this file, } else {
Attachment #738593 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 12

5 years ago
Created attachment 738898 [details] [diff] [review]
patch v3

I moved some code in osfile_unix_front.jsm to avoid twice stat().

> > +         let isDir;
> > +         if ("d_type" in contents) {
> 
> Looks like a forgotten negation.

Thanks for catching this.
I was sleepy at that time.
mochitest-chrome doesn't catch that.

> @@ +127,2 @@
> >           let dirent = new OS.Shared.HollowStructure("dirent",
> > +           OS.Constants.libc.OSFILE_SIZEOF_DIRENT + d_name_extra_size);
> 
> That sounds a little surprising. The structure is larger than its |sizeof|?

That's a normal thing in C world.
See http://pubs.opengroup.org/onlinepubs/009695399/basedefs/dirent.h.html

Thus, struct dirent should be malloc-ed as

           len = offsetof(struct dirent, d_name) + name_max + 1;
           buffer = malloc(len);

I don't think we need to be accurate in js, do we?
Attachment #738593 - Attachment is obsolete: true
Attachment #738898 - Flags: review?(dteller)
(In reply to Ginn Chen from comment #12)
> Created attachment 738898 [details] [diff] [review]
> patch v3
> 
> I moved some code in osfile_unix_front.jsm to avoid twice stat().
> 
> > > +         let isDir;
> > > +         if ("d_type" in contents) {
> > 
> > Looks like a forgotten negation.
> 
> Thanks for catching this.
> I was sleepy at that time.
> mochitest-chrome doesn't catch that.

Ah, indeed, I just checked main_osfile_async.js and it doesn't seem to test |isDir| for directory iterators.

Would you mind adding a test?

This would entail modifying main_osfile_async.js, function |test_iter|, and doing the following:
- create a directory A in directory OS.Constants.Path.tmpDir;
- create a subdirectory B in A;
- create an empty file C in A;
- iterate through A;
- ensure that the entry for B has isDir set to true and isSymlink set to false;
- ensure that the entry for C has isDir set to false and isSymlink set to false.


> > @@ +127,2 @@
> > >           let dirent = new OS.Shared.HollowStructure("dirent",
> > > +           OS.Constants.libc.OSFILE_SIZEOF_DIRENT + d_name_extra_size);
> > 
> > That sounds a little surprising. The structure is larger than its |sizeof|?
> 
> That's a normal thing in C world.
> See http://pubs.opengroup.org/onlinepubs/009695399/basedefs/dirent.h.html
> 
> Thus, struct dirent should be malloc-ed as
> 
>            len = offsetof(struct dirent, d_name) + name_max + 1;
>            buffer = malloc(len);

Ouch. That kind of makes sense, since the API never requires clients to allocate instances of |struct dirent|, but I'm not a big fan of the practice :)

> I don't think we need to be accurate in js, do we?

No, we shouldn't need to. JS does not manage that memory at all, so it's ok.
Comment on attachment 738898 [details] [diff] [review]
patch v3

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

r=me, but I would like the additional test mentioned in the previous comment
Attachment #738898 - Flags: review?(dteller) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 740077 [details] [diff] [review]
patch v4

The previous patch has 2 bugs:
1) UnixFile.dirfd(this._dir); is fd for dirp, not entries.
So stat will always get the same result.
2) There is a typo
OS.Constants.S_IFLNK; should be OS.Constants.libc.S_IFLNK;
I copied this from File.Info(stat).

That's why unit test is important.

I just add the test in test iterator.
The directory should have dirs and symbol links.

I also fixed the typo in File.Info(stat), however I didn't find a way to pass unixNoFollowingLinks, so I can't test it.
Attachment #738898 - Attachment is obsolete: true
Attachment #740077 - Flags: review?(dteller)
Comment on attachment 740077 [details] [diff] [review]
patch v4

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

Good catch.
I'd like to extend the test a little bit, and then we'll be good.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +647,5 @@
> +      var f = new FileUtils.File(entry.path);
> +      test.is(entry.isDir, f.isDirectory(), "Get file " + entry.path + " isDir correctly");
> +      if ("isSymLink" in entry) {
> +        test.is(entry.isSymLink, f.isSymlink(), "Get file " + entry.path + " isSymLink correctly");
> +      }

Why not |test.is(entry.isSymlink, f.isSymlink(), ...)|, as the check on the previous line?

Also, you should ensure that the directory your enumerate contains at least one sub-directory and one regular file. We do not have any manner of creating symlinks yet (and chances are that we never will, given that Windows symlinks are unreliable), so that part of the check will not be sufficient to ensure that isSymLink works, but at least it will ensure that some code paths don't break.
Attachment #740077 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 17

5 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #16)
> Why not |test.is(entry.isSymlink, f.isSymlink(), ...)|, as the check on the
> previous line?

I thought isSymLink or isSymlink might not be available on all platforms.
It looks like it's available on UNIX and Win, so the check can be removed.

> 
> Also, you should ensure that the directory your enumerate contains at least
> one sub-directory and one regular file. We do not have any manner of
> creating symlinks yet (and chances are that we never will, given that
> Windows symlinks are unreliable), so that part of the check will not be
> sufficient to ensure that isSymLink works, but at least it will ensure that
> some code paths don't break.

The directory it tries to enumerate is $objdir/_tests/testing/mochitest/
It should have a few sub-directory, regular files and symlinks.
(In reply to Ginn Chen from comment #17)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #16)
> > Why not |test.is(entry.isSymlink, f.isSymlink(), ...)|, as the check on the
> > previous line?
> 
> I thought isSymLink or isSymlink might not be available on all platforms.
> It looks like it's available on UNIX and Win, so the check can be removed.
> 
> > 
> > Also, you should ensure that the directory your enumerate contains at least
> > one sub-directory and one regular file. We do not have any manner of
> > creating symlinks yet (and chances are that we never will, given that
> > Windows symlinks are unreliable), so that part of the check will not be
> > sufficient to ensure that isSymLink works, but at least it will ensure that
> > some code paths don't break.
> 
> The directory it tries to enumerate is $objdir/_tests/testing/mochitest/
> It should have a few sub-directory, regular files and symlinks.

I would have created a temporary directory for that purpose, but that's not a big deal. However, could you please document your assumption in the test?
(Assignee)

Comment 19

5 years ago
Created attachment 740786 [details] [diff] [review]
patch v5

fixed another typo
now it passed tryserver
https://tbpl.mozilla.org/?tree=Try&rev=a335045dc7fc
Attachment #740077 - Attachment is obsolete: true
Attachment #740786 - Flags: review?(dteller)
Comment on attachment 740786 [details] [diff] [review]
patch v5

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

r=me, thank you very much for that patch.
Attachment #740786 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/3ef3b7592a53
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.