The default bug view has changed. See this FAQ.

Maintenance may cause history loss

RESOLVED FIXED in Firefox 6

Status

()

Toolkit
Places
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({dataloss, regression})

unspecified
mozilla7
dataloss, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox6 fixed)

Details

(Whiteboard: [places-next-wanted][fixed-in-places])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
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

6 years ago
(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?
(Assignee)

Comment 3

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

Comment 4

6 years ago
(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?
(Assignee)

Updated

6 years ago
Whiteboard: [places-next-wanted]
(Assignee)

Comment 5

6 years ago
Created attachment 538605 [details] [diff] [review]
patch v1.0

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.
(Assignee)

Comment 7

6 years ago
argh, looks like I always passed-in the right type, this way those helper are less clever! good catch.
(Assignee)

Comment 8

6 years ago
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.
Attachment #538605 - Attachment is obsolete: true
Attachment #538605 - Flags: review?(dietrich)
Attachment #539484 - Flags: review?(dietrich)
(Assignee)

Updated

6 years ago
Summary: Maintenance may delete too much history → Maintenance may cause history loss

Comment 9

6 years ago
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?
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
Created attachment 539488 [details] [diff] [review]
patch v1.2
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+
(Assignee)

Comment 13

6 years ago
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]
(Assignee)

Comment 14

6 years ago
pushed a missing test change in downloads
http://hg.mozilla.org/projects/places/rev/6a5db334b0a4
(Assignee)

Comment 15

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Comment 16

6 years ago
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.
Attachment #541665 - Flags: approval-mozilla-aurora?

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
yes, this regressed in Firefox 4, and may have been cause of some of the history loss bugs that have been reported.
(Assignee)

Updated

6 years ago
Keywords: regression

Comment 19

6 years ago
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+
(Assignee)

Comment 20

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/a08107ec64e4
status-firefox6: --- → fixed
Are there some Steps to reproduce anyone could provide in order to check this issue in QA?
(Assignee)

Comment 22

6 years ago
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.