236659 - Crash in nsAutoCompleteController when using URL bar autocomplete too soon after startup

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Lorenzo Colitti]

If you try to use URL bar autocomplete too soon after firefox starts, it crashes.

Reproducible: always (if you're fast enough).

1. Start Firefox
2. As soon as the browser appears, very quickly type a letter (e.g. y), then
press down arrow, then press enter. Note: you have to be very quick to see this.

Actual results:
1. The status bar shows "Resolving host y"
2. Firefox crashes

Expected results: No crash

I'm setting severity to normal since it's hard to trigger and there's no data
loss (the browser has just started up).

Comment 1

[Lorenzo Colitti]

Attachment: Stack from debug build

Stack trace from debug build

Comment 2

[Lorenzo Colitti]

The crash is due to dereferencing a NULL mInput in ClosePopup(), but I *think*
the root cause is that StartSearchTimer() is called a second time before
Notify() is called for the the first search.

The following text files contain a log of all nsAutoCompleteController::<xxx>
functions that are called when it crashes (if the keys are pressed too soon
after startup) and when it doesn't (if the same keys are pressed a bit later).

Comment 3

[Lorenzo Colitti]

[e-mail]: sorry for that midair collision. :)

I don't think this is critical since (a) it's so hard to trigger and (b) there's
no real dataloss since the browser has just started up. This is more of a polish
issue.

Comment 4

[Lorenzo Colitti]

Attachment: Functions called when crash occurs

Since it's a timing issue, here is a list of all nsAutoCompleteController
member functions called when the crash occurs. This was obtained on a debug
build via printfs in the code and "firefox -console"

Comment 5

[Lorenzo Colitti]

Attachment: Functions called when crash does not occur

Same as above, but with the keys pressed a litte slower and no crash.

Comment 6

[Lorenzo Colitti]

Attachment: Wallpaper patch

This fixes the problem for me.

It seems to work fine, but I don't know if it's the correct approach.

Comment 7

[Lorenzo Colitti]

Comment on attachment 143111 [details] [diff] [review]
Wallpaper patch

Requesting review so that someone else can look at it.

Comment 8

[Brian Ryner (not reading)]

I haven't debugged this, but...  it seems like the root of the problem is that
we should not allow a second timer to be created if we're still waiting on our
current timer to fire.  We attempt to cancel the timer but we end up cancelling
only the second one since we no longer have a reference to the first one.

In other words, how about just making StartSearchTimer return immediately if
mTimer is not null?

Comment 9

[Lorenzo Colitti]

Attachment: Patch as suggested by bryner

bryner, this patch does what you suggested. It fixes the crash for me.

Comment 10

[Lorenzo Colitti]

Attachment: Same patch without typos

Same as above, except without typos. :)

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 143202 [details] [diff] [review]
Same patch without typos

Looks good, my only comment is to make the |if| style match what is used
elsewhere in the file:

if (!mTimer) {

I'll go ahead and fix that and check it in.  Thanks for the patch.

Comment 12

[Brian Ryner (not reading)]

Ended up making another change to put the GetTimeout stuff inside the |if|, and
tweaked the comment a bit.  Checked in.

Comment 13

[Lorenzo Colitti]

bryner, thanks for the quick turnaround!

Comment 14

[Lorenzo Colitti]

Reassigning for credit. Please excuse the spam.