Closed
Bug 663344
Opened 13 years ago
Closed 13 years ago
Maintenance may cause history loss
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
14.44 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
13.49 KB,
patch
|
christian
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•13 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.
(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•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
Whiteboard: [places-next-wanted]
Assignee | ||
Comment 5•13 years ago
|
||
This may be worth to backport :(
Attachment #538605 -
Flags: review?(dietrich)
Comment 6•13 years ago
|
||
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•13 years ago
|
||
argh, looks like I always passed-in the right type, this way those helper are less clever! good catch.
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
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?
Assignee | ||
Comment 10•13 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•13 years ago
|
||
Attachment #539484 -
Attachment is obsolete: true
Attachment #539484 -
Flags: review?(dietrich)
Attachment #539488 -
Flags: review?(dietrich)
Comment 12•13 years ago
|
||
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•13 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•13 years ago
|
||
pushed a missing test change in downloads http://hg.mozilla.org/projects/places/rev/6a5db334b0a4
Assignee | ||
Comment 15•13 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
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Assignee | ||
Comment 16•13 years ago
|
||
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•13 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•13 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•13 years ago
|
Keywords: regression
Comment 19•13 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•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/a08107ec64e4
status-firefox6:
--- → fixed
Comment 21•13 years ago
|
||
Are there some Steps to reproduce anyone could provide in order to check this issue in QA?
Assignee | ||
Comment 22•13 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.
Description
•