Closed Bug 469422 Opened 12 years ago Closed 12 years ago

tag autocomplete disappears on every other character that is typed

Categories

(Firefox :: Bookmarks & History, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: cmtalbert, Assigned: dietrich)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 2 obsolete files)

1. Install my bookmarks into a build of 3.1 or 3.2pre
2. Try to tag something with the "printing" tag
3. Note that the autocomplete window will display on the "p", disappear on the "r", display on the "i", disappear on the "n" etc.

There are many of my tags that have this behavior.  Some of them do not.  I don't know why, and I don't know what started this.  This is a profile that has been in existence since early 3.0 days and has been upgraded by being on the nightly trunk builds forever.

I'll attach an export of my bookmarks and a screen cast of the problem (quicktime movie, I apologize).
Here is the screen cast of the issue: http://people.mozilla.org/~ctalbert/autocomplete.mov (quicktime format, again I apologize)
Happens in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081209 Minefield/3.2a1pre

and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081212 Shiretoko/3.1b3pre
Summary: Autocomplete disappears on every other character that is typed → tag autocomplete disappears on every other character that is typed
Clint, which bookmarks we have to load? Looks like you forgot to attach them to this bug.

The feature was added by bug 415960. So adding dependency.
Blocks: 415960
I was affected by this issue each time when I wanted to add a tag to a bookmark in the last weeks. Is there any chance to get it fixed for 3.1?
Flags: blocking-firefox3.1?
I can't reproduce this on OSX, though I don't think I have as many tags. Do we know if this is a number of tags issue or a recent regression?

Leaving the nomination until Dietrich can look at it, but I think this is a minus unless we can find out under what circumstances it happens and prove that it's a deeper issue. Right now it looks edge and wanted-not-blocking.
I'm also not able to reproduce, so blocking-. Renom and re-open if you still see the problem.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: blocking-firefox3.1? → blocking-firefox3.1-
Resolution: --- → WORKSFORME
re-opening, i didn't notice that whimboo's comment was so recent.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Dietrich, can you reproduce it with Clints json file? If not, shall we help out somehow?
I received clint's db, and am able to reproduce. re-blocking on this.
Flags: blocking-firefox3.1- → blocking-firefox3.1+
Priority: -- → P3
Assignee: nobody → dietrich
Do we think Clint's DB is typical? I haven't found anyone else on OSX with this issue, even amongst heavy taggers. I'm guessing (but am not sure) that it's an issue with the time required to run the tag autocomplete query when additional characters are added. Adding sdwilsh and mardak to see if they can look for optimizations in that query.
Flags: blocking-firefox3.1+ → blocking-firefox3.1?
Priority: P3 → --
It'd probably be good to block on this until we understand the problem (or have it timeout if we can't understand it).  Understanding it will then let us evaluate if it should really block or not.
Sure, OK. Adding [dietrich investigating]. I'm happy, tbh, to treat the lack of dupes and inability to get anyone else to repro as evidence enough, but more rigor is always appreciated! :)
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P2
Whiteboard: [dietrich investigating]
I can reproduce on my database now, not just Clint's. On Clint's db it only occurs if the tag begins with a letter after H. On my db it occurs for any tag that begins with a letter after C.

So far, looks like nsIAutoCompleteSearch.stopSearch() is called at some point while chunking the search results.
(In reply to comment #11)
> I'm guessing (but am not sure) that it's an
> issue with the time required to run the tag autocomplete query when additional
> characters are added.

it uses a cached array of the user's tags instead of querying the db directly, so this should not be a problem.
the AC widget is receiving popuphiding, then calling controller->stopSearch, which calls tagging's stopSearch... only for every other letter, starting only after a certain point in the tag set.

if you type a few characters very quickly, it doesn't occur. however, typing slowly again after that will cause it to reoccur.

couple of things to note:

- it's not reproducible at all for that first bit of the tag set, regardless of speed of typing

- i've tried chunking the AC results in much smaller numbers, and the behaviour starts in exactly the same spot in the tag set
Attached patch workaround (obsolete) — Splinter Review
brutal workaround. still haven't found out what's closing the popup. always occurs after the first yield, regardless of chunk size.
see https://bugzilla.mozilla.org/show_bug.cgi?id=455555#c35, i suspect could be the same cause
It's a bit different but probably the popup doesn't open again.
Status: REOPENED → ASSIGNED
Attached patch test fix and better workaround (obsolete) — Splinter Review
the attached patch works around this bug by searching all tags and sending the results back in a single chunk. i tested this with 1000 tags. the bug was gone, and things were not noticeably laggy. this is likely fine to do synchronously, as the tag list is cached in memory, no db roundtrip required. in fact, this feels smoother than before, since the flickering is gone (from the chunking). 
i chose not to entirely remove the chunking mechanism, as we do want to re-enable it once these issues are resolved, so it's commented out for now.

while troubleshooting this bug, i noticed that each time we yield, another result gets added to the searches being managed by the controller, instead of replacing the existing one. i noticed this because when adding a bunch of non-matching tags to the database (thereby forcing chunking), the test failed in several places. this, i still do not understand, and will file a followup. the non-chunking approach in the attached patch effectively fixes this.
Attachment #363373 - Attachment is obsolete: true
Attachment #365390 - Flags: review?(mak77)
Whiteboard: [dietrich investigating] → [has patch][needs review marco]
Comment on attachment 365390 [details] [diff] [review]
test fix and better workaround

>diff --git a/toolkit/components/places/src/nsTaggingService.js b/toolkit/components/places/src/nsTaggingService.js
>--- a/toolkit/components/places/src/nsTaggingService.js
>+++ b/toolkit/components/places/src/nsTaggingService.js
>@@ -545,13 +545,24 @@
>         }
>     
>         ++i;
>+
>+        /* TODO: for each yield, and each new result passed to the AC
>+         * listener, a new result is added, causing invalid matchCount
>+         * values for each result added after the first.

result and added are repeated so much that make this hard to read and understand.
is this saying "for each yield we pass a new result to the autocomplete listener. The listener will invalid matchCount values for each result added after the first."? so that also needs to explain what issue is caused by that and point to the bug tracking such issue.

>+         *
>+         * so as a workaround, all tags are searched through, basically
>+         * making this syncronous for the time being.

nit: synchronous
"for the time being" => "till the above issue is fixed"


while there, i can't understand why immediately after that we do:

    function driveGenerator() {
      if (gen.next()) { 
        var timer = Cc["@mozilla.org/timer;1"]
          .createInstance(Components.interfaces.nsITimer);
        self._callback = driveGenerator;
        timer.initWithCallback(self, 0, timer.TYPE_ONE_SHOT);
      }
      else {
        gen.close();	
      }
    }
    driveGenerator();

instead of a simple and far more compact:

  while(get.next());
  get.close();

or a while(true) get.next(); with a catch for StopIteration if you don't like the trailing ";".
The generator example uses setTimeout but looks like is done to be generic, and show to the implementer how to pause between loops.

apart that, i think this is a good way to solve the issue till ac listener receives some love, and being sync here is not a big deal since all tags are cached.
Attachment #365390 - Flags: review?(mak77) → review+
Whiteboard: [has patch][needs review marco] → [has patch]
s/get/gen/ clearly, sorry for typo
Attached patch for checkinSplinter Review
Attachment #365390 - Attachment is obsolete: true
Whiteboard: [has patch] → [has patch][ready for checkin]
http://hg.mozilla.org/mozilla-central/rev/85cad64285f9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][ready for checkin]
Target Milestone: --- → Firefox 3.2a1
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre Ubiquity/0.1.5 ID:20090310030639 and a copy of my daily profile.

How long should it bake on trunk before we can get it into 1.9.1 branch?
Status: RESOLVED → VERIFIED
OS: Mac OS X → All
Hardware: x86 → All
Does the updated test cover this whole issue or do we need a mochitest/litmus test?
Flags: in-testsuite?
the updated test does not cover this specific issue, that is mostly an autocomplete widget issue. I'm not sure due to the difficulties to reproduce this needs a specific mochitest/litmus test.

and we'll push this to branch soon, there's no need to wait further.
With a pre-populated places.sqlite file it is reproducible for me each time. We would only need such a simple testcase. Dietrich, what about the testcase you got from Clint?
--> P1, as this bug will require the wider feedback of a beta release or is of sufficient complexity that we should be looking at it sooner, not later.
Priority: P2 → P1
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090312 Shiretoko/3.1b4pre ID:20090312034554
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.