Last Comment Bug 663344 - Maintenance may cause history loss
: Maintenance may cause history loss
Status: RESOLVED FIXED
[places-next-wanted][fixed-in-places]
: dataloss, regression
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla7
Assigned To: Marco Bonardo [::mak]
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-10 02:41 PDT by Marco Bonardo [::mak]
Modified: 2011-09-29 08:34 PDT (History)
5 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v1.0 (14.43 KB, patch)
2011-06-10 14:08 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (14.42 KB, patch)
2011-06-15 05:39 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.2 (14.44 KB, patch)
2011-06-15 06:14 PDT, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
coalesced patch (13.49 KB, patch)
2011-06-24 05:30 PDT, Marco Bonardo [::mak]
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2011-06-10 02:41:59 PDT
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] 2011-06-10 02:43:12 PDT
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 2011-06-10 04:09:47 PDT
(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] 2011-06-10 04:34:43 PDT
no, we need to expire everything in tests to check queries correctness.
Comment 4 al_9x 2011-06-10 04:44:15 PDT
(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] 2011-06-10 14:08:45 PDT
Created attachment 538605 [details] [diff] [review]
patch v1.0

This may be worth to backport :(
Comment 6 Dietrich Ayala (:dietrich) 2011-06-13 08:49:35 PDT
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] 2011-06-14 13:14:38 PDT
argh, looks like I always passed-in the right type, this way those helper are less clever! good catch.
Comment 8 Marco Bonardo [::mak] 2011-06-15 05:39:46 PDT
Created attachment 539484 [details] [diff] [review]
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 2011-06-15 06:02:25 PDT
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] 2011-06-15 06:09:42 PDT
(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] 2011-06-15 06:14:50 PDT
Created attachment 539488 [details] [diff] [review]
patch v1.2
Comment 12 Dietrich Ayala (:dietrich) 2011-06-15 09:54:15 PDT
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] 2011-06-16 03:15:07 PDT
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] 2011-06-16 08:03:05 PDT
pushed a missing test change in downloads
http://hg.mozilla.org/projects/places/rev/6a5db334b0a4
Comment 15 Marco Bonardo [::mak] 2011-06-18 04:43:48 PDT
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] 2011-06-24 05:30:21 PDT
Created attachment 541665 [details] [diff] [review]
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] 2011-06-28 15:19:12 PDT
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] 2011-06-28 15:44:14 PDT
yes, this regressed in Firefox 4, and may have been cause of some of the history loss bugs that have been reported.
Comment 19 christian 2011-06-30 14:37:36 PDT
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 21 Virgil Dicu [:virgil] [QA] 2011-08-23 07:12:47 PDT
Are there some Steps to reproduce anyone could provide in order to check this issue in QA?
Comment 22 Marco Bonardo [::mak] 2011-09-29 08:34:04 PDT
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

Note You need to log in before you can comment on or make changes to this bug.