1272679 - Clear Recent History, 1 hour, left accessed site in places.sqlite

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[nemo]

STR:
① Create new profile
② Access http://existentialcomics.com   (or any site you wish to test on) and wait for it to finish loading
③ Hit ctrl-shift-del, using the default one hour period, select all checkboxes under details.
④ Close the browser.  I tested using both the MATE Window Manager ⓧ  button and
the Firefox menu exit.
⑤ Search the new firefox profile for the website 
    $ grep -rl existentialcomics.com * | xargs
    places.sqlite places.sqlite-wal sessionstore.js

The sessionstore.js thing is a separate bug that :mak asked me to file.  bug #1272675

On restart of the profile, the -wal was merged, but places.sqlite entries remained.

They appear to be due to the favicon - favicon entries seem to have no create/modify timestamps which is complicating cleanup?  Oddly favicon is preserved despite no other site data in places.

Comment 1

[Marco Bonardo [:mak]]

The problem here is that clearing less than Everything doesn't send an onClearHistory notification, and as such Places expiration doesn't start doing its job.
The only critical tracking information we don't remove is favicons, the only other textual info we have is in moz_places (removed) and moz_hosts (removed). I think we should remove orphan favicons directly, rather than rely on expiration since they contain an url.

Comment 2

[Marco Bonardo [:mak]]

This will have multiple parts:
1. since now RemoveAllPages has been removed, I'm moving back orphans expiration from Places Expiration to History.jsm
2. Places expiration shutdown can likely be simplified, maybe places-will-close-connection can go
3. History.remove and History.removeVisitsByFilter should remove orphan favicons directly

This should give us some perf advantage on clear history (we remove pages before visits, so the visits trigger has less to do, plus we can use a transaction), a simpler shutdown, and solve this bug.

Comment 3

[Marco Bonardo [:mak]]

I moved the second part (places-will-close-connection removal) to bug 1275878, since it's unrelated and could be more regression prone.

Comment 4

[Marco Bonardo [:mak]]

Unfortunately I see a leak that I can't explain, that may slowdown things...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60ef2c531ebb6d210015371072e10da9da8d8b1f
TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js | leaked 2 window(s) until shutdown [url = about:blank]
TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/browser.xul] 

This is at the end of the browser/components/privatebrowsing/test/browser folder. There's something fishy in these tests :(

Comment 5

[Marco Bonardo [:mak]]

I splitted all the "unrelated" stuff to bug 1275878, so here we can fix the privacy leak, and we won't be blocked by the memory leak the patches uncovered.

Comment 6

[Marco Bonardo [:mak]]

Attachment: MozReview Request: Bug 1272679 - Expire orphans in removeVisitsByFilter. r=adw

Review commit: https://reviewboard.mozilla.org/r/56276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56276/

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8757962 [details]
MozReview Request: Bug 1272679 - Expire orphans in removeVisitsByFilter. r=adw

https://reviewboard.mozilla.org/r/56276/#review54956

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/90899efbaf73
Expire orphans in removeVisitsByFilter. r=adw

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90899efbaf73