Closed
Bug 407018
Opened 17 years ago
Closed 17 years ago
expiration not using browser.history_expire_days_min
Categories
(Firefox :: Bookmarks & History, defect, P1)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta2
People
(Reporter: dietrich, Assigned: dietrich)
References
Details
Attachments
(1 file)
(deleted),
patch
|
moco
:
review+
mconnor
:
approvalM10+
|
Details | Diff | Splinter Review |
found some typos and test errors that cause us to not use the minimum date expiration range.
Attachment #291717 -
Flags: review?(sspitzer)
Attachment #291717 -
Flags: approvalM10?
Assignee | ||
Updated•17 years ago
|
Attachment #291717 -
Flags: approvalM10?
Assignee | ||
Updated•17 years ago
|
Priority: -- → P1
Comment 1•17 years ago
|
||
Comment on attachment 291717 [details] [diff] [review]
fix
sorry for not catching the s/selectStatement/selectMinStatement typo during the review
Attachment #291717 -
Flags: review?(sspitzer) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #291717 -
Flags: approvalM10?
Comment 2•17 years ago
|
||
Comment on attachment 291717 [details] [diff] [review]
fix
bug in the fix for a P1 blocker, needed to actually fix it, thus aM10 on behalf of drivers
Attachment #291717 -
Flags: approvalM10? → approvalM10+
Assignee | ||
Comment 3•17 years ago
|
||
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v <-- nsNavHistoryExpire.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/components/places/tests/unit/test_expiration.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_expiration.js,v <-- test_expiration.js
new revision: 1.12; previous revision: 1.11
done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 4•17 years ago
|
||
It looks like trace_malloc_allocs on bm-xserve11 (OS X) jumped significantly with this checkin. Windows and Linux didn't change though, oddly enough. I'm guessing that the actualy problem is in bug 402880, checked in ~12 hours earlier, but just wasn't exposed without this fix.
Note allocation counts and total size from the test runs before and after this checkin:
----------- Output from trace-malloc bloat test: leakstats -------------
./leakstats starting at Wed Dec 5 13:35:40 2007
Leaks: 999855 bytes, 12027 allocations
Maximum Heap Size: 18895765 bytes
172290331 bytes were allocated in 468121 allocations.
Logged 870 free (or realloc) calls for which we missed the original malloc.
----------- Output from trace-malloc bloat test: leakstats -------------
./leakstats starting at Wed Dec 5 13:46:44 2007
Leaks: 996087 bytes, 11975 allocations
Maximum Heap Size: 19387710 bytes
315604101 bytes were allocated in 570737 allocations.
Logged 873 free (or realloc) calls for which we missed the original malloc.
http://build-graphs.mozilla.org/graph/query.cgi?testname=trace_malloc_allocs&units=count&tbox=bm-xserve11.build.mozilla.org&autoscale=1&days=7&avg=1&showpoint=2007:12:05:13:24:58,467098
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•17 years ago
|
Flags: blocking-firefox3?
Comment 5•17 years ago
|
||
Confirmed, after backing out trace_malloc_allocs dropped to the previous level.
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Comment 6•17 years ago
|
||
Seth/Dietrich: do we have an idea of what's causing this and an ETA for a repair? Do we need to just back out bug 402880 entirely until we have this resolved?
Trying to figure out what to do for B2 freeze, and whether this follow-up fix is required for B2.
Assignee | ||
Comment 7•17 years ago
|
||
I'm still working on tracking down the allocations. I'm trying to get the tests running locally, as well as going to see about checking back in temporarily and grabbing the logs off of the tinderbox.
I'll come up w/ a partial backout patch, since we do want to keep some parts of bug 402880.
Comment 8•17 years ago
|
||
On line 478 you have:
nsCOMPtr<mozIStorageStatement> selectMinStatement;
nsresult rv = aConnection->CreateStatement(sqlMinAge,getter_AddRefs(selectMinStatement));
Where you create the selectMinStatement - but it is never used below (selectStatement is). Was that intended?
Comment 9•17 years ago
|
||
> Was that intended?
schrep, it sounds like you are looking at lxr (or what is checked in). dietrich patch for this bug fixes that.
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #9)
> > Was that intended?
>
> schrep, it sounds like you are looking at lxr (or what is checked in).
> dietrich patch for this bug fixes that.
>
Right: The patch for this bug fixes the issue Schrep points out. Applying that patch caused the huge trace_malloc_allocs jump on bm-xserve11, so we reverted it.
Assignee | ||
Comment 11•17 years ago
|
||
checked back in, after working w/ sspitzer, rhelmer and coop. initial numbers look good, but the box is having problems now, so not closing this yet.
turns out the mac memtest box was not cleaning it's profile, so it's history was getting bigger and older. current theory: once bug 402280 (and patched via this bug), we started expiring more of those visits, and allocating more memory.
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v <-- nsNavHistoryExpire.cpp
new revision: 1.30; previous revision: 1.29
done
Comment 12•17 years ago
|
||
Reland the test?
Assignee: nobody → dietrich
Status: REOPENED → NEW
Flags: in-testsuite?
Comment 13•17 years ago
|
||
This is the last beta 2 bug. Please update the status ASAP.
Comment 14•17 years ago
|
||
This was checked back in... resolving.
Status: NEW → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 15•17 years ago
|
||
relanded tests.
Checking in toolkit/components/places/tests/unit/test_expiration.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_expiration.js,v <-- test_expiration.js
new revision: 1.14; previous revision: 1.13
done
Comment 16•17 years ago
|
||
had to do some tweaking of browser.history_expire_days and browser.history_expire_sites to verify this with a profile containing just two days of history. but it looks good on Win XP nightly.
adding in-testsuite+ based on comment #15
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
Comment 17•15 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•