Closed Bug 1272679 Opened 4 years ago Closed 4 years ago

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


(Toolkit :: Places, defect, P1)




Tracking Status
firefox50 --- fixed


(Reporter: bugs, Assigned: mak)



(Keywords: privacy)


(1 file)

① Create new profile
② Access   (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 * | 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...
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
Attachment #8757962 - Flags: review?(adw) → review+
Pushed by
Expire orphans in removeVisitsByFilter. r=adw
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.