Closed
Bug 1400562
Opened 7 years ago
Closed 7 years ago
BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true
Categories
(Firefox :: New Tab Page, defect, P2)
Firefox
New Tab Page
Tracking
()
VERIFIED
FIXED
Firefox 58
People
(Reporter: froydnj, Assigned: ursula)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Mardak
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
[Tracking Requested - why for this release]: memory leaks Looking at about:memory revealed some interesting things in the parent process: 43,544 (100.0%) -- observer-service └──43,544 (100.0%) -- referent ├──42,767 (98.22%) ── strong └─────777 (01.78%) -- weak ├──751 (01.72%) ── alive └───26 (00.06%) ── dead 42,526 (100.0%) -- observer-service-suspect ├──20,520 (48.25%) ── referent(topic=page-thumbnail:create) ├──20,520 (48.25%) ── referent(topic=page-thumbnail:error) ├─────509 (01.20%) ── referent(topic=xpcom-shutdown) ├─────508 (01.19%) ++ (3 tiny) └─────469 (01.10%) ── referent(topic=memory-pressure) So somebody has registered thousands of observers for page thumbnail events, and those observers are never getting freed. (I have no tabs with about:newtab open.) I don't know how much those observers are holding, but it'd be good to make sure they get unregistered and GC'd regardless; leaks of any amount are no good.
Comment 1•7 years ago
|
||
mardak, it looks like you've been working on background thumbs code recently [1], can you take a look at it? [1] http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/toolkit/components/thumbnails/BackgroundPageThumbs.jsm#132-143
Flags: needinfo?(edilee)
Updated•7 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 2•7 years ago
|
||
Any STR? I tried making activity stream trigger hundreds of screenshots for top sites, but that didn't show up in about:memory for me. Maybe I'm not looking in the right place? Or something else is triggering these leaks outside of activity stream. I see this under Main Process > Other Measurements 1,525 (100.0%) -- observer-service └──1,525 (100.0%) -- referent ├──1,058 (69.38%) ── strong └────467 (30.62%) -- weak ├──465 (30.49%) ── alive └────2 (00.13%) ── dead 708 (100.0%) -- observer-service-suspect ├──339 (47.88%) ── referent(topic=xpcom-shutdown) ├──166 (23.45%) ── referent(topic=memory-pressure) ├──102 (14.41%) ── referent(topic=wake_notification) └──101 (14.27%) ── referent(topic=chrome-flush-skin-caches)
Flags: needinfo?(nfroyd)
Reporter | ||
Comment 3•7 years ago
|
||
I don't have a good STR, other than "use the browser for a bunch of stuff". I see that when I restart my browser (several hundred tabs), I have: 2,261 (100.0%) -- observer-service └──2,261 (100.0%) -- referent ├──1,800 (79.61%) ── strong └────461 (20.39%) -- weak ├──460 (20.34%) ── alive └────1 (00.04%) ── dead 1,096 (100.0%) -- observer-service-suspect ├────329 (30.02%) ── referent(topic=xpcom-shutdown) ├────271 (24.73%) ── referent(topic=memory-pressure) ├────248 (22.63%) ── referent(topic=page-thumbnail:create) └────248 (22.63%) ── referent(topic=page-thumbnail:error) Just opening and closing new tabs does not seem to trigger any new observers being added. Do you have suggestions for things that I can do to try and make that number increase reliably?
Flags: needinfo?(nfroyd)
Comment 4•7 years ago
|
||
I can't seem to get the `referent(topic=page-thumbnail:create)` line to appear even when there should be hundreds of observers. Cu.import("resource://gre/modules/BackgroundPageThumbs.jsm"); for (let i = 0; i < 100; i++) BackgroundPageThumbs.captureIfMissing(`https://localhost/?${Math.random()}`)
Comment 5•7 years ago
|
||
Okay, I can see the observers from about:memory if I manually request that many. I tried opening up hundreds of tabs and restoring them, but that doesn't trigger these `referent(topic=page-thumbnail:create)` froydnj, do you have any custom `browser.sessions.*` prefs? E.g., are the restored tabs loading automatically or only only when you switch to that tab? In any case, the many observers might not actually be a leak -- just inefficient. If you watch about:memory, does that count go down by 1 or 2 every minute? As a sanity check, try this in your Browser Console: Cu.import("resource://gre/modules/BackgroundPageThumbs.jsm"); BackgroundPageThumbs._captureQueue.length That should print out how many captures it has pending. Similarly, if you want to see what it's actually trying to capture: Cu.import("resource://gre/modules/BackgroundPageThumbs.jsm"); BackgroundPageThumbs._captureQueue.map(o => o.url)
Flags: needinfo?(edilee) → needinfo?(nfroyd)
Comment 6•7 years ago
|
||
Curious, do you have any extensions that create a thumbnail of each tab?
Comment 7•7 years ago
|
||
If you build with this patch, it should print out (to browser console and terminal) what's requesting the screenshot of what url: ``` diff --git a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm --- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm +++ b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm @@ -136,4 +136,7 @@ const BackgroundPageThumbs = { // observed or when our caller is unloading + let _=function(s){s=[s,...this].join(" ");Cu.reportError(s);dump(s+"\n")}.bind([url,Error().stack]); + _("observed"); function cleanup() { if (observe) { + _("removing"); Services.obs.removeObserver(observe, "page-thumbnail:create"); ```
Reporter | ||
Comment 9•7 years ago
|
||
(In reply to Ed Lee :Mardak from comment #5) > Okay, I can see the observers from about:memory if I manually request that > many. I tried opening up hundreds of tabs and restoring them, but that > doesn't trigger these `referent(topic=page-thumbnail:create)` > > froydnj, do you have any custom `browser.sessions.*` prefs? E.g., are the > restored tabs loading automatically or only only when you switch to that tab? They only restored when I switch to the tab. about:config tells me that I don't have any browser.sessions.* prefs. I do have browser.sessionstore.* prefs, but the only one that's changed from the default is browser.sessionstore.upgradeBackup.latestBuildID, which doesn't seem relevant to this case. > In any case, the many observers might not actually be a leak -- just > inefficient. If you watch about:memory, does that count go down by 1 or 2 > every minute? No. It keeps increasing; e.g. from comment 18 to right now, prior to restarting the browser, I had nearly 1000 observers of each topic. > As a sanity check, try this in your Browser Console: > > Cu.import("resource://gre/modules/BackgroundPageThumbs.jsm"); > BackgroundPageThumbs._captureQueue.length > > That should print out how many captures it has pending. Uh. BackgroundPageThumbs._captureQueue is undefined. How can that happen?! (In reply to Ed Lee :Mardak from comment #6) > Curious, do you have any extensions that create a thumbnail of each tab? The only extensions I have are Bugzilla Socorro Lens, the Gecko Profiler, and Test Pilot. I wouldn't imagine any of those is doing anything with thumbnails, but I could be wrong?
Flags: needinfo?(nfroyd)
Comment 10•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #9) > Uh. BackgroundPageThumbs._captureQueue is undefined. How can that happen?! Ah that might be it! Do you have any custom "browser.pagethumbnails.*" prefs set?
Updated•7 years ago
|
Flags: needinfo?(nfroyd)
Reporter | ||
Comment 11•7 years ago
|
||
(In reply to Ed Lee :Mardak from comment #10) > (In reply to Nathan Froyd [:froydnj] from comment #9) > > Uh. BackgroundPageThumbs._captureQueue is undefined. How can that happen?! > Ah that might be it! > > Do you have any custom "browser.pagethumbnails.*" prefs set? I apparently have two of them and they are all set, though I don't remember modifying them. browser.pagethumbnails.storage_version is 3 browser.pagethumbnails.capturing_disabled is true Why would those have been changed? Test Pilot?
Flags: needinfo?(nfroyd)
Comment 12•7 years ago
|
||
No clue how those would be set. But looks like there's faulty logic to add observer but skip capture when browser.pagethumbnails.capturing_disabled is true
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Component: New Tab Page → Activity Streams: Newtab
Summary: new tab page leaks observers → BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true
Comment 13•7 years ago
|
||
This sounds like a serious issue but like we may not have anything actionable for 57. I'll assume this is affected for 58 as well. Fix-optional for 57 since we are in mid-beta already.
status-firefox58:
--- → affected
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → usarracini
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8917007 [details] Bug 1400562 - BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true https://reviewboard.mozilla.org/r/188040/#review193200 ::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm (Diff revision 1) > * @opt backgroundColor The background colour when capturing an image. > */ > capture(url, options = {}) { > - if (!PageThumbs._prefEnabled()) { > - if (options.onDone) > - schedule(() => options.onDone(url)); It looks like the only direct external callers of `capture` are from tests… but it's probably a safer fix to leave this here. https://searchfox.org/mozilla-central/search?q=BackgroundPageThumbs.capture%28&path=.js ::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm:114 (Diff revision 1) > + return url; > + } > // The fileExistsForURL call is an optimization, potentially but unlikely > // incorrect, and no big deal when it is. After the capture is done, we > // atomically test whether the file exists before writing it. > let exists = await PageThumbsStorage.fileExistsForURL(url); Ehh.. mmm... I suppose either way is fine as those who have page thumbs turned off probably won't have any existing ones anyway… Otherwise, we could have checked for file existing and returning that.
Attachment #8917007 -
Flags: review?(edilee) → review-
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8917007 [details] Bug 1400562 - BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true https://reviewboard.mozilla.org/r/188040/#review193200 > Ehh.. mmm... I suppose either way is fine as those who have page thumbs turned off probably won't have any existing ones anyway… Otherwise, we could have checked for file existing and returning that. Might be better to just not return anything at all? It would be consistent with the behaviour in the capture function
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8917007 [details] Bug 1400562 - BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true https://reviewboard.mozilla.org/r/188040/#review193200 > Might be better to just not return anything at all? It would be consistent with the behaviour in the capture function Well, `capture` always returns undefined whereas it's expected for `captureIfMissing` to return a url. Altohugh it's the same url passed in, so not sure if anyone is actually using the return value. But more interesting to consider is what happens to all the callers who weren't expecting the pref to disable thumbnails? In particular, activity stream still tries to get the thumbnail path and open a file?
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8917007 [details] Bug 1400562 - BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true https://reviewboard.mozilla.org/r/188040/#review193200 > Well, `capture` always returns undefined whereas it's expected for `captureIfMissing` to return a url. Altohugh it's the same url passed in, so not sure if anyone is actually using the return value. But more interesting to consider is what happens to all the callers who weren't expecting the pref to disable thumbnails? In particular, activity stream still tries to get the thumbnail path and open a file? Yeah... there's no indication to know if we even want to attempt a capture in the first place, so in theory I guess activity stream would throw an error when trying to open the file of a thumbnail path which doesn't exist. The only other caller for `captureIfMissing` is in the old newtab code, which doesn't try to open up a file since it uses the thumbnail protocol. Maybe we should do a check on our side too for the pref before attempting to capture the thumbnail
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8917007 [details] Bug 1400562 - BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true https://reviewboard.mozilla.org/r/188040/#review193276 I guess good enough. All Highlights end up blank and console has `getScreenshot error: Unix error 2 during operation open on file mozilla-central/obj/tmp/scratch_user/thumbnails/737af5ed42720bb1426787a8cd2caff5.png (No such file or directory)` but should be no leaks… I would usually suggest a test especially for uplift, but seems like it might be tricky to test for added observer and even if we do, it will probably be rewritten/changed when we actually fix the bug with at most one observer.
Attachment #8917007 -
Flags: review?(edilee) → review+
Assignee | ||
Comment 21•7 years ago
|
||
I'll file a follow up bug to fix the observer issues properly. Thanks Ed!
Comment 22•7 years ago
|
||
Pushed by usarracini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b3109db72d87 BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true r=Mardak
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b3109db72d87
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 24•7 years ago
|
||
Cu.import("resource://gre/modules/BackgroundPageThumbs.jsm"); for (let i = 0; i < 110; i++) BackgroundPageThumbs.captureIfMissing(`https://localhost/?${Math.random()}`) Before 58.0a1 Build ID 20171010220102: 682 (100.0%) -- observer-service-suspect … ├──110 (16.13%) ── referent(topic=page-thumbnail:create) └──110 (16.13%) ── referent(topic=page-thumbnail:error) Waiting for nightly build with this fix…
Comment 25•7 years ago
|
||
After 58.0a1 Build ID 20171011141448 built from f3e939a81ee1: 212 (100.0%) -- observer-service-suspect └──212 (100.0%) ── referent(topic=xpcom-shutdown) froydnj, would be good if you could also verify when the actual Nightly build comes out. I think it should be verified enough with the m-c build for requesting uplift.
Status: RESOLVED → VERIFIED
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 26•7 years ago
|
||
Comment on attachment 8917007 [details] Bug 1400562 - BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true Approval Request Comment [Feature/Bug causing the regression]: Adding a pref causes memory leaks when attempting to capture many screenshots for Activity Stream [User impact if declined]: Potential for bad memory leaks while using the browser [Is this code covered by automated tests?]: Yes, thumbnail code has test coverage [Has the fix been verified in Nightly?]: Yes, 20171011141448 [Needs manual test from QE? If yes, steps to reproduce]: None [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Not really [Why is the change risky/not risky?]: It's a small bit of code which just short circuits a function, and only if this pref is enabled [String changes made/needed]: None
Attachment #8917007 -
Flags: approval-mozilla-beta?
Comment on attachment 8917007 [details] Bug 1400562 - BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true Fixes a memleak, Beta57+
Attachment #8917007 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 28•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/613890f7fb2f
Comment 29•7 years ago
|
||
(In reply to Ursula Sarracini (:ursula) from comment #26) > [Is this code covered by automated tests?]: Yes, thumbnail code has test > coverage > [Has the fix been verified in Nightly?]: Yes, 20171011141448 > [Needs manual test from QE? If yes, steps to reproduce]: None Setting qe-verify- based on Ursula's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Reporter | ||
Comment 30•7 years ago
|
||
(In reply to Ed Lee :Mardak from comment #25) > After 58.0a1 Build ID 20171011141448 built from f3e939a81ee1: > > 212 (100.0%) -- observer-service-suspect > └──212 (100.0%) ── referent(topic=xpcom-shutdown) > > froydnj, would be good if you could also verify when the actual Nightly > build comes out. I think it should be verified enough with the m-c build for > requesting uplift. I do not see any more problems in Nightly. Thank you!
Flags: needinfo?(nfroyd)
Updated•5 years ago
|
Component: Activity Streams: Newtab → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•