313990 - Firefox crashes on startup if bookmarkbackups/ is unreadable [@nsBookmarksService::ArchiveBookmarksFile]

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Matt Nordhoff (aka Peng on IRC & forums)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051026 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051026 Firefox/1.5

If the bookmarkbackups/ directory in your profile is unreadable (not chmod +r), Firefox will crash on startup.

If it is readable, even if it's not writable or executable, it won't crash.

Reproducible: Always

Steps to Reproduce:
1. chmod 000 bookmarkbackups/
2. Start Firefox
Actual Results:  
Firefox crashes

Expected Results:  
Firefox would start up normally, though it certainly wouldn't make a backup of bookmarks.html.

Standard Firefox nightly, empty profile.

It was also confirmed on a trunk CVS build by Mook in #firefox.

Here's what he had to say about what causes it:

<Mook> http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4395 backupFileNames.mImpl is null
<Mook> strange, hasMoreElements is 0...
<Mook> hmm, existingBackups->mRawPtr is null
<Mook> Peng: file it as http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4377 existingBackups needs a null check.
<Mook> Of course, you probably know better than I do :)
<Mook> well, existingBackups is null, so http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4383 crashes due to dereferencing a null nsCOMPtr.

That's mostly over my head, of course. :P

Comment 1

[Matt Nordhoff (aka Peng on IRC & forums)]

Attachment: Useless.

Since I am unable to send in Talkback reports, here you go, though Mook's information should be useful enough.

Comment 2

[Matt Nordhoff (aka Peng on IRC & forums)]

It'll also crash if bookmarkbackups is a symlink to /dev/null. I don't know if this is caused by the same problem, but it should be able to survive bookmarkbackups being something unexpected...

Comment 3

[Sébastien Auger]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051027 Firefox/1.5

I see this too. You should put OS too All. The memory is growing near 2mb each seconds. This is a very critical bug .. we should come with a patch that shut down  this page when unreadable instead of trying to open it to finaly crash with a high memory usage.

Comment 4

[Sébastien Auger]

sorry there was a confusion with the bug before this one, it somehiow managed to post here instead of bug 313983

Comment 5

[timeless]

Comment on attachment 200967 [details]
Useless.

this is not the talkback report. it's just the data that was sent to the server. please run talkback and copy the incident id.

Comment 6

[Matt Nordhoff (aka Peng on IRC & forums)]

As I said, I couldn't connect to the server to send the reports and get Talkback IDs. And now the reports have vanished from the queue. I've crashed it again, and got TB11171279E, I think, though I can't get to the website and make sure.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Check rv, don't warn. Fixes the crash.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 201215 [details] [diff] [review]
patch

>Index: browser/components/bookmarks/src/nsBookmarksService.cpp
>===================================================================

>     if (forceArchive == PR_FALSE) {
>+

get rid of the extra new line.

>         nsCOMPtr<nsISimpleEnumerator> existingBackups;
>-        bookmarksBackupDir->GetDirectoryEntries(getter_AddRefs(existingBackups));
>+        rv = bookmarksBackupDir->GetDirectoryEntries(getter_AddRefs(existingBackups));
>+        if (NS_FAILED(rv)) return rv;

We should warn here, so use NS_ENSURE_SUCCESS.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch v2

Comment 10

[Asaf Romano (gone)]

Comment on attachment 201355 [details] [diff] [review]
patch v2

r=mano

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Trunk:
mozilla/browser/components/bookmarks/src/nsBookmarksService.cpp;
new revision: 1.71;

Comment 12

[Matt Nordhoff (aka Peng on IRC & forums)]

I can confirm it's fixed in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051101 Firefox/1.6a1 ID:2005110104, but is there any chance of the patch going into 1.5? I want to feel more secure about making Firefox unable to write to bookmarkbackups (though if that bug about configuring it was fixed......), but I don't want to use the trunk (it currently has some other annoying bugs).

Comment 13

[Asaf Romano (gone)]

Comment on attachment 201355 [details] [diff] [review]
patch v2

Asking for approval on behalf of Gavin. This is a very safe reproducable startup crash fix.

Comment 14

[Asaf Romano (gone)]

landed this for gavin to make sure this makes today's builds.

Checking in nsBookmarksService.cpp;
/cvsroot/mozilla/browser/components/bookmarks/src/nsBookmarksService.cpp,v  <--  nsBookmarksService.cpp
new revision: 1.67.2.3; previous revision: 1.67.2.2
done

Comment 15

[Matt Nordhoff (aka Peng on IRC & forums)]

Yup, no crash in today's mozilla1.8 build. Thanks very much, both of you. :)