55293 - Pages in history don't expire after x days while mozilla is running

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Gilles Durys]

Build Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m18) Gecko/20001004.

I have set the delay for pages to expire in history to 9 days.
But it seems that nothing is done : I still have pages from 18/09 in history
(last time I manually deleted the history file)

Comment 1

[Radha on family leave (not reading bugmail)]

Global history issues. Over to rjc.

Comment 2

[Claudius Gayle]

confirmed with 2000101608 builds. yes this is on the branch too.
nominating for rtm although i kow it'll get minused. I need this
on the radar as part of a larger history issue.

cc: sairuh just so you know here's a pref that doesn't work.

Comment 3

[don]

Robert, isn't this your bug?

Comment 4

[sairuh (rarely reading bugmail)]

my/claudius' guess is that this occurs on all platforms --but we need to
doublecheck just to be sure...

Comment 5

[Daniel Veditz [:dveditz]]

I can't see the PDT accepting this at this point :-(

Comment 6

[R.K.Aa.]

*** Bug 61135 has been marked as a duplicate of this bug. ***

Comment 7

[Alec Flett]

Mine!

Comment 8

[David :Bienvenu]

Alec, I have a fix for this, which I'll attach here.

Comment 9

[David :Bienvenu]

*** Bug 61994 has been marked as a duplicate of this bug. ***

Comment 10

[David :Bienvenu]

Attachment: proposed fix

Comment 11

[David :Bienvenu]

Attachment: proposed fix, part II (prefs)

Comment 12

[David :Bienvenu]

Alec, I'm handing these patches off to you, if you don't mind. I know you're
doing other stuff in history, so you can merge this stuff in. Note that we need
a default for the expiration limit, as far as I can tell. The proposed fix is
less than ideal in a number of ways, so feel free to improve on it :-) Since we
seem to do a commit of history whenever we close a window, we need to be careful
not to do compress commits too often, since they are more expensive than
incremental commits.

Comment 13

[Alec Flett]

sr=alecf on that patch - thanks david
I actually want to fix this in a slightly different way by adding timers, which
would fire every hour or so, but I like your refactoring.. so when you check
this in, leave this bug open, and I'll add timers.

Comment 14

[David :Bienvenu]

Waterson, can I get an r= on this?

Alec, if you expire based on timers, you should pass in true for the
notification parameter (and test it, of course :-) I took the notification stuff
from the clear history routine, so it should work fine, but I didn't try it ) 

Comment 15

[Alec Flett]

I'm attaching a slightly updated patch that observes the pref changing.

Comment 16

[Alec Flett]

Attachment: tweak to proposed fix which observes prefs changing

Comment 17

[Alec Flett]

david: I took your patch and ran with it, so I'll take the bug back if I can get
an r= from you :)

waterson - if you can sr= my updated patch, that would rock.

Comment 18

[Chris Waterson]

- Why do you need to store the file size as a member variable? Can't you just
ask for it when you need it? (And if you _do_ need it as a member variable, I
didn't see where you updated it...shouldn't you do that after a commit?)

- You should #ifdef the ``mork doesn't compute shouldCompress right'' code so it
can be easily removed later.

- Could you share redundant code between ExpireEntries() and RemoveAllPages()?
For example, make a RemoveIf() method that takes a predicate that tests whether
or not the row should be nuked?

- You should make nsGlobalHistory `support weak references' so that the observer
service doesn't pin history.

Comment 19

[David :Bienvenu]

r=bienvenu for your stuff, Alec, modulo Chris's comment about the observer service.

I cache away the file size so I don't have to duplicate or factor out the code
that gets an nsIFile for the global history db. Perhaps we could cache away the
nsIFile in the global history, or factor out the code. Can you do that, Alec?

But, N.B., if an when we remove the #ifdef code, we would remove the caching of
the file size code too, since it would no longer be needed (i.e., it could be
part of the #ifdef).

We could share the common code between ExpireEntries() and RemoveAllPages(),
though it would either involve factoring out some of the common code (the
easiest thing to do), or adding a RemoveIf() predicate. Adding a RemoveIf
predicate is probably more trouble than it's worth, IMO, whereas factoring out
the common code could be used when we implement actual delete of selected
history entries.

Comment 20

[Alec Flett]

(oops, I wrote this & had a mid-air collision, so some of this matches up with
what bienvenu just wrote)

re: sharing more code between removeallpages/expireentries - I noticed this too,
and will address that in my timer patch, which will come after this goes in
re: weak refs - good call... I'll do that
re: file size - it looks like we basically throw out the filename after
startup.. at the moment, the compress only happens on shutdown though, so I
think david's loose approximation is fine until I get timers in there. (The
worst that can happen is a situation where your history starts out small, but
then you do alot of surfing in one session, and the db won't be compressed until
the next time you startup & shutdown the browser)

I'd rather land this in stages instead of one giant landing..
Since we won't be marking this fixed yet, how does the patch plus the
 observer stuff?

Patch forthcoming

Comment 21

[Alec Flett]

Attachment: updated to support weak references

Comment 22

[David :Bienvenu]

r=bienvenu, and Alec's right about the loose approximation stuff - eventually,
the ship will right itself.

Comment 23

[Chris Waterson]

sr=waterson

Comment 24

[Alec Flett]

thanks. the updated patch has been checked in. Next stop: more refactoring, and 
timers.

Comment 25

[Daniel Bratell]

And it was backed out by cls 20:39:

"Backing out alecf's changes that were causing a crash on startup on unix builds"

Comment 26

[Alec Flett]

argh. dunno what went wrong.. investigating

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

On the basis of the 2000-12-11-21 build alone, nsGlobalHistory::ExpireEntries
jumped to #2 on the talkback list for the past week.  Stack trace:

         nsGlobalHistory::ExpireEntries
[d:\builds\seamonkey\mozilla\xpfe\components\history\src\nsGlobalHistory.cpp
line 1840]
         nsGlobalHistory::CloseDB
[d:\builds\seamonkey\mozilla\xpfe\components\history\src\nsGlobalHistory.cpp
line 1930]
         nsGlobalHistory::~nsGlobalHistory
[d:\builds\seamonkey\mozilla\xpfe\components\history\src\nsGlobalHistory.cpp
line 273]
         nsGlobalHistory::`scalar deleting destructor'
         nsGlobalHistory::Release
[d:\builds\seamonkey\mozilla\xpfe\components\history\src\nsGlobalHistory.cpp
line 306]
         nsGlobalHistoryConstructor
[d:\builds\seamonkey\mozilla\xpfe\components\build\nsModule.cpp  line 44]
         nsGenericFactory::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsGenericFactory.cpp  line 48]
         nsComponentManagerImpl::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsComponentManager.cpp  line 1202]
         nsComponentManager::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsRepository.cpp  line 82]
         nsServiceManagerImpl::GetService
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp  line 345]
         nsServiceManager::GetService
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp  line 560]
         nsGetServiceByCID::operator()
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp  line 46]
         nsCOMPtr_base::assign_from_helper
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp  line 66]
         nsBrowserContentHandler::GetDefaultArgs
[d:\builds\seamonkey\mozilla\xpfe\browser\src\nsBrowserInstance.cpp  line 2312]
         LaunchApplication
[d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp  line 362]
         startupPrefEnumerationFunction
[d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp  line 491]
         nsPref::EnumerateChildren
[d:\builds\seamonkey\mozilla\modules\libpref\src\nsPref.cpp  line 1379]
         HandleArbitraryStartup
[d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp  line 544]
         DoCommandLines
[d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp  line 591]
         main1  [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp
line 1002]
         main   [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp
line 1263]

This is from n.p.m.crash-data.  The internal talkback stuff probably has much
more detail...

Comment 28

[David :Bienvenu]

Alec and I have fixes for all the problems.

Comment 29

[Alec Flett]

not a crash bug (it only was for about 8 hours), removing "crash" keyword and
error location

Comment 30

[Viswanath Ramachandran]

nav triage team: not a beta stopper. 

Comment 31

[Ben Bucksch (:BenB)]

Vishy, please note that bug 61994 is a dup of this one. Please read the other
bug to evaluate the severity of this one.

Comment 32

[Alec Flett]

a slight clarification here, for those listening: pages now expire after X days,
but they only do it on shutdown.. the basic functionality is there..

 so the bit about using timers, which I still have to fix, is not a beta stopper..

Comment 33

[Ben Bucksch (:BenB)]

Oh, OK, sorry.

Comment 34

[Asa Dotzler [:asa]]

This should not be targetted on mozilla 0.9 if Netscape has determined that
alecf will not be working on it between now and nsbeta1.
 Netscape nav triage team, please set TM to "---" or "Future" when you nsbeta1-
a bug.  Thanks.

Comment 35

[Koike Kazuhiko]

Somebody already fixed this bug.

Comment 36

[Alec Flett]

no, this bug is not completely fixed. we need to periodically expire pages while
the browser is up. Read above comments, re: timers.

Comment 37

[Paul Chen]

Changing summary to "Pages in history don't expire after x days while mozilla is
running" to better reflect current state of bug. Resetting target milestone.

Comment 38

[Alec Flett]

to the Future... and beyond!

Comment 39

[Alec Flett]

*** Bug 78915 has been marked as a duplicate of this bug. ***

Comment 40

[edcjones]

This old bug is still alive and kicking in 0.9.2. I use RedHat linux 7.1 on a PC.

Comment 41

[Alec Flett]

argh.. take two at reassigning history bugs to new owner

Comment 42

[R.K.Aa.]

*** Bug 95616 has been marked as a duplicate of this bug. ***

Comment 43

[edcjones]

I think there may be two different bugs here. One of them seems to been fixed in
0.9.5.

Bug 1. The bug as originally described: "I have set the delay for pages to
expire in history to 9 days. But it seems that nothing is done"

Bug 2. The bug described by the current summery: "Pages in history don't expire
after x days while mozilla is running"

I have had the first form of the bug: nothing ever expires. If I 

Go to Edit /Preferences / Navigator / History and set "Remember visited pages
for the first ___ days" as 1.

Visit an url, causing its color to change.

Wait two days.

The url still has the visited color.

In 0.9.5 _this_ bug is gone.

Comment 44

[Blake Ross]

Attachment: patch

Comment 45

[Blake Ross]

alec, I think this is just that same bug again. can you sr?

Comment 46

[Alec Flett]

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

sr=alecf

Comment 47

[Blake Ross]

fixed.