395066 - Hang when trying to clear browsing history

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

When trying to clear my private data, current trunk Firefox hung.
It took long enough to be annoyed by it (>10s), so I killed the Firefox process.

It happened even when I only had the "Browsing History" option checked.

I had a lot of history items in there.
With the SQLite data browser I got40300 for the amount of moz_historyvisits.

And my places.sqlite file had 14MB as size, in case that matters.

Comment 1

[Jo Hermans]

The trunk is currently very slow when clearing history, just wait until it's finished. Since we're now storing history for 180 days by default, this has become much annoying. It will takes minutes ! And because there's no progress dialogbox, people will assume that Firefox is hanging.

Too bad that Sqlite doesn't offer a "truncate table" command :-(

Maybe as a workaround (if we don't find ther cause), we can drop the tablr and recreate it ?

Comment 2

[Martijn Wargers (dead)]

That sounds bad, and I think this will be something that people will encounter.

Comment 3

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007090614 Minefield/3.0a8pre ID:2007090614

I have a 45.1MB places.sqlite file and a 45 day browsing history. I made a new profile, copied my usual places.sqlite to it, started firefox up with the new profile, Tools > Clear Private Data, and chose to just clear my browsing history. It took my AlthonXP2400 on win2k _1hr32min_ of 99% CPU usage to clear the browser history.

Comment 4

[(not reading, please use [e-mail] instead)]

thanks (as always) to martjin, jo and steve.  I'll take a look into this ASAP.

Comment 5

[Dietrich Ayala (:dietrich)]

Here's the breakdown of time in ExpireItems() in seconds, when clearing history in my places.sqlite of 25mb:

FindVisits: 0
EraseVisits: 1
EraseHistory: 12
EraseFavicons: 0
EraseAnnos: 36

Comment 6

[Jo Hermans]

For EraseAnnotations, can't we use the same optimization as in bug 386711, issuing a single sql-statement with an in-expression (see EraseVisits) ?

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> For EraseAnnotations, can't we use the same optimization as in bug 386711,
> issuing a single sql-statement with an in-expression (see EraseVisits) ?
> 

Yes, that's what I'm currently writing :)

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: fix 1 - fixes EraseAnnotations

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #8)
> Created an attachment (id=280125) [details]
> fix 1 - fixes EraseAnnotations
> 

this brought EraseAnnotations from 36 secs to <0 secs w/ my profile.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: fix 1 - fixes EraseAnnotations (v2)

removes unnecessary anno svc.

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 280127 [details] [diff] [review]
fix 1 - fixes EraseAnnotations (v2)

r=sspitzer, but 

1) don't check in your timing / printf code.

2) use nsCString instead of nsCAutoString, as mentioned over irc, because, for lots of ids

nsAutoString / nsCAutoString- derived from nsString, a string which owns a 64 code unit buffer in the same storage space as the string itself. If a string less than 64 code units is assigned to an nsAutoString, then no extra storage will be allocated. For larger strings, a new buffer is allocated on the heap.

Comment 12

[(not reading, please use [e-mail] instead)]

I think dietrich also has a fix for EraseHistory().

dietrich, do you think this will make m8?

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: fix 1 - fixes EraseAnnotations (v3)

cleaned up patch, carrying over r=sspitzer.

requestion 1.9 approval: this fixes the bug to a large extent. i'm still working on fixing EraseHistory(), but as that's the lesser problem, we shouldn't block on getting this in for M8.

Comment 14

[Dietrich Ayala (:dietrich)]

checked in the fix to annotations. i'm still working on a fix for deleting moz_places records, so leaving this bug open. reporter(s), please comment with how long it takes after this fix.

Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/places/tests/unit/test_expiration.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_expiration.js,v  <--  test_expiration.js
new revision: 1.7; previous revision: 1.6
done

Comment 15

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007091002 Minefield/3.0a8pre

With a new profile and my existing places.sqlite (45.1MB, 45 day history), Clear Private Data took 4m12s to clear my history - a vast improvement from over an hour and a half!

Comment 16

[Marcia Knous [:marcia]]

Litmus Triage Team: further discussion is needed about the best way to create a Litmus case around this. Perhaps if we have a dirty profile from my profile project that can be accessed, it could be worked.

Comment 17

[Marco Bonardo [:mak]]

as done in the other functions, you could add also into eraseAnnotation an early return to avoid the query if there are no ids to delete

if (placeIds.IsEmpty())
  return NS_OK;

and probably nsCAutoString in other erase functions could be converted to nsCString per comment #11

On my 28MB places.sqlite clear history (only browsing history) took about 1,55 minutes

Comment 18

[Marco Bonardo [:mak]]

Attachment: attempted patch to erasehistory (and cleanup)

i've tried with different queries on erasehistory, the only one i got an advantage is this, i moved from 1,55 minutes to 1,35 minutes, so if someone wants to give it a try... other queries did not move the timing, even if they were more compact

this does also the cleanup on autostring and early return

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 280878 [details] [diff] [review]
attempted patch to erasehistory (and cleanup)

this looks good, and passes the unit tests. thanks Marco!

Comment 20

[Marco Bonardo [:mak]]

does this need other reviews? 
if not this should be checkin-needed

Comment 21

[Reed Loden [:reed]]

Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.17; previous revision: 1.16
done

Comment 22

[Tracy Walker [:tracy]]

Things such as hangs, crashes and other general nastiness are implicit failures for any test case.  

The test case for clearing browser history is already in Litmus: https://litmus.mozilla.org/show_test.cgi?id=4141

also marking verified per comments stating vast clear history improvement.

Comment 23

[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