327331 - nsNavHistory::Init allows improper initialization

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

If nsNavHistory::Init fails, it will permanently leak the nsNavHistory object through pref observers and observerservice observers: CreateLookupIndexes() should be called before attaching observers. Once we attach observers we should not throw exceptions. In addition, there are situations (such as when getService is called with an interface nsNavHistory doesn't implement) where the constructor function itself could fail. We should use a specialty singleton constructor func to ensure against error.

Comment 1

[Benjamin Smedberg]

Attachment: Use a singleton constructor, rev. 1

Comment 2

[Brett Wilson]

Comment on attachment 212019 [details] [diff] [review]
Use a singleton constructor, rev. 1

>     if (NS_SUCCEEDED(rv) && historyFile) {
>       importer->ImportHistory(historyFile, this);
>     }
>   }
> 
>-  return CreateLookupIndexes();
>+  return NS_OK;

Can you put a comment  above the return statement saying something about not adding anything here and referencing the above comment? The bottom of the function is the most tempting place to add new stuff, and most people won't read the previous one.

I just noticed that the bookmark service has the same problem. Filed bug 327350 on that.

Comment 3

[Benjamin Smedberg]

Fixed on trunk.

Comment 4

[Brett Wilson]

All places stuff needs to be manually checked into 1.8 branch as well. It looks like this didn't make it.

Comment 5

[Benjamin Smedberg]

This was mostly backed out by bug 327331 with a cryptic comment:

Indeed, create would not put the pointer in the service manager. That's ok, because we use the gHistoryService object itself in the constructor, so there isn't a chance that I can see of ever creating a dual object.

The problem here is that if you use the following code:

var history = Components.classes[navhistorycontractid].getService(Components.interfaces.nsINotImplemented);

The constructor succeeds, adds observers, and sets gHistoryService, while getservice fails (due to the unimplemented interface)... then you *can* assert.

Comment 6

[Brett Wilson]

Sorry about the cryptic comment, I was in the middle of having the build broken and couldn't find you on IRC.

I was definitely seeing double initialization, although I realize now its because of my build being out of sync here. We can check it in again. I can do it next week or you can do it anytime you want. You really need to but it on the branch this time, though. Places is kept in sync between trunk and branch.

Comment 7

[Dietrich Ayala (:dietrich)]

this was checked in but not marked fixed. need to do further analysis of svc init to determine if this is fixed or not. would like to have some kind of automated test that tests init with critical failure scenarios, etc.

Comment 8

[Benjamin Smedberg]

Dietrich, as far as I know this was backed out and never checked back in (it doesn't appear on current trunk).

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> Created an attachment (id=212019) [details]
> Use a singleton constructor, rev. 1
> 

hm, this actually is allowing multiple initialization.

interestingly, if i initialize from xpconnect first, it works ok. but if i first initialize the bookmarks service (which inits history), then try to get the service from js, then it'll double initialize.

Comment 10

[Dietrich Ayala (:dietrich)]

changing the title to reflect that the leak-observers-if-init-fails problem is gone, but the getService-with-not-implemented-iid problem still exists.

spinning off the "fragile in error conditions" to bug 327350, covering each of the places services.

this does not necessarily block A5, as we don't have any reported problems, and the scenario in comment #5 is more likely from extension code than internally.

Comment 11

[Dietrich Ayala (:dietrich)]

preventative, pushing to beta 1.

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: v1.0 cleanup

OK, so it looks like the first part of comment 0 is already addressed here.  While figuring out what was going on in nsNavHistory::Init, I went ahead and added some javadoc comments to places in nsNavHistory.h, and I made the warnings about adding things after the observers a bit more scary looking to ensure that nobody EVER adds something after them without really thinking hard about it first.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v1.0 fix

Basically the first patch :)

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 300760 [details] [diff] [review]
v1.0 cleanup


>+  /**
>+   * Initializes the database file.  If the database does not exist, was
>+   * corrupted, or aForceInit is true, we re-import bookmarks.  We also backup

not really true for non-fx apps. please correct to "we recreate the database".

>+   * the database if it was corrupted or aForceInit is true.
>+   *
>+   * @param aForceInit
>+   *        Indicates if we should close an open database connection or not.
>+   */
>   nsresult InitDBFile(PRBool aForceInit);


>+  /**
>+   * Initializes the database.  This performs any necessary migrations for the
>+   * database.  All migration is done inside a transaction that is rolled back
>+   * if any error occurs.

i'd clarify to say that history is imported, and some prefs set. but even those prefs i'm planning on moving out to browser, to make this less browser-dependent (bug 394205).



>+  /**
>+   * Used to setup the idle timer used to update global frecency.
>+   */
>   nsCOMPtr<nsITimer> mIdleTimer;

also for expiration, etc. i'd generalize to "periodic update and cleanup tasks", or something to that effect.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v1.1 cleanup [checked-in]

(In reply to comment #14)
> also for expiration, etc. i'd generalize to "periodic update and cleanup
> tasks", or something to that effect.
There is mExpireNowTimer for expirations, mLazyTimer for lazy add, mAutoCompleteTimer for autocomplete, and this one...

We should consider optimizing that situation...

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 301034 [details] [diff] [review]
v1.1 cleanup [checked-in]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
new revision: 1.241; previous revision: 1.240
Checking in toolkit/components/places/src/nsNavHistory.h;
previous revision: 1.130

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

So, the last patch was actually triggering the "creating nsNavHistory twice" assertion when running test_exclude_livemarks.js.  My hunch was that somehow we were hitting a refcount of 0, so this fixes it.  I just don't know if this fixes the situation where we leak because getService is called on an interface nsNavHistory does not implement.  Since this changed so much, re-requesting review.  This patch just does what the download manager and storage service does.

How places just passes pointers to itself around is rather confusing I might add...

Comment 18

[Benjamin Smedberg]

Comment on attachment 302151 [details] [diff] [review]
v1.1

Oh man, how I hate the SINGLETON_CONSTRUCTOR macro!

Comment 19

[Shawn Wilsher :sdwilsh]

Why do you hate the SINGLETON_CONSTRUCTOR macro?

Checking in toolkit/components/places/src/nsNavHistory.cpp;
new revision: 1.250; previous revision: 1.249
Checking in toolkit/components/places/src/nsNavHistory.h;
new revision: 1.138; previous revision: 1.137
Checking in toolkit/components/places/src/nsPlacesModule.cpp;
new revision: 1.6; previous revision: 1.5

Comment 20

[Gervase Markham [:gerv]]

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