Crash when Places imports FF1.5 bookmarks

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
P1
critical
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Robert Sayre, Assigned: Brian Ryner (not reading))

Tracking

({crash})

Trunk
x86
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

8.43 KB, text/plain
Details
11.46 KB, patch
Annie Sullivan
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
On Ubuntu Linux 5.10/GCC4, Firefox crashes when importing bookmarks from FF1.5.
(Reporter)

Comment 1

12 years ago
Created attachment 208524 [details]
stack trace

Updated

12 years ago
Blocks: 318057

Updated

12 years ago
Keywords: crash

Comment 2

12 years ago
Robert: Can you attach your bookmarks.html file to this bug? It looks like the problem is with one of your livemarks, you might be able to remove everything but that to avoid sharing all your bookmarks with the world...
Assignee: nobody → bryner

Comment 3

12 years ago
It seems to be a problem where the associated web site wasn't set for a livemark.  We should handle this better.  Bryner, do you want me to look into it?
(Assignee)

Comment 4

12 years ago
sure
Assignee: bryner → annie.sullivan
Assignee: annie.sullivan → bryner
Priority: -- → P1
(Assignee)

Comment 5

12 years ago
Ok, I think I see the problem here.  The LivemarkLoadListener gets a pointer to a LiveMarkInfo that lives inside of the mLivemarks array.  This is not good, because if mLivemarks ever has to realloc its storage, the item will be moved and the LivemarkLoadListener will be pointing to freed memory.
(Assignee)

Comment 6

12 years ago
Created attachment 209262 [details] [diff] [review]
patch

The first part of this patch is pretty straightforward.  I changed the LivemarkInfo objects to be heap-allocated so we don't have to worry about them moving.

I noticed another problem though -- it seems like we need to clean up somehow if a livemark is removed when there's a pending LivemarkLoadListener for that livemark.  I added a channel pointer to the LivemarkInfo object, and cancel the channel before removing the livemark.  I think this should be ok -- darin, am I missing any cases where I could end up with a dangling channel pointer, or the pointer wouldn't get cleared as expected, or OnStopRequest could still fire after the livemark is removed?
Attachment #209262 - Flags: superreview?(darin)
Attachment #209262 - Flags: review?(annie.sullivan)

Updated

12 years ago
Attachment #209262 - Flags: review?(annie.sullivan) → review+

Comment 7

12 years ago
Bryner--your patch definitely fixes a serious problem in livemarks, but I think that the problem Robert is reporting is that we don't handle the aSiteURI argument being null in CreateLivemark().

From the callstack:

#9  0xb4965d59 in nsLivemarkService::CreateLivemark (this=0x8574568, aFolder=3, aName=@0x85a102c, aSiteURI=0x0,
    aFeedURI=0x84f7438, aIndex=-1, aNewLivemark=0xbfff1c7c)
    at /home/sayrer/firefox/mozilla/browser/components/places/src/nsLivemarkService.cpp:256

And from nsLivemarkService.cpp, line 256:
  rv = aSiteURI->GetSpec(siteURISpec);

So I think we should also add an if (aSiteURI != nsnull) around the part where we add an annotation.

Comment 8

12 years ago
Comment on attachment 209262 [details] [diff] [review]
patch

I spoke with bryner offline about some problems with this patch.  He's working on a revised version.
Attachment #209262 - Flags: superreview?(darin) → superreview-
(Assignee)

Comment 9

12 years ago
Created attachment 209624 [details] [diff] [review]
patch

Fixes the problem Annie mentioned and the stuff Darin and I talked about.  The major changes are:

- LivemarkInfo objects are now refecounted. This is necessary since the lifetime of the LivemarkLoadListener could be different from the lifetime of the entry in the livemark service's array.
- We create a distinct loadgroup for each livemark load.  This is necessary so that we can always cancel the load (in the case of a redirect, a new channel will be created so cancelling the original will have no effect, but the new channel will be in the same loadgroup).

Unfortunately, livemark importing seems to be broken for other reasons right now so I can't test this.
Attachment #209262 - Attachment is obsolete: true
Attachment #209624 - Flags: superreview?(darin)
Attachment #209624 - Flags: review?(annie.sullivan)

Updated

12 years ago
Attachment #209624 - Flags: review?(annie.sullivan) → review+

Updated

12 years ago
Attachment #209624 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 10

12 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.