674353 - Crash when pressing enter in location bar with Enter Selects addon enabled

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Josh Matthews [:jdm]]

STR:
1. install Enter Selects (with addon compatibility disabled for trunk)
2. add two search keywords that are very close (ie. mxr and mxrf)
3. In the location bar, type "mxr testing"
4. reposition the cursor to just after the |mxr|, then add an f
5. press enter before the autocomplete box appears

Expected:
either a google search for |mxrf testing| or an mxr search for |testing|

Actual:
crash

Comment 1

[Josh Matthews [:jdm]]

Attachment: Backtrace

Comment 2

[Josh Matthews [:jdm]]

https://crash-stats.mozilla.com/report/index/bp-4fdccee4-011e-484f-b717-7a93e2110610

Comment 3

[Josh Matthews [:jdm]]

So we're stuck in a loop when calling nsAutoCompleteController::OpenPopup, which triggers the load event in EnterSelects (presumably, since mInput->SetPopupOpen goes directly into the xpconnect machinery and we end up interpreting from enterSelects.xul:46). This code ends up calling gURLBar.controller.handleEnter, which calls nsAutoCompleteController::PostSearchCleanup, which tries to call OpenPopup again, and we spiral downwards into oblivion.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Avoid using null pointer when trying to autocomplete.

Comment 5

[Josh Matthews [:jdm]]

Marco, it looks like my earlier analysis has been superseded by the changes from inline autocompletion. As such, this patch now fixes the crash for me.

Comment 6

[Josh Matthews [:jdm]]

Example of the new crash report: https://crash-stats.mozilla.com/report/index/bp-31a0d632-2178-4e62-8914-82fc82120125

Comment 7

[Marco Bonardo [:mak]]

So, ProcessResult checks mInput, what makes it null later, the popup actions (invalidate, open, close) in ProcessResult?

Comment 8

[Josh Matthews [:jdm]]

I hoped I would be able to catch the nulling-call to SetInput in gdb, but I haven't been able to do so (too many focus problems that cause the bug not to be observed). I presume that the popup code triggers events in the Enter Selects addon, which then causes SetInput(NULL) to be called for some reason.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 591457 [details] [diff] [review]
Avoid using null pointer when trying to autocomplete.

Review of attachment 591457 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ -1390,5 @@
>  
>  nsresult
>  nsAutoCompleteController::CompleteDefaultIndex(PRInt32 aResultIndex)
>  {
> -  if (mDefaultIndexCompleted || mBackspaced || mSearchString.Length() == 0)

Hm, well I would have preferred some sort of comment on how we reach the mInput unbinding situation, for future reference, but if that's so hard to figure, let's begin by stopping the crashes for now.
I see in other parts of this code we have to do the same check for similar reasons.

Comment 10

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/863a1db98fbe

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/863a1db98fbe