861829 - Firefox 20.0.1 on Solaris doesn't restore tabs on startup

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[brent.paulson]

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.

Comment 1

[Ginn Chen]

Looks like a regression of Bug 532150.

Something doesn't work on Solaris.

Comment 2

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

Brent, is this the only problem with Firefox 20.0.1 on Solaris? In particular, does the search bar work?

Comment 3

[Ginn Chen]

If I type something in localtion bar, it searches fine.
History also works fine.

Comment 4

[Ginn Chen]

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.

Comment 5

[Ginn Chen]

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


Also on Solaris dirent doesn't have d_type.

Comment 6

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

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.

Comment 7

[Ginn Chen]

I'm working on it.

Comment 8

[Ginn Chen]

Attachment: patch

Solaris port

Comment 9

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

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.

Comment 10

[Ginn Chen]

Attachment: patch v2

I agree "feature detection" is better.

Comment 11

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

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 {

Comment 12

[Ginn Chen]

Attachment: 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?

Comment 13

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

(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 14

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

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

Comment 15

[Ginn Chen]

Attachment: 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.

Comment 16

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

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.

Comment 17

[Ginn Chen]

(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.

Comment 18

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

(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?

Comment 19

[Ginn Chen]

Attachment: patch v5

fixed another typo
now it passed tryserver
https://tbpl.mozilla.org/?tree=Try&rev=a335045dc7fc

Comment 20

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

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.

Comment 21

[Ginn Chen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef3b7592a53

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ef3b7592a53