BackgroundPageThumbs.captureIfMissing leaks when hidden capturing_disabled pref is true

VERIFIED FIXED in Firefox 57

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: froydnj, Assigned: ursula)

Tracking

(Blocks 1 bug)

unspecified
Firefox 58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57+ fixed, firefox58 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

Reporter

Description

2 years ago
[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]

Comment 2

2 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

2 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

2 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

2 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

2 years ago
Curious, do you have any extensions that create a thumbnail of each tab?

Comment 7

2 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");
```
Tentatively tracking for 57.  Are these leaks new?
Reporter

Comment 9

2 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

2 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

2 years ago
Flags: needinfo?(nfroyd)
Reporter

Comment 11

2 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

2 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
Priority: -- → P2

Updated

2 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
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

Updated

2 years ago
Assignee: nobody → usarracini

Comment 15

2 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

2 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

2 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

2 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

2 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

2 years ago
I'll file a follow up bug to fix the observer issues properly. Thanks Ed!

Comment 22

2 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
Assignee

Updated

2 years ago
Blocks: 1407393
https://hg.mozilla.org/mozilla-central/rev/b3109db72d87
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Comment 24

2 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

2 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

2 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+
(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

2 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)
You need to log in before you can comment on or make changes to this bug.