532094 - crash [@ nsNavHistory::AutoCompleteFeedback(int, nsIAutoCompleteController*)]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

While taking a look at crashkill stats i noticed this crash, number 71 between 3.6b4 crashes, but is also has a report on 3.5.3, so does not look like a regression.

I was surprised not seeing this filed

xul.dll  	nsNavHistory::AutoCompleteFeedback  	 toolkit/components/places/src/nsNavHistory.cpp:7886
1 	xul.dll 	nsNavHistory::Observe 	toolkit/components/places/src/nsNavHistory.cpp:5643
2 	xul.dll 	nsObserverService::NotifyObservers 	xpcom/ds/nsObserverService.cpp:182
3 	xul.dll 	nsAutoCompleteController::EnterMatch 	toolkit/components/autocomplete/src/nsAutoCompleteController.cpp:1141
4 	xul.dll 	nsAutoCompleteController::StopSearch 	toolkit/components/autocomplete/src/nsAutoCompleteController.cpp:1026

we crash on 7886  rv = stmt->BindStringParameter(0, input);

stmt is obtained through mozIStorageStatement *stmt = GetDBFeedbackIncrease();
and i think it could be invalid in some situation.

Comment 1

[Marco Bonardo [:mak]]

Mike, not being a regression i'm not asking blocking, but in case we get a fix really soon, would probably be cool to have it on 3.6.

Comment 2

[Marco Bonardo [:mak]]

and this is the Places topcrash on 3.6b4

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

ensuring statements are valid before using them looks like a wise choice.

Comment 4

[Marco Bonardo [:mak]]

and also removing the feedback observer before finalizing the used statement is a wise choice.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 415393 [details] [diff] [review]
patch v1.0

looks good enough for review, if the stmt creation fails for any reason, we get a null statement, we should just ensure it's valid before using it, otherwise throw.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 415393 [details] [diff] [review]
patch v1.0

>-  mozIStorageStatement *GetDBVisitToURLResult();
>+  mozIStorageStatement * GetDBVisitToURLResult();
>   nsCOMPtr<mozIStorageStatement> mDBVisitToURLResult; // kGetInfoIndex_* results
>-  mozIStorageStatement *GetDBVisitToVisitResult();
>+  mozIStorageStatement * GetDBVisitToVisitResult();
>   nsCOMPtr<mozIStorageStatement> mDBVisitToVisitResult; // kGetInfoIndex_* results
>-  mozIStorageStatement *GetDBBookmarkToUrlResult();
>+  mozIStorageStatement * GetDBBookmarkToUrlResult();
These stile changes are contrary to how we do pointers everywhere else.  I'd prefer it if you didn't do them.

>-  mozIStorageStatement* GetDBFeedbackIncrease();
>+  mozIStorageStatement * GetDBFeedbackIncrease();
Although fixing this one to be like the rest is a good idea.

r=sdwilsh with style nits fixed.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.1

we finally have a coding style, i was eagerly waiting for this :)

addressed comments.

Asking pre-approval for m-c and 1.9.2, this is a pretty common Places crash, present in 3.5.x and 3.6.x (about 500 crashes in 2 weeks)

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 415409 [details] [diff] [review]
patch v1.1

You have my approval to land this on trunk once reviewed, renominate if it passes through that OK.

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/831838886578

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 415409 [details] [diff] [review]
patch v1.1

everything looks fine on central, afaict. asking approval for real 1.9.2 landing.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 415409 [details] [diff] [review]
patch v1.1

a192=beltzner, great work, Mak

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d7c3f3105089