Closed Bug 1272679 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bugs, Assigned: mak)

References

Details

(Keywords: privacy)

Attachments

(1 file)

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.
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.
Assignee: nobody → mak77
Keywords: privacy
Priority: -- → P1
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.
Blocks: 977053
Blocks: 1275878
I moved the second part (places-will-close-connection removal) to bug 1275878, since it's unrelated and could be more regression prone.
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 :(
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.
No longer blocks: 977053
Comment on attachment 8757962 [details]
MozReview Request: Bug 1272679 - Expire orphans in removeVisitsByFilter. r=adw

https://reviewboard.mozilla.org/r/56276/#review54956
Attachment #8757962 - Flags: review?(adw) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/fx-team/rev/90899efbaf73
Expire orphans in removeVisitsByFilter. r=adw
https://hg.mozilla.org/mozilla-central/rev/90899efbaf73
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.