Closed Bug 532094 Opened 11 years ago Closed 11 years ago
crash [@ ns
Nav History::Auto Complete Feedback(int, ns IAuto Complete Controller*)]
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.
and this is the Places topcrash on 3.6b4
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+
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)
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.
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? → approval1.9.2+
Crash Signature: [@ nsNavHistory::AutoCompleteFeedback(int, nsIAutoCompleteController*)]
You need to log in before you can comment on or make changes to this bug.