312036 - history.dat contains entries deleted from the "date and site" view

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Boris Glawe]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.7.12) Gecko/20050922 Fedora/1.0.7-1.1.fc4 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.7.12) Gecko/20050922 Fedora/1.0.7-1.1.fc4 Firefox/1.0.7

If I visit for example www.google.com and remove this site from the history
sidebar afterwards , the URL won't be visible in the address bar or in any
history list as expected.

BUT it can still be found by greping the users configuration files. Use this
command for validation

# grep -ri google ~/.mozilla/firefox




Reproducible: Always

Steps to Reproduce:
1. visit an arbitrary site
2. remove this site from the history sidebar
3. grep your configuration directory with the command specified above

Actual Results:  
site is still visible in history.dat and other files

Expected Results:  
site should not exist any longer in any files (except cache files maybe)

I've reported this bug long before firexox existed with the mozilla browser.

They have resolved the problem very quickly. Unfortunately I don't remember how
(I think is was something like a "compressed commit" or "compressed submit"  or
something like that)

By the way: in moziilla's case the problem only existed when removing some
entries explicitly in the sidebar or in the history window. It didn't happen,
when pressing "clear all".

Comment 1

[Jesse Ruderman]

The bug you filed before is bug 234700.  That bug is being marked as
fixed-aviary1.0, but you say you're still seeing the bug with Firefox 1.0.7.  Hmm.

Comment 2

[Boris Glawe]

I've done a "clear all" now (my history of about 3 months is gone now :-(  ).
The entries not visible any more. So it's seems to be exactly the same behaviour
as I've reported in bug https://bugzilla.mozilla.org/show_bug.cgi?id=234700 .

I can definitely still see this but.

Comment 3

[Daniel Veditz [:dveditz]]

Confirming. Should we reopen bug 234700? Ben's checkin from 2005-05-30 is
definitely in there (as well as mconnor's update in August, rev=1.33.2.1.2.7).
Darin moved things around in 1.33.2.1.2.8 but it was moved intact.

When I view history by date or site the removals _do_ happen. When I view by
"date AND site" (the most useful option in Firefox's stunted history panel) then
the data appears to remain in history.dat unless I clear all or some other
action that causes a compress-commit, such as deleting another site under one of
the working views.

Can't call this a regression, I've tested back to 1.0.2 and I think this case
was never fixed. It's probably clearer after all this time to use this bug
rather than reopen the older bug to cover the partial fix.

Comment 4

[Daniel Veditz [:dveditz]]

Bug 234700 put the compress-commit fix in nsGlobalHistory::RemovePagesFromHost,
which is called explicitly in the suite:
http://lxr.mozilla.org/mozilla/source/xpfe/components/history/resources/history.js#240

Firefox does an nsGlobalHistory::Unassert, which calls
nsGlobalHistory::RemoveMatchingRows, bypassing the commit in RemovePagesFromHost
(which calls RemoveMatchingRows to do the work).

We can fix this by moving the Commit() into RemoveMatchingRows. If we do we'll
do a couple extra compresses shutting down (called by ExpireEntries).

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: Compress commit when removing by site

Comment 6

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

Comment on attachment 199201 [details] [diff] [review]
Compress commit when removing by site

>@@ -1235,16 +1232,13 @@ nsGlobalHistory::RemoveAllPages()
>   nsresult rv;
> 
>   rv = RemoveMatchingRows(matchAllCallback, nsnull, PR_TRUE);
>   if (NS_FAILED(rv)) return rv;
>   
>   // Reset the file byte order.
>-  rv = InitByteOrder(PR_TRUE);
>-  if (NS_FAILED(rv)) return rv;
>-
>-  return Commit(kCompressCommit);
>+  return InitByteOrder(PR_TRUE);

This means there won't be a commit after the InitByteOrder call, which seems a bit odd -- we could save a history.dat that doesn't have a byte order.  Then again, it would be empty, so it wouldn't be too serious if we then fail to read it in, right?  (We'd recover?  Or could we somehow end up permanently with no byte order stored?)

>-  return ( err == 0) ? NS_OK : NS_ERROR_FAILURE;
>+  return (err == 0) ? Commit(kCompressCommit) : NS_ERROR_FAILURE;

It might be a little cleaner to write this as:

if (err != 0)
  return NS_ERROR_FAILURE;

return Commit(kCompressCommit);

Comment 7

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

Also, how bad is the performance of doing extra compress commits?

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #6)
> This means there won't be a commit after the InitByteOrder call

I can't remember why I did that (probably a mistake, thinking the Commit in RemoveMatchingRows() was enough). I'll put it back.

> Also, how bad is the performance of doing extra compress commits?

When you delete under the "date and site" view this patch adds a commit we weren't doing before, a commit we do already perform when deleting under other views. No one has commented on a performance differential there. There would also be an extra commit on shutdown which is not going to be noticed.

Comment 9

[Daniel Veditz [:dveditz]]

Attachment: Added back the Commit at the end of RemoveAllPages

Comment 10

[Daniel Veditz [:dveditz]]

Fix checked into trunk.

Comment 11

[Mike Connor [:mconnor]]

clearing blocking nomination, this shouldn't be an issue in places (which replaces toolkit's history impl)