Closed Bug 532094 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Places, defect)

x86
Windows Vista
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: mak, Assigned: mak)

References

()

Details

(Keywords: crash, Whiteboard: [#71 Firefox 3.6b4 topcrash][#1 Places 3.6b4 topcrash])

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
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.
Severity: normal → critical
and this is the Places topcrash on 3.6b4
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — Splinter Review
ensuring statements are valid before using them looks like a wise choice.
and also removing the feedback observer before finalizing the used statement is a wise choice.
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.
Attachment #415393 - Flags: review?(dietrich)
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.
Attachment #415393 - Flags: review?(dietrich) → review+
Attached patch patch v1.1Splinter Review
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)
Attachment #415393 - Attachment is obsolete: true
Attachment #415409 - Flags: approval1.9.2?
Whiteboard: [#71 3.6b4 Firefox topcrash][#1 3.6b4 Places topcrash]
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.
Attachment #415409 - Flags: approval1.9.2?
http://hg.mozilla.org/mozilla-central/rev/831838886578
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [#71 3.6b4 Firefox topcrash][#1 3.6b4 Places topcrash] → [#71 Firefox 3.6b4 topcrash][#1 Places 3.6b4 topcrash]
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 415409 [details] [diff] [review]
patch v1.1

everything looks fine on central, afaict. asking approval for real 1.9.2 landing.
Attachment #415409 - Flags: approval1.9.2?
Attachment #415409 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 415409 [details] [diff] [review]
patch v1.1

a192=beltzner, great work, Mak
Crash Signature: [@ nsNavHistory::AutoCompleteFeedback(int, nsIAutoCompleteController*)]
You need to log in before you can comment on or make changes to this bug.