663344 - Maintenance may cause history loss

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

This manual expiration step is unlimited, and as such if it runs when user is over limit it may expire all visits
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#770

Comment 1

[Marco Bonardo [:mak]]

I think a safe fix is to consider 0 a valid value and pass 0, so that expiration will only take care of orphans without removing visits.

Comment 2

[al_9x]

(In reply to comment #1)
> I think a safe fix is to consider 0 a valid value and pass 0, so that
> expiration will only take care of orphans without removing visits.

I though that "limit" here meant: how much of the total required expiration work is done in this expiration call.  It seems logical for UNLIMITED to mean: do everything that is needed for automatic expiration, but instead it means expire everything.

The current behavior of the expiration code with UNLIMITED flag doesn't make any sense, it expires everything, but only if it finds that something needs expiring. Since this is never correct/desired/sensible behavior, shouldn't UNLIMITED in this context be interpreted to mean: everything that's needed, rather than everything?

Comment 3

[Marco Bonardo [:mak]]

no, we need to expire everything in tests to check queries correctness.

Comment 4

[al_9x]

(In reply to comment #3)
> no, we need to expire everything in tests to check queries correctness.

Well, then perhaps there should be another flag, something like LIMIT.ALL_REQUIRED?

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This may be worth to backport :(

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 538605 [details] [diff] [review]
patch v1.0

Review of attachment 538605 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/head_common.js
@@ +275,2 @@
>  {
> +  let url = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;

url is not used anywhere in this function.

@@ +295,5 @@
> + * @return number of visits found.
> + */
> +function visits_in_database(aURI)
> +{
> +  let url = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;

url is not used anywhere in this function.

Comment 7

[Marco Bonardo [:mak]]

argh, looks like I always passed-in the right type, this way those helper are less clever! good catch.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.1

fix helpers. the facts tests were passing even with that error is expected, no test is passing in a nsIURI yet.

Comment 9

[al_9x]

let limit = parseInt(aData); // May be NaN.

did you intend to check limit for NaN and use it instead of aData for the rest of that block?

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> let limit = parseInt(aData); // May be NaN.
> 
> did you intend to check limit for NaN and use it instead of aData for the
> rest of that block?

No, I just added the comment to notice it in future, but actually I assigned aData to limit and then used aData, so good catch! More eyes (and less context switching) are always useful. Actually it doesn't change change, both "anystring" > 0 and NaN > 0 are false, I mostly wanted to parseInt to have a number or NaN.
New patch coming.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 539488 [details] [diff] [review]
patch v1.2

Review of attachment 539488 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsPlacesExpiration.js
@@ +537,5 @@
> +      else {
> +        // Fallback any non-positive values to unlimited, but notice this
> +        // differs from LIMIT.UNLIMITED, since it won't expire all visits.
> +        this._debugLimit = limit > 0 ? limit : -1;
> +        this._expireWithActionAndLimit(ACTION.DEBUG, LIMIT.DEBUG);

Please expand the comment to describe what exactly it *does* expire. Right now the comments are basically "all" and "not all".

Comment 13

[Marco Bonardo [:mak]]

I've changed the code to to a if/elseif/else with expanded comments for each branch. should be more understandable now.
http://hg.mozilla.org/projects/places/rev/ea9b8e556091

Comment 14

[Marco Bonardo [:mak]]

pushed a missing test change in downloads
http://hg.mozilla.org/projects/places/rev/6a5db334b0a4

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ea9b8e556091
http://hg.mozilla.org/mozilla-central/rev/6a5db334b0a4

will coalesce these 2 patches and then ask for Aurora approval.

Comment 16

[Marco Bonardo [:mak]]

Attachment: coalesced patch

Asking approval for Aurora since this bug may cause history loss when hit, the risk is contained since there is only one callpoint to the modified code.

Comment 17

[[:Cww]]

Do we know when this regressed or is this even a regression?  Is there anything in Firefox 6 (or 5?) that would make hitting this dataloss more likely?

Release drivers are trying to assess the benefit of this fix.

Comment 18

[Marco Bonardo [:mak]]

yes, this regressed in Firefox 4, and may have been cause of some of the history loss bugs that have been reported.

Comment 19

[LegNeato]

Comment on attachment 541665 [details] [diff] [review]
coalesced patch

Approved for releases/mozilla-aurora. Please land asap before 2011-07-05 @ 9:00 am PDT.

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/a08107ec64e4

Comment 21

[Virgil Dicu [:virgil] [QA]]

Are there some Steps to reproduce anyone could provide in order to check this issue in QA?

Comment 22

[Marco Bonardo [:mak]]

Sorry I missed the bugmail and never answered :(
The test should be enough, but I think it should be possible to test with these steps:
1. visits some pages, like 10 so that history menu is full
2. create and set places.history.expiration.max_pages to a low number, smaller than the 10 pages above
3. from the Error Console like Components.utils.import("resource://gre/modules/PlacesDBUtils.jsm"); PlacesDBUtils.expire();
4. wait 5 seconds and check history menu is not empty