Closed Bug 663344 Opened 9 years ago Closed 9 years ago

Maintenance may cause history loss

Categories

(Toolkit :: Places, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla7
Tracking Status
firefox6 --- fixed

People

(Reporter: mak, Assigned: mak)

Details

(Keywords: dataloss, regression, Whiteboard: [places-next-wanted][fixed-in-places])

Attachments

(2 files, 2 obsolete files)

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
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.
(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?
no, we need to expire everything in tests to check queries correctness.
(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?
Whiteboard: [places-next-wanted]
Attached patch patch v1.0 (obsolete) — Splinter Review
This may be worth to backport :(
Attachment #538605 - Flags: review?(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.
argh, looks like I always passed-in the right type, this way those helper are less clever! good catch.
Attached patch patch v1.1 (obsolete) — Splinter Review
fix helpers. the facts tests were passing even with that error is expected, no test is passing in a nsIURI yet.
Attachment #538605 - Attachment is obsolete: true
Attachment #538605 - Flags: review?(dietrich)
Attachment #539484 - Flags: review?(dietrich)
Summary: Maintenance may delete too much history → Maintenance may cause history loss
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?
(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.
Attached patch patch v1.2Splinter Review
Attachment #539484 - Attachment is obsolete: true
Attachment #539484 - Flags: review?(dietrich)
Attachment #539488 - Flags: review?(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".
Attachment #539488 - Flags: review?(dietrich) → review+
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
Flags: in-testsuite+
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed-in-places]
pushed a missing test change in downloads
http://hg.mozilla.org/projects/places/rev/6a5db334b0a4
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Attached patch coalesced patchSplinter Review
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.
Attachment #541665 - Flags: approval-mozilla-aurora?
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.
yes, this regressed in Firefox 4, and may have been cause of some of the history loss bugs that have been reported.
Keywords: regression
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.
Attachment #541665 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Are there some Steps to reproduce anyone could provide in order to check this issue in QA?
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
You need to log in before you can comment on or make changes to this bug.