458849 - transition download visits saved when "Keep my history..." is unchecked.

Status: VERIFIED FIXED

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

Description

[Bjorn Poonen]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3

In the Privacy pane of preferences, I uncheck "Keep my history for at least ... days" (where ... is anything, e.g., 0).  In the same pane, I clear private data, including history. I then visit the URL of a file such as a PDF file.  "Show All History" then shows that the URL has been saved to History.

Reproducible: Always

Steps to Reproduce:
1. Open Preferences.
2. Select the Privacy pane.
3. Uncheck the box "Keep my history for at least ... days".
4. Click "Clear Now" (making sure that all history items are selected to be cleared)
5. Select "Show All History" under the History menu - history is empty.
6. Close the History window.
7. Visit http://www-math.mit.edu/~poonen/slides/cantrell3.pdf  (or any other PDF file online)
8. Select "Show All History" under the History menu.
Actual Results:  
cantrell3.pdf shows up in the History window.

Expected Results:  
History should still be empty, since "Keep my history..." was unchecked.

Unchecking "Keep my history" should mean that no history is saved.
Setting the number of days for history to be saved to 0 should have the same effect.

Comment 1

[mirage]

same problem when we limit the duration of the registration period. history is saved over this period...

Comment 2

[Marco Bonardo [:mak]]

(In reply to comment #1)
> same problem when we limit the duration of the registration period. history is
> saved over this period...

how do you limit? notice the pref says "for AT LEAST X days", so we will really save for more that those days. to limit on an exact number of days you should set the config browser.history_expire_days to the same number of days.

Comment 3

[mirage]

(In reply to comment #2)
> (In reply to comment #1)
> > same problem when we limit the duration of the registration period. history is
> > saved over this period...
> 
> how do you limit? notice the pref says "for AT LEAST X days", so we will really
> save for more that those days. to limit on an exact number of days you should
> set the config browser.history_expire_days to the same number of days.

hmmm, this is not an good configuration, if I choose to save at least 3 days, these 3 days history have to be saved and the 4th days history must be delete !!

This should be :
If this option is checked => History is saved
the period backup history limits backup duration of history.

That's what I thinking...

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #3)
> hmmm, this is not an good configuration, if I choose to save at least 3 days,
> these 3 days history have to be saved and the 4th days history must be delete
> !!

what you want is "for AT LAST X days", take a look at bug 425219, since that could change actual implementation.

Comment 5

[Marco Bonardo [:mak]]

reproducible with original STRs, i see that the visit has been saved

Comment 6

[Shawn Wilsher :sdwilsh]

This happens because nsIDownloadHistory::addDownload (implemented by nsNavHistory) doesn't check if we can add to history, which just calls AddVisit, which also doesn't check.

We just need to check in nsNavHistory::AddDownload, and this is totally testable.

Comment 7

[Marco Bonardo [:mak]]

thanks Shawn. should we move the bug to download manager then?

Comment 8

[Marco Bonardo [:mak]]

ops nevermind, sorry i misread your comment!

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch

tests for disabled history and private browsing, throws in case (since from what i see in the idl addDownload throws if it can't add)

Comment 10

[Shawn Wilsher :sdwilsh]

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

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>+  // don't add when history is disabled
>+  if (IsHistoryDisabled())
>+    return NS_ERROR_NOT_AVAILABLE;
It's not right to throw because the history implementation is available.  We just aren't writing to history.

>+var tests = [
>+  test_dh,
>+  test_dh_privateBrowsing,
>+  test_dh_disabledHistory
nit: comma after last test

Comment 11

[Marco Bonardo [:mak]]

Boris, Shawn suggested to ask you for confirmation on the correct behaviour here, i think NS_ERROR_NOT_AVAILABLE should be returned also when the service cannot add the download (so there't not a service available that CAN add it), so a third party implementer is aware it has not been added. Shawn thinks that nsIDownloadHistory should throw only if the service is not available at all, while if it won't add the download it should silently fail.

What do you think is the correct behaviour to follow in these cases?

Comment 12

[Boris Zbarsky [:bzbarsky]]

The API documentation says that this should only happen if history is not available.  In this case it really is available, but decides not to save the data.

Ideally, the API would return whether the history saved the data or not, but that's a separate issue...

I think it's worth being consistent with the other IsHistoryDisabled() checks here and using NS_OK.  (And in practice, none of our callers of this method check rv so it doesn't matter much.)

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.1

fixed comments

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 353468 [details] [diff] [review]
patch v1.1

r=sdwilsh

Comment 15

[Marco Bonardo [:mak]]

Attachment: for checkin

i forgot to remove the check for exception in the test, this fixes that and is ready for push.

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/32a3759db50f

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/55d8c8727154

Comment 18

[Tracy Walker [:tracy]]

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

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 ID:20090305133223