Closed Bug 1400562 Opened 3 years ago Closed 3 years ago

BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true

Categories

(Firefox :: New Tab Page, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: froydnj, Assigned: ursula)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

[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.
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)
Whiteboard: [MemShrink] → [MemShrink:P2]
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)
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)
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()}`)
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)
Curious, do you have any extensions that create a thumbnail of each tab?
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");
```
Tentatively tracking for 57.  Are these leaks new?
(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)
(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?
Flags: needinfo?(nfroyd)
(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)
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
Priority: -- → P2
Component: New Tab Page → Activity Streams: Newtab
Summary: new tab page leaks observers → BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true
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.
Assignee: nobody → usarracini
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-
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 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?
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 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+
I'll file a follow up bug to fix the observer issues properly. Thanks Ed!
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
Blocks: 1407393
https://hg.mozilla.org/mozilla-central/rev/b3109db72d87
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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…
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)
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+
(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-
(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)
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.