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

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Richard Newman [:rnewman]]

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";

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Try:

https://tbpl.mozilla.org/?tree=Try&rev=3baa64a08656

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Marco Bonardo [:mak]]

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

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30e591fc7153

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c716b93520c4
https://hg.mozilla.org/releases/mozilla-beta/rev/659d9c2acc29

Comment 7

[u279076]

Does this have or need tests?

Comment 8

[Marco Bonardo [:mak]]

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...

Comment 9

[u279076]

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

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[u279076]

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.

Comment 12

[Ioana (away)]

Catalin is working on this and will update the bug as soon as he has some results.

Comment 13

[Catalin Varga [QA][:VarCat]]

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.

Comment 14

[Marco Bonardo [:mak]]

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

Comment 15

[Catalin Varga [QA][:VarCat]]

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

Comment 16

[Catalin Varga [QA][:VarCat]]

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

Comment 17

[Catalin Varga [QA][:VarCat]]

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.

Comment 18

[Marco Bonardo [:mak]]

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?

Comment 19

[Catalin Varga [QA][:VarCat]]

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

Comment 20

[Catalin Varga [QA][:VarCat]]

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

Comment 21

[Catalin Varga [QA][:VarCat]]

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