Closed Bug 1042561 Opened 10 years ago Closed 9 years ago

Addressing autocomplete widget: Typed text in red despite results/matches found if suggestions change by last input

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 + fixed
firefox39 --- fixed
firefox-esr31 - wontfix
firefox-esr38 --- fixed

People

(Reporter: aryx, Assigned: ttaubert)

References

Details

(Keywords: regression)

Attachments

(6 files, 4 obsolete files)

1.41 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
6.05 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
1.85 KB, patch
mak
: review+
Details | Diff | Splinter Review
1.46 KB, patch
mak
: review+
Details | Diff | Splinter Review
3.95 KB, patch
mak
: review+
Details | Diff | Splinter Review
9.15 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
Thunderbird 31.0 (20140717165725) on Windows 8.1
Thunderbird Daily 2014-07-22 on Windows 8.1

Addressing autocomplete widget: Typed text in red despite results/suggestions/matches found

Steps to reproduce:
1. Have two contacts in the address book. Second one should contain the first letter of the first contact not followed by the second letter of the first contact.
2. Open mail composition window.
3. In the To field, type the first letter of the contact name.
Actual and expected result: Typed text shown in black.
4. Type 1 more letter of the name.
Actual result: Typed letters shown in red.
Expected result: Typed letter shown in black.

The text only seems to be red if the suggestions change after typing in the last letter.
Keywords: regression
Can verify although I get both red and black depending on how many letters I type. See this thread

http://forums.mozillazine.org/viewtopic.php?f=39&t=2854517

third post
Dependent on typing speed: if more than one entry found, then next letter entered turns test red. When typing too fast for multiple entries to show in drop-down list, issue does not occur.
Both this bug and bug #1034976 appeared at the same time, in updating Thunderbird 24.6.0 to 31.0.  Both are regressions that might have been caused by the same change.
For me this is only happening on Windows (Windows 7). I'm not seeing this with TB31 on Linux.
It is also happening on OSX
Setting platform to "All" due to comment #5 (Mac OSX), even though not observed on Linux.
OS: Windows 8.1 → All
Hardware: x86_64 → All
(In reply to rsx11m from comment #7)
> Setting platform to "All" due to comment #5 (Mac OSX), even though not
> observed on Linux.

I have two computers, both running Kubunbtu 14.04, and both of which show this behaviour following updating to TB 31.0
Confirming as regression due to bug 959209:

Last good: 20140427
First bad: 20140428

The bug occurs if the autocomplete popup is shown before the last keystroke, independent from the fact if the suggestions changed.
In mozilla/toolkit/content/widgets/autocomplete.xml

<method name="onSearchComplete">

this.mController.matchCount == 0 when it shouldn't. Also, onSearchComplete fires twice.
Same problem here.

Xubuntu Precise 12.04

Too many bugs with 31. Somone kindly posted a link to 24 here : https://bugzilla.mozilla.org/show_bug.cgi?id=1043310  for those like me who want to revert :

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/releases/24.7.0/
Using windows Vista OS
Thunderbird: v 31.0

In new Write window To field type first letter and it is black text.
options appear in drop down and they are black text.
If I select an address whilst the typed in letter(s)is black then the email address is autocompleted in black.

If I then type a second letter, the type turns to red and the same drop down is in black, but when I select an address, it is inserted in red type. 

I then have to remove part of the line until the black text reappears, which is after deleting the email address part and the left chevron but not the display name before being able to get black type again and reselecting.

Occurs every time, not random.


I'm not sure if this is a separate bug but it does relate:
If I type: ga
it will be in red type and the drop down options are in black type.
There seems to have been a modification in the search parameters.
The first option is a name that does not have any 'ga' in the name, but there is a 'ga' within the email address. 
This seems to be returning loads of erroneous options and in some cases slows up the search. After all, if you type in 'a', how many search results would you expect this new searching method to produce?

However, if I then select the first option where the Display name does not contain the 'ga' but it is in the email ddress, then an error occurs. 
I end up with the following being autocompleted in the To field:
ga >> Display Name followed by email address in chevrons
This is also all in red type and also an invalid email address.

if i only type letter 'g' it will be in black text
it i then select the sme first option as above, I end up with the following being autocompleted in the To field:
g >> Display Name followed by email address in chevrons
but this time it is written in black text and still an invalid email address.

Reproducable everytime.
I should clarify comment 12.

if i only type letter 'g' it will be in black text
it i then select the same first option which is displaying in the actual address To field, I end up with the following being autocompleted in the To field and written in black type:
g >> Display Name followed by email address in chevrons


if i only type letter 'g' it will be in black text
If i then ignore the prompt displaying in the actual To field, but select the first option in the drop down list, then the correct email address is inserted in black type.

if i only type letters 'ga' it will be in red text
If i then ignore the prompt displaying in the actual To field, but select the first option in the drop down list, then the correct email address is inserted, but in red type.

So there seems to be a couple of issues but all related.
1. The search has gone awol, potentially it will choose everything in the address books when you input the first letter.

2. more than one letter typed turns text red and autocompletes in red.

3. selecting a contact where the display name does not match search but matches email address
can return red text or black text depending upon number of letters and if selecting the prompt in the TO field will insert an invalid email address.
Odd. Maybe autocomplete is hurt or stressed, hence the red :(
See Also: → 928641
This thread is more to the point than bug 1034976 but doesn't address what I see. The red color is irritating but not consequential. The auto-completion options offered are off-base and consequential.

It appears that TB maintains a combined-list content of the address book sorted from most frequent addressee to the least. When I type any single letter in the to-field, it selects the first entry that contains the typed letter, almost never the desired addressee. For example, suppose my most frequent correspondent is
Raggedy Ann <raggedy@ra.com>
If I type "y", the first choice is:
y >> Raggedy Ann <raggedy@ra.com>

As suggested in 1034976, I edited mail.addr_book.quicksearchquery.format to change "contains" to "beginsWith " and it had no effect.
Although, as Tom Stuart states, "auto-completion options are... consequential", the misleading red indication that an address is incorrect also has annoying usability consequences, in that it causes users to re-enter valid addresses and provides no useable feedback that an address may be invalid. Both issues require amelioration.
What is noticed when diffing applicable code with v. 30 or 29?
Version: Thunderbird 31.2.0
OS: Ubuntu 12.04
The bug as described in comment #12 &  comment #13 is still reproducable for me every time. 

Especially the part of this bug which adds two chevrons is really annoying:
This occours every time when you type one or more letters (which are not the first letters of the e-mail address but part of the display name). If auto-complete presents the right reciepient and you click in the input field (not in the dropdown), the input field shows your entered letters two chevrons Display Name and E-Mail adress in chevrons in black color.

Example: If I type "mo" and the entry "Mozilla Team <whatever@mozilla.org>" is in my address book this entry is shown in the input field. Now, if i click in the input field, this is the result:

mo >> Mozilla Team <whatever@mozilla.org>

Only if recipient name and the first part of the mail address are identical and you have it in only one address book,the address entry gets black and valid. If you have the same entry in two or more address books, the entry in the input field gets valid but red.

Do I have any option to stop this whole color changing thing until the problems are fixed?
@MichaelW: The >> are another bug: bug 1043310 (which is fixed).
Hi
Just downloaded version 31.4.0 and I still have the bug showing the to-addresses in red, even thought the person is in my main address book. They used to be in black until nov-dec 2014 unless the person was NOT in any phone-books... :-(
Thanks
Yes, this hasn't been fixed yet. We're all anxiously waiting for a fix ;-(
This is one of the remaining auto-complete regressions introduced in TB 31 and IMHO it would be good to fix it for TB 38, hence the tracking flag.
Can confirm this bug on Linux version of Thunderbird 31.4.0
(In reply to Nir from comment #27)
> Can confirm this bug on Linux version of Thunderbird 31.4.0

And Thunderbird 31.5.0.
My bug 1136712 was marked as a duplicate, but there is one aspect of my 'steps to reproduce' that I didn't see noted in this bug.

I think it may actually provide an extra clue as to how to go about fixing...

Here are my steps to reproduce:

1. Start composing an email
2. Start entering the first few characters of an email address that will result in multiple matches
3. When you enter the very first character, a list of matching addresses is presented, with the currently selected entry in black
4. Enter another character, it selects a different entry to match, and it turns red

and here is the extra tidbit I didn't see noted above:

5. Keep typing characters and note that the entry remains red unless you type at least one additional character AFTER there is only one result match showing

So, when you finally type enough characters to get to a single result, it remains red - UNTIL you type just one more character, after which it turns black again...

Hope this helps get this last addressing bug squashed...

Thanks to the TB devs, 38 is shaping up to be a really fine release!
(In reply to Charles from comment #29)
> Hope this helps get this last addressing bug squashed...

We all hate the misleading red, but unfortunately it's not the 'last' auto-complete bug. If you want something more dramatic, e-mail being send to the wrong recipient, please turn to bug 1138033 :-((
Wow, wasn't aware of this one... hope this is on track for being fixed in 31 as well...
Windows Vista
This 'red' font bug persisted until I updated to 31.5.0 when it stopped.
However, now I never get anything in red. 
I can create any correctly formed email address that I know is not in any address book and they are in black font. 
So, a weird reversal has occurred in version 31.5.0 on Windows Vista OS.
(In reply to Anje from comment #32)
> Windows Vista
> This 'red' font bug persisted until I updated to 31.5.0 when it stopped.
> However, now I never get anything in red. 
> I can create any correctly formed email address that I know is not in any
> address book and they are in black font. 
> So, a weird reversal has occurred in version 31.5.0 on Windows Vista OS.

Anje, I still get black AND red on 31.5.0 on Windows 8.1. But it's not always correct, as others have reported before.
In reply to Anje,

I'm with Thomas D - on 31.5.0 on Win7. I have exactly the same symptoms as Charles - type one more character than is needed to identify a name uniquely in order to display in black. This only fails when one person has 2 email addresses which cannot be separated on description alone.
(In reply to Anje from comment #32)
> Windows Vista
I fired up an old Vista machine and installed TB 31.5. Same wrong behaviour as on Windows 7/8:
Known addresses are marked in red under certain circumstances.
Unknown addresses are marked in red, as they should be.
Magnus pointed me to this code:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/autocomplete.xml?force=1#211
Here attribute "nomatch" is set depending on the number of matches found. This attribute drives the colour, which I confirmed by always removing it, so I only get black recipients.

So I put debug prints at
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1548 and
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/nsAutoCompleteController.cpp#79

Result: Supposedly no matches found although there was a popup with a few matches.
=== nsAutoCompleteController::PostSearchCleanup: calling OnSearchComplete, count=0
=== nsAutoCompleteController::GetMatchCount: 0
So basically there seems to be a problem in the autocomplete controller, since there were matches.
OnSearchComplete is called twice as already stated in comment #10.
If in nsAutoCompleteController::AfterSearches() you **always** call PostSearchCleanup()
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1131
(get rid of this "if (...)") then you get this result:

After first character typed:
=== nsAutoCompleteController::ProcessResult: calling PostSearchCleanup
=== nsAutoCompleteController::PostSearchCleanup: calling OnSearchComplete, count=48
=== nsAutoCompleteController::GetMatchCount: 48 <-- correct
=== nsAutoCompleteController::AfterSearches: calling PostSearchCleanup
=== nsAutoCompleteController::PostSearchCleanup: calling OnSearchComplete, count=48
=== nsAutoCompleteController::GetMatchCount: 48 <-- correct

After second character typed:
=== nsAutoCompleteController::StopSearch: calling PostSearchCleanup
=== nsAutoCompleteController::PostSearchCleanup: calling OnSearchComplete, count=0
=== nsAutoCompleteController::GetMatchCount: 0 <-- wrong
=== nsAutoCompleteController::ProcessResult: calling PostSearchCleanup
=== nsAutoCompleteController::PostSearchCleanup: calling OnSearchComplete, count=0
=== nsAutoCompleteController::GetMatchCount: 0 <-- wrong
=== nsAutoCompleteController::AfterSearches: calling PostSearchCleanup
=== nsAutoCompleteController::PostSearchCleanup: calling OnSearchComplete, count=3
=== nsAutoCompleteController::GetMatchCount: 3 <-- correct

In this case no red is displayed and the auto-complete runs as desired. So who is the module owner? Who can tell me whether unconditionally calling PostSearchCleanup() from AfterSearches() is a valid fix?

In this case OnSearchComplete is called three times, but the last time gives the correct result, in my case 3 instead of 0. The original code calls OnSearchComplete twice with the incorrect result.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Jorg K from comment #37)

> In this case no red is displayed and the auto-complete runs as desired. So
> who is the module owner? Who can tell me whether unconditionally calling
> PostSearchCleanup() from AfterSearches() is a valid fix?
> 
> In this case OnSearchComplete is called three times, but the last time gives
> the correct result, in my case 3 instead of 0. The original code calls
> OnSearchComplete twice with the incorrect result.

I don't know the details of this code, but calling the same function 3 times, of which 2 cycles yield wrong results, sounds like this code needs to be revisited, streamlined and fixed. We've had reports over delays in autocompletion, so we certainly want make this code more efficient rather than adding more inefficiency. So from my pov, if Jorg's analysis is correct, we would not accept the "fix" suggested above.
I believe that my analysis is correct. I just stuck "print" statements into the code an ran it. And I am calling PostSearchCleanup() from AfterSearches() unconditionally.

I have no expertise in this code whatsoever, and I quite agree that calling the same function twice with the wrong result is bad and in my hack/kludge calling it a third time, is most likely not a valid "fix".

So how do we move on from there? Who is the module owner? I can see a few people who worked on this:
http://hg.mozilla.org/mozilla-central/filelog/0b3c520002ad/toolkit/components/autocomplete/nsAutoCompleteController.cpp
I'll just pick one at random additionally to Magnus.
Flags: needinfo?(mak77)
If the bug is in toolkit, also Neil could help there.
Flags: needinfo?(neil)
By my reading of the code PostSearchCleanup() should only be called once mSearchesOngoing becomes zero. This should happen in three ways:
a) Autocomplete is interrupted
b) Autocomplete tries to kick off searches but they all fail to start
c) All the autocomplete results arrive
I assume a) and b) don't apply here, which just leaves c) as a possibility.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk from comment #41)
> By my reading of the code PostSearchCleanup() should only be called once

Any further analysis why this is not happening and how this can be fixed? Certainly not the way I suggested ;-)
Flags: needinfo?(neil)
From Neil's analysis I assume that the first call via 
nsAutoCompleteController::StopSearch: calling PostSearchCleanup
is undesired. Below we have the call stack. Somewhere there is a timer that fires: nsTimerImpl::Fire

Does anyone understand what's going on here? The search stops despite the fact that there are matches and the popup isn't pulled down either. Weird.

nsAutoCompleteController::StopSearch(<args>) Line 1143	C++
NS_InvokeByIndex(<args>) Line 71	C++
CallMethodHelper::Invoke(<args>) Line 2110	C++
CallMethodHelper::Call(<args>) Line 1449	C++
XPCWrappedNative::CallMethod(<args>) Line 1414	C++
XPC_WN_CallMethod(<args>) Line 1144	C++
js::CallJSNative(<args>) Line 226	C++
js::Invoke(<args>) Line 498	C++
Interpret(<args>) Line 2599	C++
js::RunScript(<args>) Line 448	C++
js::Invoke(<args>) Line 517	C++
js::Invoke(<args>) Line 554	C++
JS::Call(<args>) Line 4228	C++
mozilla::dom::EventHandlerNonNull::Call(<args>) Line 260	C++
mozilla::dom::EventHandlerNonNull::Call<nsISupports *>(<args>) Line 350	C++
mozilla::JSEventHandler::HandleEvent(<args>) Line 215	C++
nsXBLPrototypeHandler::ExecuteHandler(<args>) Line 329	C++
nsXBLEventHandler::HandleEvent(<args>) Line 51	C++
mozilla::EventListenerManager::HandleEventSubType(<args>) Line 976	C++
mozilla::EventListenerManager::HandleEventInternal(<args>) Line 1125	C++
mozilla::EventListenerManager::HandleEvent(<args>) Line 331	C++
mozilla::EventTargetChainItem::HandleEvent(<args>) Line 210	C++
mozilla::EventTargetChainItem::HandleEventTargetChain(<args>) Line 301	C++
mozilla::EventDispatcher::Dispatch(<args>) Line 636	C++
nsXULPopupManager::FirePopupHidingEvent(<args>) Line 1394	C++
nsXULPopupManager::HidePopup(<args>) Line 978	C++
mozilla::dom::PopupBoxObject::HidePopup(<args>) Line 62	C++
mozilla::dom::PopupBoxObjectBinding::hidePopup(<args>) Line 129	C++
mozilla::dom::GenericBindingMethod(<args>) Line 2537	C++
js::CallJSNative(<args>) Line 226	C++
js::Invoke(<args>) Line 498	C++
Interpret(<args>) Line 2599	C++
js::RunScript(<args>) Line 448	C++
js::Invoke(<args>) Line 517	C++
js::Invoke(<args>) Line 554	C++
js::InvokeGetterOrSetter(<args>) Line 624	C++
js::Shape::set(<args>) Line 52	C++
SetExistingProperty(<args>) Line 2220	C++
js::NativeSetProperty(<args>) Line 2250	C++
JS_SetPropertyById(<args>) Line 2712	C++
JS_SetProperty(<args>) Line 2792	C++
nsXPCWrappedJSClass::CallMethod(<args>) Line 1204	C++
nsXPCWrappedJS::CallMethod(<args>) Line 536	C++
PrepareAndDispatch(<args>) Line 85	C++
SharedStub(<args>) Line 113	C++
nsAutoCompleteController::ClosePopup(<args>) Line 1034	C++
nsAutoCompleteController::ProcessResult(<args>) Line 1504	C++
nsAutoCompleteController::OnSearchResult(<args>) Line 740	C++
NS_InvokeByIndex(<args>) Line 71	C++
CallMethodHelper::Invoke(<args>) Line 2110	C++
CallMethodHelper::Call(<args>) Line 1449	C++
XPCWrappedNative::CallMethod(<args>) Line 1414	C++
XPC_WN_CallMethod(<args>) Line 1144	C++
js::CallJSNative(<args>) Line 226	C++
js::Invoke(<args>) Line 498	C++
Interpret(<args>) Line 2599	C++
js::RunScript(<args>) Line 448	C++
js::Invoke(<args>) Line 517	C++
js::Invoke(<args>) Line 554	C++
JS_CallFunctionValue(<args>) Line 4216	C++
nsXPCWrappedJSClass::CallMethod(<args>) Line 1211	C++
nsXPCWrappedJS::CallMethod(<args>) Line 536	C++
PrepareAndDispatch(<args>) Line 85	C++
SharedStub(<args>) Line 113	C++
nsAutoCompleteController::StartSearch(<args>) Line 1098	C++
nsAutoCompleteController::Notify(<args>) Line 765	C++
nsTimerImpl::Fire(<args>) Line 634	C++
nsTimerEvent::Run(<args>) Line 729	C++
nsThread::ProcessNextEvent(<args>) Line 855	C++
NS_ProcessNextEvent(<args>) Line 265	C++
mozilla::ipc::MessagePump::Run(<args>) Line 140	C++
MessageLoop::RunInternal(<args>) Line 233	C++
MessageLoop::RunHandler(<args>) Line 227	C++
MessageLoop::Run(<args>) Line 201	C++
nsBaseAppShell::Run(<args>) Line 166	C++
nsAppShell::Run(<args>) Line 180	C++
nsAppStartup::Run(<args>) Line 282	C++
XREMain::XRE_mainRun(<args>) Line 4160	C++
XREMain::XRE_main(<args>) Line 4236	C++
XRE_main(<args>) Line 4456	C++
thunderbird.exe!do_main(<args>) Line 195	C++
thunderbird.exe!NS_internal_main(<args>) Line 380	C++
thunderbird.exe!wmain(<args>) Line 124	C++
[External Code]	
[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
(In reply to Jorg K from comment #43)
> Does anyone understand what's going on here? The search stops despite the
> fact that there are matches and the popup isn't pulled down either. Weird.
Well, I'll explain what's going on, but I can't explain why it's going on; the XPFE autocomplete widget doesn't do this.

> nsAutoCompleteController::StopSearch(<args>) Line 1143	C++
> NS_InvokeByIndex(<args>) Line 71	C++
The search is stopped by a call from JS.

> JS::Call(<args>) Line 4228	C++
> mozilla::dom::EventHandlerNonNull::Call(<args>) Line 260	C++
Which is invoked for an event.

> mozilla::EventDispatcher::Dispatch(<args>) Line 636	C++
> nsXULPopupManager::FirePopupHidingEvent(<args>) Line 1394	C++
In particular, a popuphiding event.
Looking in autocomplete.xml, the popuphiding event fired on the autocomplete-results-popup.

> mozilla::dom::PopupBoxObjectBinding::hidePopup(<args>) Line 129	C++
> mozilla::dom::GenericBindingMethod(<args>) Line 2537	C++
The popup was hidden by a call from JS.

> JS_SetProperty(<args>) Line 2792	C++
> nsXPCWrappedJSClass::CallMethod(<args>) Line 1204	C++
Which was invoked because C++ called an XPConnect setter.
Looking in autocomplete.xml, this was probably the popupOpen property of the autocomplete binding.

> SharedStub(<args>) Line 113	C++
> nsAutoCompleteController::ClosePopup(<args>) Line 1034	C++
This was the C++ that called SetPopupOpen(false);

> nsAutoCompleteController::ProcessResult(<args>) Line 1504	C++
> nsAutoCompleteController::OnSearchResult(<args>) Line 740	C++
> NS_InvokeByIndex(<args>) Line 71	C++
Because some JS autocomplete search provider returned some results.

> SharedStub(<args>) Line 113	C++
> nsAutoCompleteController::StartSearch(<args>) Line 1098	C++
Because the autocomplete component started its delayed search.

> nsAutoCompleteController::Notify(<args>) Line 765	C++
> nsTimerImpl::Fire(<args>) Line 634	C++
Because searches are delayed so that they don't start while you are typing.

This behaviour is therefore a regression from bug 660592; the old code used to say
> if (mRowCount) {
>   OpenPopup();
> } else if (mSearchesOngoing == 0) {
>   ClosePopup();
> }
The new code says
> if (mRowCount || !minResults) {
>   OpenPopup();
> } else if (result != nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING) {
>   ClosePopup();
> }
Switching this back to mSearchesOngoing would prevent the search from being stopped erroneously. I don't know whether this will actually fix the problem here though.

(Calling DumpJSStack() might have been useful.)
Flags: needinfo?(neil)
It's an advantage to know what you're doing ;-) I sadly don't :-((

I changed the line in question from
> else if (result != nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING) {
back to
> } else if (mSearchesOngoing == 0) {

And lo and behold, the red goes away, PostSearchCleanup() is only called once
=== nsAutoCompleteController::ProcessResult: calling PostSearchCleanup
=== nsAutoCompleteController::PostSearchCleanup: calling OnSearchComplete, count=3
=== nsAutoCompleteController::GetMatchCount: 3 <-- correct !!
and all is well.

So what's next?
Flags: needinfo?(neil)
(In reply to Jorg K from comment #45)
> and all is well.
> 
> So what's next?

Turn that into a patch, get review from mak, get it checked in, pat yourself on the back for doing all the debugging legwork.
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
Attachment #8574380 - Flags: review?(mak77)
As a mere whining user, I just say Thanks to Jorg & Neil, and I'll sit back and wait for the patch to come out. Regards, Michael
Jorg K, you are d man!

Thanks so much for all your hard work on these bugs!
and to Neil too! (sorry Neil, should have included you)
Blocks: 660592
Awesome teamwork everyone, thanks! :)
(In reply to Charles from comment #50)
> and to Neil too! (sorry Neil, should have included you)

I wasn't much use until comment #42 when Jorg's excellent investigative skills turned up that unexpected stack trace.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Thanks for getting this resolved. It's been 'bugging' me!

Any idea when this will be released? I didn't see a way for me to track that on my own.
Waiting for the review ;-) No idea when it's going to happen.
It missed the boat for TB 37 beta, so perhaps in 38 beta.
Component: Message Compose Window → Autocomplete
Product: Thunderbird → Toolkit
Target Milestone: --- → mozilla38
Blocks: 1142879
@Marco: Could you please review the one-line-change I made.
The change was suggested at the bottom of comment #44 by Neil.

I think it's fair to say that neither Neil nor I understand the code well, so we really need you to see whether the fix OK or to make another suggestion.

This bug is fairly important for all TB users, since the autocomplete function is used when recipient addresses are entered. If they are not found in any of the address book(s), they should be displayed in red. Due to this bug addresses found in the address book(s) are also shown in red, which is highly confusing to the user.
Flags: needinfo?(mak77)
Comment on attachment 8574380 [details] [diff] [review]
One line change worked out be Neil - THANKS!!

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

checking mSearchesOngoing looks like a good idea (I missed that change among the refactoring patch), but this patch is clearly failing mochitest-5:
INFO TEST-UNEXPECTED-FAIL | toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Test timed out. - expected PASS

It's timing out at the test 202, you should debug there and see what's up. Maybe the popup is not closing anymore in the previous test and thus it doesn't get popupShown?
Unfortunately I can't approve the patch with such a failure and this code is complex enough that I don't have a clue about why the test is failing.
Attachment #8574380 - Flags: review?(mak77) → feedback+
Flags: needinfo?(mak77)
My apologies, as a newcomer I didn't know how to interpret the results at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fddf119b2b1f
Now I do (at least a bit better than before) ;-)

I ran the test by hand:
$ ./mach mochitest-plain toolkit/components/satchel/test/test_form_autocomplete_with_list.html
and as you had predicted, the pop-up from test 201 doesn't get cleared.

I added some debugging at the line in question 
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1509
and ran the test again. Here the output. The first value printed is "result", always 4, so RESULT_SUCCESS.
However mSearchesOngoing is negative (printed with %d), so the my suggestion mSearchesOngoing == 0 won't trigger.

I thought mSearchesOngoing is counting the number of searches that's happening, and it's an unsigned int!
I wouldn't expect to see negative numbers. Looks like it got counted down below zero.

87 INFO expecting popup for test 201
Pulling down pop-up 4, searches=-4
88 INFO popup shown for test 201
89 INFO Starting test #202
90 INFO expecting popup for test 202
91 INFO popup shown for test 202
92 INFO Starting test #203
93 INFO TEST-PASS | toolkit/components/satchlist.html | Checking form3 input
94 INFO expecting popup for test 203
Pulling down pop-up 4, searches=-6
95 INFO popup shown for test 203
96 INFO Starting test #204
97 INFO expecting popup for test 204
98 INFO popup shown for test 204
99 INFO Starting test #205
100 INFO TEST-PASS | toolkit/components/satc_list.html | Checking form3 input
101 INFO expecting popup for test 299
Pulling down pop-up 4, searches=-10

I also ran a test in the Thunderbird case.
The print I get there is:
Pulling down pop-up 2
So that's RESULT_FAILURE.

It's all a bit confusing to me. If you have a moment, can you please shed some light on the situation.
Flags: needinfo?(mak77)
This works in FF (Mochitest 5) and TB.

Please give be your opinion before I push it to the try server.

To me, it's a kludge, not decrementing mSearchesOngoing below 0. Most likely we forgot to reset it to zero at some stage, since - as can be seen in comment #58 - it's getting more and more negative.
Attachment #8579608 - Flags: feedback?(mak77)
we need to find the reason mSearchesOngoing becomes negative, we can't workaround it cause it might be hiding a worse bug.
It's possible the bug is due to the consumer not properly sending results before indicating end of the search. Or it might be a race condition in the controller.
For external consumers it might be worth to add protection to onSearchResult so that consumers will get an exception if mSearchesOngoing is zero (that means they are sending results in the wrong order).
If you could try to check when we increment and decrement mSearchesOngoing, that might be useful. Otherwise I'll try to debug that by myself in the next days.
Flags: needinfo?(mak77)
Here is some more debug, it's printed right after mSearchesOngoing is decremented (printed with %d and %u). As one can see, it's going down ;-(

I checked, the cross-reference:
http://mxr.mozilla.org/mozilla-central/ident?i=mSearchesOngoing+&tree=mozilla-central&filter=
It gets assigned once and decremented twice.

I would very much appreciate if you could debug it since you're familiar with the code. Once you've found a fix, I will try your fix in Thunderbird.

81 INFO Starting test #200
mSearchesOngoing=-1 4294967295
82 INFO TEST-PASS | toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Checking form3 input
83 INFO expecting popup for test 200
mSearchesOngoing=-2 4294967294
84 INFO popup shown for test 200
85 INFO Starting test #201
mSearchesOngoing=-3 4294967293
86 INFO TEST-PASS | toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Checking form3 input
87 INFO expecting popup for test 201
mSearchesOngoing=-4 4294967292
88 INFO popup shown for test 201
89 INFO Starting test #202
90 INFO expecting popup for test 202
mSearchesOngoing=-5 4294967291
91 INFO popup shown for test 202
92 INFO Starting test #203
93 INFO TEST-PASS | toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Checking form3 input
94 INFO expecting popup for test 203
mSearchesOngoing=-6 4294967290
95 INFO popup shown for test 203
96 INFO Starting test #204
97 INFO expecting popup for test 204
mSearchesOngoing=-7 4294967289
mSearchesOngoing=-8 4294967288
mSearchesOngoing=-9 4294967287
98 INFO popup shown for test 204
99 INFO Starting test #205
100 INFO TEST-PASS | toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Checking form3 input
101 INFO expecting popup for test 299
mSearchesOngoing=-10 4294967286
102 INFO popup shown for test 299
103 INFO Starting test #300
104 INFO expecting popup for test 300
mSearchesOngoing=0 0
105 INFO popup shown for test 300
106 INFO Starting test #301
107 INFO TEST-PASS | toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Checking form3 input
108 INFO expecting popup for test 301
109 INFO popup shown for test 301
110 INFO Starting test #302
111 INFO TEST-FAIL | toolkit/components/satchel/test/test_form_autocomplete_with_list.html | The author of the test has indicated that flaky timeouts are expect
ed.  Reason: untriaged
mSearchesOngoing=0 0
Investigated the test failure yesterday. With Marcos help we figured out that the nsAutoCompleteController does not handle search result updates correctly and we need to handle OnUpdateSearchResult() and OnSearchResult() at least somewhat differently.

This patch includes Jorg's one line change and all satchel and autocomplete tests succeed.

Jorg, can you please check if that patch still fixes the TB issue you're seeing?
Attachment #8580554 - Flags: review?(mak77)
Attachment #8580554 - Flags: feedback?(mozilla)
Attachment #8579608 - Flags: feedback?(mak77)
Attachment #8574380 - Attachment is obsolete: true
Attachment #8579608 - Attachment is obsolete: true
Thank you, Tim. The patch still fixes the issue (tested with a local build).
Thanks for the patch. It's working fine in Thunderbird.
I assume you will take care of all the other tests in Firefox.
Sorry, you already said "all satchel and autocomplete tests succeed". So it's all good.
Thank you both for testing! And thank you Jorg for your initial patch, I'll split it off so that you get the appropriate credit for the fix :)
Comment on attachment 8580554 [details] [diff] [review]
0001-Bug-1042561-Autocomplete-widget-Typed-text-in-red-de.patch

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

It looks good to me, let's try it.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +760,5 @@
>  
>  NS_IMETHODIMP
>  nsAutoCompleteController::OnSearchResult(nsIAutoCompleteSearch *aSearch, nsIAutoCompleteResult* aResult)
>  {
> +  MOZ_ASSERT(mSearches.Contains(aSearch));

please also MOZ_ASSERT(mSearchesOngoing)
I know you also do below, and that's fine, but let's be overzealous

@@ +777,5 @@
> +
> +  // If our results are incremental, the search is still ongoing.
> +  if (result != nsIAutoCompleteResult::RESULT_SUCCESS_ONGOING &&
> +      result != nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING) {
> +    MOZ_ASSERT(mSearchesOngoing);

If you add this at the top, you can remove it from here. I want to assert regardless the searchResult value, if you are notifying a result a search must be ongoing.

::: toolkit/components/autocomplete/nsAutoCompleteController.h
@@ +50,5 @@
>    nsresult ClearSearchTimer();
>    void MaybeCompletePlaceholder();
>  
> +  void HandleSearchResult(nsIAutoCompleteSearch* aSearch,
> +                          nsIAutoCompleteResult* aResult);

this code module has pointer decorators towards names (even if that sucks, imo), so please keep its current style.
Attachment #8580554 - Flags: review?(mak77) → review+
(In reply to Tim Taubert [:ttaubert] from comment #66)
> Thank you both for testing! And thank you Jorg for your initial patch, I'll
> split it off so that you get the appropriate credit for the fix :)

This is very kind, but wasn't necessary. Thanks! The idea came from Neil. I'll just take the credit for perseverance and "debugging legwork" (as Neil put it in comment #46). I consider this fix as a three-nation (UK, IT, DE) collaborative effort.
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2335401&repo=fx-team
Flags: needinfo?(mozilla)
Comment on attachment 8580554 [details] [diff] [review]
0001-Bug-1042561-Autocomplete-widget-Typed-text-in-red-de.patch

Actually, I'm the wrong guy to ask.
Flags: needinfo?(ttaubert)
Attachment #8580554 - Flags: feedback?(mozilla)
Flags: needinfo?(mozilla)
r=mak
Attachment #8580554 - Attachment is obsolete: true
Flags: needinfo?(ttaubert)
Attachment #8581469 - Flags: review+
Turns out that we run into one of the new assertions when nsAutoComplete call StopSearch() but nsFormAutoComplete doesn't correctly cancel the query. The query would intermittently (but very often on my machine) still cause handleComplete() to be called. Adding a simple check fixes the test failures.
Attachment #8581471 - Flags: review?(mak77)
nsAutoCompleteController::StartSearch() iterates mSearches and calls mSearches[i]->StartSearch(). One of the tests immediately called onSearchResult() and continued testing. mSearches.Length() was two then and we ran into our new assertions. Iterating a copy of mSearches does the trick.
Attachment #8581473 - Flags: review?(mak77)
The current try run looks a little better but there still are some test failures left to fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=376a2754abcb
Comment on attachment 8581471 [details] [diff] [review]
0003-Bug-1042561-Ensure-a-canceled-autocomplete-search-do.patch

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

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +286,5 @@
>            }
>          };
>  
> +        let query = FormHistory.getAutoCompleteResults(searchString, params, processResults);
> +        this._pendingQuery = query;

Is this solving some problem or is it just coding style?
(In reply to :aceman from comment #77)
> > +        let query = FormHistory.getAutoCompleteResults(searchString, params, processResults);
> > +        this._pendingQuery = query;
> 
> Is this solving some problem or is it just coding style?

Together with the newly added condition above this fixes the problem as described in comment #74.
(In reply to Tim Taubert [:ttaubert] from comment #78)
> (In reply to :aceman from comment #77)
> > > +        let query = FormHistory.getAutoCompleteResults(searchString, params, processResults);
> > > +        this._pendingQuery = query;
> > 
> > Is this solving some problem or is it just coding style?
> 
> Together with the newly added condition above this fixes the problem as
> described in comment #74.

Ah, processResults.handleCompletion is actually run after you create the "query" variable.
Turns out that FormAutoCompleteChild.stopAutoCompleteSearch() wasn't implemented because we can't really stop searches that were triggered that way. To prevent running into assertions we have to ignore the results though.
Assignee: mozilla → ttaubert
Attachment #8581630 - Flags: review?(mak77)
Attachment #8581471 - Flags: review?(mak77) → review+
Comment on attachment 8581473 [details] [diff] [review]
0004-Bug-1042561-Iterate-a-copy-of-mSearches-in-StartSear.patch

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

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +1111,5 @@
>    nsCOMPtr<nsIAutoCompleteInput> input = mInput;
>  
> +  nsCOMArray<nsIAutoCompleteSearch> searchesCopy(mSearches);
> +  for (uint32_t i = 0; i < searchesCopy.Length(); ++i) {
> +    nsCOMPtr<nsIAutoCompleteSearch> search = searchesCopy[i];

This needs a comment examplaining why we do that.
Let's not add more mistery to the controller :)
Attachment #8581473 - Flags: review?(mak77) → review+
Comment on attachment 8581630 [details] [diff] [review]
0005-Bug-1042561-Implement-stopAutoCompleteSearch-for-For.patch

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

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +416,5 @@
>      },
>  
>      stopAutoCompleteSearch : function () {
>         this.log("stopAutoCompleteSearch");
> +       this._lastSearchID++;

I'm not sure why we increment both when a search start and when a search ends... wouldn't the stop increment be enough by itself? Are there cases where we don't invoke stopAutoCompleteSearch? (this is a real question, I don't know this code very well).

Maybe rather than a counter this could be _pendingSearch, set to null in stopAutoCompleteSearch and to an empty object when a search begins?

So, when a new search begins, 
if (this._pendingSearch)
  this.stopAutoCompleteSearch();
let search = this._pendingSearch = {};

...

if (search != this._pendingSearch)
  return;
Attachment #8581630 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #83)
> >      stopAutoCompleteSearch : function () {
> >         this.log("stopAutoCompleteSearch");
> > +       this._lastSearchID++;
> 
> I'm not sure why we increment both when a search start and when a search
> ends... wouldn't the stop increment be enough by itself? Are there cases
> where we don't invoke stopAutoCompleteSearch? (this is a real question, I
> don't know this code very well).

I don't know the code too well either so I thought better be safe than sorry.

> Maybe rather than a counter this could be _pendingSearch, set to null in
> stopAutoCompleteSearch and to an empty object when a search begins?

Yeah thought about doing that too, ended up with the counter. Can change it though, don't have a strong opinion here.
Attachment #8582342 - Flags: review?(mak77)
Attachment #8582342 - Flags: review?(mak77) → review+
Any chance to get this into mozilla38 or even 31 (if there is another TB 31.x release planned)?
It's quite an annoying bug for Thunderbird users.
I'm honestly not thrilled of making these changes ride the trains. For sure not for 31. 38 could be done I guess, since the alternative is to wait until the next ESR.
Yes, please for 38 then :-)
Yes, please don't make us wait another year to get a fix for this annoying and very embarrassing (think: the boss) issue. It has been bad enough 'riding this train' in the 31 release with all of the massively user impactful addressing bugs.
Tim, can we get a coalesced patch please?
And actually, would be nice to have a push to Try on the top of Aurora, if things don't look green there it might be expensive to fix them.
Flags: needinfo?(ttaubert)
+1 to getting a fixed released ASAP - 31.6? After the other bug/s related to the actual wrong address getting selected from the dropdown list, users have lost confidence. I get questions about this "address in red" bug every day - from the boss at the top all the way down. Frankly it is embarrassing that such an obvious UI bug has been there for so long with no fix.
Users think that Thunderbird is trying to tell them something is wrong. Putting something up erroneously in red in a part of the UI that is used many times every day could almost count as a security bug - it reduces security by forcing users to ignore stuff in red. I certainly do not like educating users to ignore red messages, and in this case users need to be able to know that the email address in red means they are sending to somewhere new/unusual.
I think we can't justify uplifting this to 31 given that it was around for so long and ESR 38 is around the corner. I'll push to try and see how that goes and will request an uplift to 38 if all goes well.
(In reply to Tim Taubert [:ttaubert] from comment #97)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e00928dad68a

The try run is good. It looks bad but only because I did run some test suites that aren't enabled on Aurora and can thus be ignored. M-e10s and JP tests don't run on Aurora. The Windows M3 failures are expected (bug 1146061) and you can see them on current Aurora tip as well.
Comment on attachment 8583036 [details] [diff] [review]
Coalesced patch for Aurora (38)

Approval Request Comment
[Feature/regressing bug #]: Not sure, longstanding bug.
[User impact if declined]: Autocomplete in TB is misbehaving as described. We're sure that it affects Firefox performance/reliability too but it's hard to track.
[Describe test coverage new/current, TreeHerder]: Has a lot of tests all over the place and they're passing even the newly added assertions.
[Risks and why]: Risk should be low. We mostly fixed some misbehaving code so that it doesn't run into the newly added assertions.
[String/UUID change made/needed]: None.

We'd like to uplift this to Firefox 38 mainly so that it's included with the ESR release.
Attachment #8583036 - Flags: approval-mozilla-aurora?
Comment on attachment 8583036 [details] [diff] [review]
Coalesced patch for Aurora (38)

OK, we will still have time to see if it has side effects, and, if it is the case, backout this patch
Attachment #8583036 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Can this fix be pushed for Thunderbird 31 (or 38), even if it is not pushed for Firefox?

It has a much, much more negative impact for Thunderbird users than Firefox, I suspect.
(In reply to Charles from comment #101)
> Can this fix be pushed for Thunderbird 31 (or 38), even if it is not pushed
> for Firefox?

Short answer is: No. TB is based on Mozilla core and does not administer separate patches.
Anyway, it already landed on mozilla38, so it will be fixed in TB 38. So fingers crossed that it doesn't do any harm ;-)
Thanks to all working on and implementing the fix!

Can someone translate for those of us unfamiliar with the release process - can you provide an estimate of when 'TB 38' might be released?
This is publicly available information:
https://wiki.mozilla.org/Releases
https://wiki.mozilla.org/RapidRelease/Calendar#Future_branch_dates

Beta coming out at the end of March, release six weeks later, mid May ... if there are no delays.
(In reply to Tim Taubert [:ttaubert] from comment #72)
> Created attachment 8581469 [details] [diff] [review]
> 0001-Bug-1042561-Autocomplete-Typed-text-in-red-despite-r.patch

Hi. I've bisected a regression to this patch - Bug #1151764. Could you retest on current thunderbird trunk?

When I do:
cd ~/build/comm-central/mozilla
hg update -r235318
cd ~/build/comm-central/
./mozilla/mach build
./mozilla/mach run
Then bug #1151764 does not manifest

But with:
cd ~/build/comm-central/mozilla
hg update -r235319
cd ~/build/comm-central/
./mozilla/mach build
./mozilla/mach run
# Bug #1151764 does manifest
Flags: needinfo?(ttaubert)
Sorry, I have no TB checkout and don't know a lot about TB. Would be great if someone else could step in and help here.
Flags: needinfo?(ttaubert)
Sorry, can't help.
I tried on a local build of TB 40 (Daily) with the address book enclosed in bug 1151764 on Windows 7.
With every keystroke, irrespective of how long I wait, I get the correct feedback.
Removing tracking-thunderbird38 since this is fixed there, and I can't set a status-thunderbird38 flag.
This problem has been present since the last good release the release of  24.8.1 and has been present in all releases through the current 31.07; it's been almost 8 months and still there is no definite release date for a fix.
I guess I do not understand the "status: RESOLVED FIXED"
This is fixed in TB38. Try the latest beta of TB38 or wait for the official release within the next one to two weeks. Find the beta here: https://www.mozilla.org/en-US/thunderbird/channel/
Blocks: 1169422
Depends on: 1151764
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: