Closed Bug 572800 Opened 11 years ago Closed 11 years ago

Ensure previous autocomplete query has been canceled before starting a new one (was: invalid 'in' operand this._usedPlaceIds)

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b1

People

(Reporter: zpao, Assigned: mak)

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

I've hit this a couple times with no good STR, so just filing to have on record.

The first time I hit this, I had opened a new tab (browser session has been open for a while) and was trying to match a recent url in the location bar. No results showed and that error was on the console. Attempts to reproduce failed.

The second time I was updating Minefield and had some other oddities happening too - 1 window has chrome correct, others didn't seem to remember toolbar customizations, some extension errors, an error from DownloadUtils.jsm. This places error was in the console several times. I tried to reproduce by restarting a few times after, but no luck.

Both of these have been in the past month using Minefield.
<gavin>	nsAutoCompleteController::HandleText calls stopSearch() before StartSearchTimer(), but nsAutoCompleteController::HandleKeyNavigation doesn't
<gavin>	I'm not sure the nsIAutocompleteSearch impl should depend on the controller doing that right, though

idea is: fix the controller, but also make so that autocomplete implementations cancel previous search if the controller "forgets" to do it before starting a new search.
actually fixing the above could increase responsivity of the location bar, if we end up having 2 live queries we have a slowdown we don't really need, plus bugs that could cause having results notifications when we don't expect them.
Keywords: perf
OS: Mac OS X → All
Hardware: x86 → All
Summary: Invalid 'in' operand this._usedPlaceIds - nsPlacesAutoComplete.js → Ensure previous autocomplete query has been canceled before starting a new one (was: invalid 'in' operand this._usedPlaceIds)
Attached patch patch v1.0 (obsolete) — Splinter Review
something like this, not sure how much of this can be automatically tested
Attached patch patch v1.1 (obsolete) — Splinter Review
The test sux a bit because is somehow bound to the implementation but should touch all paths ensuring stopSearch is called before startSearch (and last test is failing before the patch, passing after, no news here). The ac code is full of special cases thus it's hard to make a nice generic test (and I fear going through b-c would be too much orange-prone).
Attachment #452244 - Attachment is obsolete: true
Attachment #452459 - Flags: review?(sdwilsh)
taking for now.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment on attachment 452459 [details] [diff] [review]
patch v1.1

You should use the public domain license block for tests per http://www.mozilla.org/MPL/license-policy.html

Also, please add a test where you call the places autocomplete with startSearch twice (yes impl. dependent, but we can still test that it won't fail).

r=sdwilsh
Attachment #452459 - Flags: review?(sdwilsh) → review+
Attached patch patch v1.2Splinter Review
fixed license, not going to add another test per discussion on IRC. To summarize a test calling twice startSearch in Places impl would not expose anything interesting since we came here through a single js warning, not an error.
Attachment #452459 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/73a19cbc9adc
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
You need to log in before you can comment on or make changes to this bug.