Closed Bug 953312 Opened 7 years ago Closed 7 years ago

Places expiration code still waits for "back", not "active", from nsIdleService

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox26 --- wontfix
firefox27 --- verified
firefox28 --- verified
firefox29 --- verified
firefox-esr17 --- wontfix
firefox-esr24 --- wontfix

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file)

This changed in Gecko 16, and whoever changed it didn't update consumers in the tree (see Bug 953282 for Sync).

toolkit/components/places/nsPlacesExpiration.js
39:const TOPIC_IDLE_END = "back";
Depends on: 715041
Try:

https://tbpl.mozilla.org/?tree=Try&rev=3baa64a08656
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #8351600 - Flags: review?(mak77)
Comment on attachment 8351600 [details] [diff] [review]
Proposed patch. v1

Review of attachment 8351600 [details] [diff] [review]:
-----------------------------------------------------------------

no comment about suddenly changing a notification this way...

Thanks for catching this, it's bad cause due to this we didn't properly expire old history entries, has likely affected performance and telemetry data.
I'd like to get it uplifted, as far as possible, since it's a very simple fix.
Attachment #8351600 - Flags: review?(mak77) → review+
well, I first landed this as https://hg.mozilla.org/integration/mozilla-inbound/rev/4136cb9f3aef but it had wrong bug number in the commit message, so I backed it out with https://hg.mozilla.org/integration/mozilla-inbound/rev/f23f0b5c59a5 and finally relanded with proper bug number
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e591fc7153
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/30e591fc7153
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8351600 [details] [diff] [review]
Proposed patch. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 715041
User impact if declined: after the first idle Places stops expiring history, that means profiles in the wild are growing without control. This may be causing perf issues for most actively browsing users, so the sooner we restart cleaning up, the best is.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Simple topic name change, no risk
String or IDL/UUID changes made by this patch: none
Attachment #8351600 - Flags: approval-mozilla-beta?
Attachment #8351600 - Flags: approval-mozilla-aurora?
Attachment #8351600 - Flags: approval-mozilla-beta?
Attachment #8351600 - Flags: approval-mozilla-beta+
Attachment #8351600 - Flags: approval-mozilla-aurora?
Attachment #8351600 - Flags: approval-mozilla-aurora+
No longer depends on: 953311
Blocks: 715041
No longer depends on: 715041
Does this have or need tests?
Flags: needinfo?(rnewman)
Flags: in-testsuite?
it doesn't have tests since testing idle is sort of hacky, and sending artificial notifications would not save us from cases where a notification changes this way...
(In reply to Marco Bonardo [:mak] from comment #8)
> it doesn't have tests since testing idle is sort of hacky, and sending
> artificial notifications would not save us from cases where a notification
> changes this way...

Is this something that can be tested manually? It sounds like it wouldn't be reliable even if we could.
A way to test this manually may be::
- start with a sized history (let's say 1000 entries) but no bookmarks
- set places.history.expiration.max_pages to 10
- go idle for more than 5 minutes
- come back from idle
- check after a couple hours of activity (no idle) that history has been reduced much towards the 10 pages target

As you can see it's not exact science, but it should be feasible, though it's very hairy.
Flags: needinfo?(rnewman)
Ioana, please have someone on your team run through the test in comment 10 and do some exploratory testing around it. I don't think we can 100% verify this fix but we can at least make an effort.
Flags: needinfo?(ioana.budnar)
Keywords: verifyme
Flags: in-testsuite? → in-testsuite-
Catalin is working on this and will update the bug as soon as he has some results.
Flags: needinfo?(ioana.budnar)
QA Contact: catalin.varga
Following the STR from comment 10 I've managed to reproduce the bug using:

FF Nightly: 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Build Id: 20131227030203
OS: Windows 7 x64
 
After going idle for 10 minutes the history is not reducing anymore.

I've verified this bug using:

FF 27
Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Build id:20140116125114
OS: Windows 7 x64

The history keeps reducing after the 10 minutes idle break.

However I've noticed that if the deleting of the links occurs when there is a flash video playing the cpu usage increases to 50% and Firefox starts becoming unresponsive. This issue is reproducible on older versions also.

On Monday I will verify the bug on Linux and Mac Os, also I will verify the unresponsiveness issue on Linux and Mac Os and will log a bug.
(In reply to Catalin Varga [QA][:VarCat] from comment #13)
> However I've noticed that if the deleting of the links occurs when there is
> a flash video playing the cpu usage increases to 50% and Firefox starts
> becoming unresponsive. 

Please file a separate bug with STR, if this gets confirmed.
Verified as fixed using the following environments:

FF 27 beta 7
Build id:20140116125114
OS: Windows 7 x64, Ubuntu 12.04 x32 , Mac Os x 10.9
(In reply to Marco Bonardo [:mak] from comment #14)
> (In reply to Catalin Varga [QA][:VarCat] from comment #13)
> > However I've noticed that if the deleting of the links occurs when there is
> > a flash video playing the cpu usage increases to 50% and Firefox starts
> > becoming unresponsive. 
> 
> Please file a separate bug with STR, if this gets confirmed.

I didn't managed to reproduce the issue on a different machine or a different OS so I guess it was something with my machine, so I didn't file a bug.
I've verified this bug using the following environment:

FF 28.0a2 Aurora
Build Id: 20140123004002
OS: Windows 7, Ubuntu 12.04 x32

but the bug seems to be reproducible after the idle break I used FF profile for a long time without the history decreasing to the 10 page target.
I should have told that we don't expire history younger than 7 days... was that a newly created profile or was it copied and had older history?
(In reply to Marco Bonardo [:mak] from comment #18)
> I should have told that we don't expire history younger than 7 days... was
> that a newly created profile or was it copied and had older history?

Thanks for the quick reply that explains why the history stopped reducing. I will verify the bug using older history links.
Verified as fixed using the following environment:

FF 28.0a2 Aurora
Build Id: 20140130004003
OS: Windows 7, Ubuntu 13.04 x64, Mac Os 10.7.5
Verified as fixed using the following environment:
FF 29.0b4
Build Id: 20140331125246
OS: Win 7x 64, Ubuntu 13.10 x64, Mac Os 10.9
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.