431548 - Tagging history item takes a long time without progress indication (slight hang / freeze of UI)

Status: VERIFIED FIXED

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

Description

[Damian Shaw [Quan]]

On:

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008042906 Minefield/3.0pre ID:2008042906

STR:

1. Go to the Places Library
2. Go to the History section
3. Click on an unbookmarked history item
4. In the bottom pane, type some tag in to the tag section
5. Click on the unbookmarked history item
6. Firefox goes weird! Including temporarily freezing up.

Comment 1

[Tracy Walker [:tracy]]

wierd?  are you seeing a flickering bug?  if so this is likely a dupe.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

What's happening is that it's creating a bookmark and adding tags to it for that history item, and I bet that operation is taking time.

This chalks up to "we have no good progress indication" as well as a bit of "why should that operation take a lot of time"?

Doubt that this will be a common operation, and it's just sucky not crashy, so don't think it blocks.

Comment 3

[Damian Shaw [Quan]]

(In reply to comment #1)
> wierd?  are you seeing a flickering bug?  if so this is likely a dupe.
> 

Well, for example, lots of elements in my Bugzilla report page got highlighted in a blue rectangle. 

Comment 4

[Damian Shaw [Quan]]

I can't reproduce that anymore :/

Guess it's just what Beltner says :-). Still, I'm on a pretty fast system and it comes off to me as Firefox totally freezing up (although yeah, I get control back 10 - 15 seconds later). Can't imagine what it might be like on slow computers.

Comment 5

[Marco Bonardo [:mak]]

yes the problem is probably not having a progress indicator but not taking a century for doing simple things like adding a tag... last days patch (flickering related) could have mitigated, but leave this open for further investigation on tag adding perf

Comment 6

[Damian Shaw [Quan]]

Given a focus on places performance and tagging features in 3.1, I'm nominating it for blocking, though I'd understand if it's only wanted.

Comment 7

[Dietrich Ayala (:dietrich)]

Could be mitigated or even resolved by bug 432706, and helped even more by the fsync work in 3.1.

Marking blocking, will re-evaluate after more perf work lands.

Comment 8

[Marco Bonardo [:mak]]

see also bug 460830, Tags has a wrong history observer.

Comment 9

[Marco Bonardo [:mak]]

ok, i can definately reproduce this, notice that history is a query node, so when you change something the full query is refreshed (and the full tree rebuilt), so for example, how much time does take the library to open history tree? at least that time will be needed to do the update.
Hwv another strange thing is that after i've added the tag, if i move to a bookmarks folder and i delete a bookmark, or tag a bookmark it takes a long time, like if history tree would be rebuilding even if it's not shown.

Comment 10

[Marco Bonardo [:mak]]

and ::Refresh() for "Tags" is called a bunch of times when adding a single tag

Comment 11

[Marco Bonardo [:mak]]

what i think is that when adding/removing a tag we generate a bunch of observers calls (onItemAdded/Removed/Changed) that are observed by open queries with a bookmarks observer, so for example the left pane and Tags. Being those queries, every call causes a Refresh() that requery history, being history really big we slowdown rebuilding the tree. The callstack at every call to nsNavHistoryQueryFolder::Refresh() seems to confirm that.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch

unless we kill a bunch of notifications or revise completely the observers system i think we could batch tagging and untagging so we will rebuild the History tree only onEndUpdateBatch. The main problem (as always) is that tags are bookmarks, every tag change fires up item notifications.

Comment 13

[Dietrich Ayala (:dietrich)]

maybe i'm missing something, but notifications are still sent inside batches. so, while we should take this patch since it puts all the db activity in a transaction, it doesn't solve the problem you discuss above.

in order to suspend refreshing and live-updating inside a batch, we need the patch on bug 432706.

another problem with your previous comment is that the History tree shouldn't be paying attention to bookmark notifications, unless it's QUERYUPDATE_COMPLEX_WITH_BOOKMARKS (which it seems like the History tree wouldn't be, right?).

Comment 14

[Dietrich Ayala (:dietrich)]

(In reply to comment #13)
> in order to suspend refreshing and live-updating inside a batch, we need the
> patch on bug 432706.

never mind this, i see you set it as blocking this bug.

Comment 15

[Marco Bonardo [:mak]]

(In reply to comment #13)
> maybe i'm missing something, but notifications are still sent inside batches.

the strange thing is that doing this i don't see all of them while i only see endupdatebatch calls... i'll go back to the debugger to see exactly what happens.

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to comment #13)
> another problem with your previous comment is that the History tree shouldn't
> be paying attention to bookmark notifications, unless it's
> QUERYUPDATE_COMPLEX_WITH_BOOKMARKS (which it seems like the History tree
> wouldn't be, right?).

per irc, it's the Tags query that is causing this, not History.

Comment 17

[Marco Bonardo [:mak]]

do you want me to strip patch to a new bug?

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 352739 [details] [diff] [review]
patch

r=me

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/c368f6264e50

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/833dc6994cb0

Comment 21

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre

Comment 22

[Gervase Markham [:gerv]]

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