265903 - Camino crashes when starting a download if download folder not specified in IC preferences (e.g. a fresh user account)

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Nathaniel Madura]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041024 Camino/0.8+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041024 Camino/0.8+

Using nightlies (this bug does not affect 0.8.1), on any client with a fresh
install (including new users on machine with an exisint install) Camino crashes
when attempting to download anything.

Reproducible: Always
Steps to Reproduce:
1. Create a new user account
2. Login to new user account
3. Launch Camino
4. Download something
Actual Results:  
Camino crashed

Expected Results:  
File should have downloaded

Comment 1

[Jasper]

WFM using trunk 2004102408 (v0.8+) NB on 10.3.5

I have tested all the given situations and all works fine here.
All I can say is perhapos you should backup your profile/support folder and
trash the orig one. Launch Camino, quit it. And move your bookmarks,cookies and
history file back to the new profile/support folder.

Comment 2

[Nathaniel Madura]

Attachment: Log From TalkBack

I installed Camino onto a brand new machine and using the user that installed
Camino I did not experience a crash, but creating a new user and launching
Camino caused a crash when ever I tried to download anything. I tried removing
the camino folder in ~/Library/Application Support, that didn't affect
anything. It crashes everytime I download something with that newer user
account. The attached file is from the crash reporter.

Comment 3

[Jasper]

Nathan what are you downloading and to what webiste do you go, becuase I
honestly do not see any such behaviour. And nobody on the forum or mailing list
has reported such an issue.

Comment 4

[Mike Pinkerton (not reading bugmail)]

can you post a crash from either the crashlog or copied from the OS crash
reporter (not Talkback)? The talkback info by itself is not human readable.

thanks!

Comment 5

[Nathaniel Madura]

Attachment: Log From CrashReporter

Attached is a copy of the log as spit out from the OS X CrashReporter... looks
like the data contained in the Crash report is actually from three crashes on
two days (I believe all three are from trying to download a file) and on the
two different days were using (I believe) the latest nightlies at the time... I
definitely downloaded the nightly tonight which caused the crash today (10/26).

Comment 6

[Nathaniel Madura]

Oops, I was also requested to submit a page that caused the crash, (I believe it
is _any_ file that I try to download) however, typically my test case is the
default start page ( http://www.mozilla.org/projects/camino/homepage.html ) and
I usually just download from the link that says 0.81

Comment 7

[Hiro]

The crashing problem is reported when "fast user switching" is used by firefox.
(bug266138)
Is login performed from the login panel?

Comment 8

[Nathaniel Madura]

Fast user switching is not being used, it isn't even enabled. A full "Log out"
is preformed, and then the new user is selected from the dialog box and "Logged
in." This is true of the other installs I have tried this with.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Crash log (3 crashes of 20041029 0.8+ on new user acct)

I can now reproduce this with 20041029 0.8+ (I was unable to do so on earlier
nightlies when I tried).  Here is a crash log (3 separate tries, the first two
on one login and the third on a second login to that user but having deleted
the profile before starting Camino).  In case it's useful, the corresponding
TalkBack incidents are TB1610495H, TB1610528E, and TB1610941Q.

Comment 10

[Mike Pinkerton (not reading bugmail)]

hrm, it's crashing in internet config. my guess is that your IC prefs are
corrupted somehow. we had a bug like this a while back but never could get it to
repro.

if you move ~/Library/Preferences/com.apple.internetconfig.plist to the desktop,
log out and back in (just to be safe) and try camino again, do you get the same
crashes?

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Crash log after moving Internet Config prefs to desktop

(In reply to comment #10)

> if you move ~/Library/Preferences/com.apple.internetconfig.plist to the
desktop, log out and back in (just to be safe) and try camino again, do you get
the same crashes?

Unfortunately, it still crashes.  No new
~/Library/Preferences/com.apple.internetconfig.plist seems to be created,
either, if that's relevant.  2004103108 (v0.8+) 10.3.5

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Figured this one out thanks to bug 218271 comment 16.  Confirming with 0.8.2 and
 2004120608 (v0.8+) on 10.3.6.

Mike was right, the IC prefs were "corrupted."  The key here is that the "stock"
internetconfig.plist (created when a fresh user account is created)--or a
missing internetconfig.plist, obviously--is missing the entry for the downloads
directory!

It seems that simply *opening* Safari prefs (which have the downloads dir
settings in the first panel) causes this entry to be created in the IC plist,
and Camino no longer crashes when downloading on a fresh user account, which
probably explains why it's fairly rare.  This probably needs to be addressed
when implementing bug 218271 (if user can set Camino as default from Camino, no
reason to open Safari and thus he/she won't pick up the IC download dir setting.

I'll attach three IC plists: the stock one from my new user, Safari's editing of
the stock profile (via opening Safari prefs), and the plist created by opening
Safari's prefs when there is no IC plist file at all.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Original plist created when new user account created

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Plist showing how opening Safari's prefs modifies the stock plist

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: plist created by opening Safari's prefs when no IC plist exists

Comment 16

[Mike Pinkerton (not reading bugmail)]

nice catch, we'll keep this in mind for sure!

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Crash log from attempt to open Navigation prefs with IC plist missing downloads dir

More info (perhaps a separate bug, but the root cause seems to me to be the
same):

I remembered that you can set the downloads directory in
Camino...unfortunately, in this case you can't.  If the downloads dir entry is
missing from the IC prefs (or the IC prefs is missing), Camino crashes when
opening the Navigation pane (which contains the Downloads tab), and only the
Navigation pane, of the Camino prefs.  Once Safari has added that field, the
crash goes away.

I've confirmed this behavior on both 0.8.2 and 2004120608 (v0.8+) on 10.3.6.

Comment 18

[Julian Pellico]

This might be a fun little thing to look at

Comment 19

[Bruce Davidson]

Updating summary to reflect the results of Smokey's investigation.

Comment 20

[Bruce Davidson]

The crash is definitely happening in Apple's code. Control never returns from
::ICGetPref(). I can reproduce the same crash in Colloquy (which uses similar
code to us) by trying to open its file transfer pref pane from a fresh user
account. However it manages to avoid crashing the whole app - you just can't
access that one pref pane.

I've tried detecting the error by using ::ICFindPrefHandle, but this too
crashes. I can't find any reference to the bug on the internet, but maybe
someone (Mike?) should raise it with Apple.

I'm going to have a look at changing nsDirectoryService.cpp so that it loops
over all the IC preferences and checks to see if each one is the download
directory, to avoid having to ask for it and causing the crash.

Comment 21

[Bruce Davidson]

Attachment: Proposed patch

This patch makes nsDirectoryService loop through all the IC prefs to see if the
download directory has been set, and only gets the pref it it exists. This
avoids the crash in Apple's code.

Although there are ~200 keys on my system, don't expect the performance
implications to be significant, we only call this code when we display the
download pref pane or when we start a download.

Comment 22

[Mike Pinkerton (not reading bugmail)]

as an aside, colloquy is open source. how do they handle this?

Comment 23

[Bruce Davidson]

An alternate approach, suggested by smfr, to call ICGetPref but without the
buffer, just to query if the pref exists still crashes out in the same place. I
suggest that we go ahead with the current patch.

For the record, the answer to pink's question is that Colloquy don't handle it
either, they crash too (as of 2.0 (v2C11))!

Comment 24

[Simon Fraser [no longer active]]

Comment on attachment 170032 [details] [diff] [review]
Proposed patch

Please remove tabs from these changes

>Index: xpcom/io/nsDirectoryService.cpp
>===================================================================

>+                // ICGetPref() crashes when getting the download directory if the download
>+                // directory has never been specified (e.g. a new user account), bug 265903.
>+                // To work around this we enumerate through the IC prefs to see if the
>+                // download directory has been specified before trying to obtain it.
>+            	long numPrefs = 0;
>+            	err = ::ICCountPref(icInstance, &numPrefs);
>+            	if (err == noErr)
>+            	{
>+            		Str255 key;

Put 'key' inside the loop (the most local scope).

>+            		for ( long i = 0; i < numPrefs; ++i )
>+            		{

Fix those and sr=sfraser

Comment 25

[Bruce Davidson]

Attachment: Patch addressing review comments

Patch simply fixes whitespace and addresses the review nit. Can people transfer
r= across please.

Comment 26

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 171918 [details] [diff] [review]
Patch addressing review comments

sr=pink

has a bug been filed with apple? someone should do that before it's forgotten
about forever and other apps have to suffer (like colloquy)

Comment 27

[Josh Aas]

landed on trunk

Comment 28

[Stuart Morgan]

> has a bug been filed with apple?

Consider them informed.

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I don't know the procedure for officially marking a bug as "Verified", but I
will confirm that 2005012108 (v0.8+) no longer crashes when: 

1) attempting to start a download with a) an IC.plist w/o a downloads directory
entry and b) without an IC.plist

2) opening Camino's Downloads prefpane when the same a) and b) conditions exists.

Great fix!

Comment 30

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 171918 [details] [diff] [review]
Patch addressing review comments

need a= for this to get on the branch.

Comment 31

[Mike Kaply [:mkaply]]

Comment on attachment 171918 [details] [diff] [review]
Patch addressing review comments

a=mkaply