408463 - top crash @ nsAutoCompleteController::HandleStartComposition(), RevertTextValue() and HandleKeyNavigation()

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

top crash @ nsAutoCompleteController::HandleStartComposition()

From a crash report:  http://crash-stats.mozilla.com/report/index/27165375-a90f-11dc-99a5-001a4bd43e5c

Looking at the code:

 339    // Stop all searches in case they are async.
 340    StopSearch();
 341                   
 342    PRBool isOpen;
 343    mInput->GetPopupOpen(&isOpen);

From the stack:

0	nsAutoCompleteController::HandleStartComposition()	mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp:343 
1	NS_InvokeByIndex_P	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101 
2	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2347 
3	XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*)	mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1467

This sounds familiar, right?  (See bug #395344)

After we call StopSearch(), we need to check mInput

Something like:

   // Stop all searches in case they are async.
   StopSearch();
 
+  if (!mInput) {
+    // StopSearch() can call PostSearchCleanup() which might result
+    // in a blur event, which could null out mInput, so we need to check it
+    // again.  See bug #395344 for more details
+    return NS_OK;
+  }

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 2

[(not reading, please use [e-mail] instead)]

From http://crash-stats.mozilla.com/?do_query=1&product=Firefox&query_search=signature&query_type=contains&query=nsAutoCompleteController&date=&range_value=1&range_unit=weeks, it looks like this is happening in a few other places:

nsAutoCompleteController::RevertTextValue()

1136                   nsresult
1137                   nsAutoCompleteController::RevertTextValue()
1138                   {
1139                     nsAutoString oldValue(mSearchString);
1140                     
1141                     PRBool cancel = PR_FALSE;
1142                     mInput->OnTextReverted(&cancel);  

nsAutoCompleteController::EnterMatch()

and 

nsAutoCompleteController::HandleKeyNavigation()

I'll audit nsAutoCompleteController and add the check for !mInput after calls to StopSearch() where necessary.

Comment 3

[Asaf Romano (gone)]

Comment on attachment 293276 [details] [diff] [review]
patch

r=mano

Comment 4

[(not reading, please use [e-mail] instead)]

Checking in nsAutoCompleteController.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp,v  <--  nsAutoCompleteController.cpp
new revision: 1.73; previous revision: 1.72
done

fixed checked in, but I still need to audit and the rest of the code for other instances and fix them.

I'll do that in a follow up patch.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: bulletprooting

Comment 6

[(not reading, please use [e-mail] instead)]

this patch contains the fix for crash in 
nsAutoCompleteController::RevertTextValue(), which makes sense as we are calling StopSearch() before calling RevertTextValue().

this patch contains another fix for a crash in nsAutoCompleteController::HandleKeyNavigation(), but I'm not seeing where StopSearch() is happening, so I've bulletproofed and added a warning.

the crash in nsAutoCompleteController::EnterMatch() from crash-stat was from a long time ago, and was just once, so I'm not as worried about it, but I'll keep an eye on it:

http://crash-stats.mozilla.com/?do_query=1&product=Firefox&query_search=signature&query_type=contains&query=nsAutoCompleteController&date=&range_value=1&range_unit=months

Comment 7

[(not reading, please use [e-mail] instead)]

I've logged a spin off bug #409902, per mano, to figure out why mInput is null in HandleKeyNavigation().

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: updated bulletproofing

Comment 9

[Asaf Romano (gone)]

Comment on attachment 294614 [details] [diff] [review]
updated bulletproofing

r=mano

Comment 10

[(not reading, please use [e-mail] instead)]

fixed.

Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp,v  <--  nsAutoCompleteController.cpp
new revision: 1.74; previous revision: 1.73
done

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

I ran separate queries for the three methods listed in comment 6, and http://crash-stats.mozilla.com couldn't find any current crashers.

Verified FIXED