Crash reporter fails if your Firefox profile is in a volume other than /home

RESOLVED FIXED

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dholbert, Assigned: timeless)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Steps to reproduce:
 1. Run firefox with "-profile [some_empty_directory]" switch.
      e.g. mkdir /tmp/foo; firefox -profile /tmp/foo -no-remote
 2. Click "Cancel" on the EULA popup, or press Esc.

ACTUAL RESULT: This dialog appears:
 Title: Mozilla Crash Reporter
 Content: Firefox had a problem and crashed. We'll try to restore your tabs and windows when it restarts. Unfortunately the crash reporter is unable to submit a crash report. Details: Couldn't move crash dump.

NOTE: The STR populates the (initially empty) profile directory with some data, which seems to prevent subsequent crashes if I repeat the STR using the same profile directory.  (If I delete that directory and repeat the STR, I get the crash again.)

NOTE: I don't get this crash using Firefox 3.0.1 from getfirefox.com.  However, I do get this in today's 3.1b1pre nightly:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080915020438 Minefield/3.1b1pre
(Assignee)

Comment 2

10 years ago
there are really two bugs here, i'll use this for the fact that the crash reporter failed to submit the report, once that's fixed, you can file a new bug about the firefox crash :)
Component: General → Breakpad Integration
Product: Firefox → Toolkit
QA Contact: general → breakpad.integration
So that's failing here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp#275

called from here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp#537
gSettingsPath should be your profile directory, although in this case perhaps something is broken? Does ~/.mozilla/firefox/ exist and is it writeable? is there a Crash Reports directory in there?
(In reply to comment #3)
> Does ~/.mozilla/firefox/ exist and is it writeable? is
> there a Crash Reports directory in there?
Yes, yes, and yes.

Just in case something was screwy, though, I moved my .mozilla folder elsewhere and let Firefox build it on-the-fly, and I still get the issue.

But, I figured something out -- it looks like the crash reporter fails whenever I put my profile in a directory located on a different partition from /home. (like "/tmp/foo" in comment 0 -- I have /home as its own partition)

To test this theory, I tried creating a parent directory (owned by me), placing it in various locations, and running firefox with a freshly-created subdirectory inside of this directory.
e.g.
  mkdir parent
  sudo mv parent $LOCATION/
  cd $LOCATION/tmp2
  mkdir foo; $NIGHTLY/firefox -profile foo -no-remote
  rm -fr foo

Here, $NIGHTY is yesterday's nightly, with timestamp 20080915020438.  And I tried these options for $LOCATION:
 /                          Crash reporter fails
 /tmp                       Crash reporter fails
 /mnt                       Crash reporter fails
 /mnt/nested/test/          Crash reporter fails
 /home                      Crash reporter succeeds
 /home/dholbert             Crash reporter succeeds
 /home/dholbert/Desktop     Crash reporter succeeds
 /home/dholbert/flashdrive  Crash reporter fails

In the last example, I've mounted a flash drive in /home/dholbert/flashdrive, to test whether it's the path that counts (e.g. are all /home subdirectories ok?) or whether it's the partition.  Apparently, it's the partition.

Perhaps this is due to a race condition that depends on a speedy file transfer from the profile directory into ~/.mozilla/firefox/Crash Reports...?  Such a file transfer would be effectively instantaneous if the two directories are on the same partition, but it'd a bit more time if they're on different partitions.
Summary: Crash & failed crash-report when canceling EULA w/ empty profile → Crash & failed crash-report when canceling EULA w/ empty profile on a different volume from /home
(In reply to comment #4)
>   sudo mv parent $LOCATION/
>   cd $LOCATION/tmp2

oops -- I meant "cd $LOCATION/parent" in the second line there.
(In reply to comment #2)
> you can file a new bug
> about the firefox crash :)

Filed bug 455617 on the crash itself.
Thanks for the analysis! So we're using the POSIX rename function. Can you compile and run the following program:

#include <stdio.h>
#include <errno.h>

int main(int argc, char** argv)
{
  if (argc == 3) {
    if (rename(argv[1], argv[2]) != -1) {
      printf("renamed ok!\n");
    }
    else {
      printf("errno: %d\n", errno);
    }
  }
  return 0;
}

Pass it an existing filename on one partition, and then another filename on a separate partition, and figure out what errno is? (symbolic value would be better than the decimal value, if you can ferret that out of a header file)
Thanks for the handy test program!  I get:
  errno: 18
which is apparently "EXDEV", described on a man page for rename here:
  http://linux.die.net/man/2/rename

I quote: "EXDEV [...] oldpath and newpath are not on the same mounted filesystem. [...] rename(2) does not work across different mount points"

D'oh.
Summary: Crash & failed crash-report when canceling EULA w/ empty profile on a different volume from /home → Crash reporter fails if your Firefox profile is in a mountpoint other than /home
Summary: Crash reporter fails if your Firefox profile is in a mountpoint other than /home → Crash reporter fails if your Firefox profile is in a volume other than /home
Thanks for that, that's sort of what I thought was going to happen. After perusing the source for `mv` (ugh), it looks like the way to rename a file across devices is to open both files, write the contents of the source into the destination, and then unlink the source. That sounds pretty awful, but clearly we ought to do it. I'll take a patch!
An alternate solution might be to just shell out to `mv`. Would be a lot less code.
(Assignee)

Updated

9 years ago
OS: Linux → All
(Assignee)

Comment 11

9 years ago
Created attachment 392222 [details] [diff] [review]
untested

this compiles on OS X, i don't want to try to test it, although I do have a number of interesting volumes (zfs, hfs(filevault)), but to test it, i'd have to um... get a working browser....
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #392222 - Flags: review?(ted.mielczarek)
I feel like there's probably a cleverer way to do this on OS X, especially since that's in an ObjC++ file, so you can use Cocoa.
Comment on attachment 392222 [details] [diff] [review]
untested

The Linux bits look ok, although I think you're missing a waitpid in the parent process branch of the fork. (I wish there was a combo fork+exec+waitpid function in the system library. I guess system(3) does that, but you can only pass the whole commandline as a string.)

I'd rather not take the mac bits unless you want to fix it in a Cocoa-y way. Mac users are much less likely to hit this anyway, since I don't know anyone that repartitions their Mac, and they come from the factory in a configuration that won't hit this. It's up to you, though.
Attachment #392222 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 15

9 years ago
Created attachment 393589 [details] [diff] [review]
waitpid, and osx impl

i don't have a linux env handy, so it isn't compile tested. the mac version compiles.
Attachment #392222 - Attachment is obsolete: true
Attachment #393589 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 16

9 years ago
Created attachment 393686 [details] [diff] [review]
compiles on linux, works on osx

so, i got a try server build for os x and copied my crash reporter into my 3.5.2 build and it worked (home=zfs, profile=tmp=/=hfs).

this patch has one change for linux - it builds, but i can't find someone to test it :/
Attachment #393589 - Attachment is obsolete: true
Attachment #393686 - Flags: review?(ted.mielczarek)
Attachment #393589 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 17

9 years ago
Created attachment 393695 [details] [diff] [review]
works on osx and linux

thanks to dholbert for testing,... last stupid bug is gone. reading man pages is important :(
Attachment #393686 - Attachment is obsolete: true
Attachment #393695 - Flags: review?(ted.mielczarek)
Attachment #393686 - Flags: review?(ted.mielczarek)
Comment on attachment 393695 [details] [diff] [review]
works on osx and linux

+  /* use system /bin/mv instead, time to fork */

Nit: single-line comments in this file all use //

+    if (args[0] && args[1])
+      execve("/bin/mv", args, 0);
+    if (args[0])
+      free(args[0]);
+    if (args[1])
+      free(args[1]);

Don't you mean args[1] and args[2] here?

+  NSString *source = [[NSString alloc] initWithCString:file.c_str() encoding:[NSString defaultCStringEncoding]];
+  NSString *dest = [[NSString alloc] initWithCString:newfile.c_str() encoding:[NSString defaultCStringEncoding]];

It's my understanding that if you call |alloc| on something in ObjC, you later need to call |release| explicitly. I think you can avoid this by using [NSFileManager 
stringWithFileSystemRepresentation:str length:strlen(str)], which returns a NSString*.

http://developer.apple.com/documentation/Cocoa/Reference/Foundation/Classes/nsfilemanager_Class/Reference/Reference.html#//apple_ref/occ/instm/NSFileManager/stringWithFileSystemRepresentation:length:
Attachment #393695 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 19

9 years ago
Created attachment 393830 [details] [diff] [review]
fewer bugs
Attachment #393695 - Attachment is obsolete: true
Attachment #393830 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 20

9 years ago
Created attachment 393836 [details] [diff] [review]
slightly more caution
Attachment #393830 - Attachment is obsolete: true
Attachment #393836 - Flags: review?(ted.mielczarek)
Attachment #393830 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 21

9 years ago
Created attachment 393847 [details] [diff] [review]
qref'd properly...

i clearly need sleep
Attachment #338730 - Attachment is obsolete: true
Attachment #393836 - Attachment is obsolete: true
Attachment #393847 - Flags: review?(ted.mielczarek)
Attachment #393836 - Flags: review?(ted.mielczarek)
Comment on attachment 393847 [details] [diff] [review]
qref'd properly...

Looks good, thanks!
Attachment #393847 - Flags: review?(ted.mielczarek) → review+
Keywords: crash → checkin-needed
Landed: http://hg.mozilla.org/mozilla-central/rev/ad96557d450c
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Comment on attachment 393847 [details] [diff] [review]
qref'd properly...

Would like to get this on the branches. It makes the crash reporter go from non-functional to functional for Linux users that have their disk partitioned in this way, which is not uncommon. No tests because it's hard to write a test that requires a specific disk partitioning.
Attachment #393847 - Flags: approval1.9.2?

Updated

9 years ago
Attachment #393847 - Flags: approval1.9.2? → approval1.9.2+
Don't forget who the assignee is, and what that means for getting it pushed, when you get your branch approval...
Keywords: checkin-needed
Whiteboard: [c-n: 1.9.2-branch]
pushed to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0a786da1aee7
status1.9.2: --- → final-fixed
Keywords: checkin-needed
Whiteboard: [c-n: 1.9.2-branch]
You need to log in before you can comment on or make changes to this bug.