1971994 - Parent process memory leak coming from FetchIconInfo

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

From DMD:

Unreported {
  103,909 blocks in heap block record 1 of 16,216
  581,877,664 bytes (411,227,058 requested / 170,650,606 slop)
  Individual block sizes: 65,536 x 54; 49,152 x 162; 32,768 x 7,760; 16,384 x 3,942; 8,192 x 15,202; 4,096 x 7,912; 3,840 x 621; 3,584 x 1,068; 3,328 x 1,309; 3,072 x 8,744; 2,816 x 1,436; 2,560 x 1,081; 2,304 x 1,781; 2,048 x 864; 1,792 x 2,351; 1,536 x 3,626; 1,280 x 2,626; 1,024 x 8,937; 768 x 23,009; 512 x 2,142; 496 x 225; 480 x 490; 464 x 239; 448 x 229; 432 x 2,454; 416 x 2,705; 400 x 192; 384 x 69; 368 x 21; 336 x 46; 320 x 168; 304 x 48; 288 x 11; 272 x 27; 256 x 29; 240 x 32; 224 x 51; 208 x 317; 192 x 1,183; 176 x 534; 144 x 97; 128 x 47; 96 x 68
  34.24% of the heap (34.24% cumulative)
  59.85% of unreported (59.85% cumulative)
  Allocated at {
    #01: replace_malloc(unsigned long) (/Applications/Firefox Nightly.app/Contents/MacOS/libmozglue.dylib + 0x4b9fc)
    #02: moz_xmemdup (/Applications/Firefox Nightly.app/Contents/MacOS/libmozglue.dylib + 0x33764)
    #03: mozilla::storage::Statement::GetBlob(unsigned int, unsigned int*, unsigned char**) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x567c38)
    #04: mozilla::places::(anonymous namespace)::FetchIconInfo(RefPtr<mozilla::places::Database> const&, nsCOMPtr<nsIURI> const&, unsigned short, mozilla::places::IconData&) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x14add08)
    #05: mozilla::places::AsyncGetFaviconForPageRunnable::Run() (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x14ad0b0)
    #06: NS_ProcessNextEvent(nsIThread*, bool) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x1dac40)
    #07: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x4a4d94)
    #08: MessageLoop::Run() (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x464a84)
  }
}

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This seems to be continuing to grow:

Unreported {
  169,266 blocks in heap block record 1 of 16,484
  947,914,800 bytes (669,909,034 requested / 278,005,766 slop)
  Individual block sizes: 65,536 x 88; 49,152 x 264; 32,768 x 12,635; 16,384 x 6,424; 8,192 x 24,787; 4,096 x 12,893; 3,840 x 1,012; 3,584 x 1,749; 3,328 x 2,125; 3,072 x 14,257; 2,816 x 2,330; 2,560 x 1,761; 2,304 x 2,900; 2,048 x 1,408; 1,792 x 3,833; 1,536 x 5,900; 1,280 x 4,280; 1,024 x 14,515; 768 x 37,460; 512 x 3,517; 496 x 360; 480 x 800; 464 x 403; 448 x 374; 432 x 4,010; 416 x 4,396; 400 x 314; 384 x 122; 368 x 30; 336 x 70; 320 x 268; 304 x 79; 288 x 18; 272 x 50; 256 x 44; 240 x 50; 224 x 91; 208 x 534; 192 x 1,913; 176 x 870; 144 x 157; 128 x 75; 96 x 100
  42.22% of the heap (42.22% cumulative)
  66.25% of unreported (66.25% cumulative)
  Allocated at {
    #01: replace_malloc(unsigned long) (/Applications/Firefox Nightly.app/Contents/MacOS/libmozglue.dylib + 0x4b9fc)
    #02: moz_xmemdup (/Applications/Firefox Nightly.app/Contents/MacOS/libmozglue.dylib + 0x33764)
    #03: mozilla::storage::Statement::GetBlob(unsigned int, unsigned int*, unsigned char**) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x567c38)
    #04: mozilla::places::(anonymous namespace)::FetchIconInfo(RefPtr<mozilla::places::Database> const&, nsCOMPtr<nsIURI> const&, unsigned short, mozilla::places::IconData&) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x14add08)
    #05: mozilla::places::AsyncGetFaviconForPageRunnable::Run() (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x14ad0b0)
    #06: NS_ProcessNextEvent(nsIThread*, bool) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x1dac40)
    #07: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x4a4d94)
    #08: MessageLoop::Run() (/Applications/Firefox Nightly.app/Contents/MacOS/XUL + 0x464a84)
  }
}

Comment 2

[Andrew McCreight [:mccr8]]

jesup and I have also been seeing very high heap-unclassified in the main process, so possibly this is the same issue.

Comment 3

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: This seems like a pretty bad ever growing memory leak in the main process, which will be bad for any users who leave their browser open for a long time. It isn't clear when it started happening but 3 of us now have noticed similar issues in Nightly.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

I got up to 2,348,376,256 bytes before switching DMD modes

Comment 5

[Marco Bonardo [:mak]]

I suspect the data blob lifecycle is not managed properly, we GetBlob from the database, that hands us ownership of a data buffer, later code stores that in some IconInfo structs... and I suspect when the data is not effectively assigned to an owning ref pointer it will get leaked.
We must audit the GetBlob calls in FaviconHelpers.cpp and ensure there's clear ownership of the data buffer, or that we free it when it's not necessary anymore.
We should also check how we can better track this memory.

Comment 6

[Marco Bonardo [:mak]]

I think we can assume Bug 1950287 had a relation to this by spreading use of the data buffer to more cases and structs.

I know Daisuke is on PTO next week, so I may look at it. Jeff is also investigating now.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1950287

:daisuke, since you are the author of the regressor, bug 1950287, could you take a look?

For more information, please visit BugBot documentation.

Comment 8

[Marco Bonardo [:mak]]

Daisuke is on PTO next week, so I'll pick this up.

Comment 9

[Andrew McCreight [:mccr8]]

For what it is worth, I confirmed using DMD that I'm seeing the same unreported memory as comment 0. (I thought I'd started the browser in heap scan mode to get more info about what is causing the leak, but apparently not.)

Comment 10

[Andrew McCreight [:mccr8]]

I got a DMD heap scan mode log with this leak. I tried a half dozen of these blob things with allocation stacks like in comment 0, but it couldn't find any references to them in the heap. This either means that any references are encoded in some odd way that the conservative pointer analysis can't understand or the chunks of memory are just floating around with nothing holding onto them.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I think under the right conditions this loop just leaks the memory that it gets back from GetBlob

Comment 12

[Andrew McCreight [:mccr8]]

Hmm yeah data (and maybe other things like IconInfo should really be a UniquePtr, in my opinion, to prevent leaks on the many error paths in there. At least I think that's where the ownership lies.

Comment 13

[Andrew McCreight [:mccr8]]

The other caller of GetBlob, FetchMostFrecentSubPageIcon, immediately sticks the data return value into an owning string so that one looks okay.

Comment 14

[Marco Bonardo [:mak]]

Attachment: Bug 1971994 - Avoid memory leak from unowned buffer in FetchIconInfo.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d93bf629baf4
https://hg.mozilla.org/integration/autoland/rev/1e474f87fb10
Avoid memory leak from unowned buffer in FetchIconInfo. r=places-reviewers,Standard8

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1e474f87fb10

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1950287

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I have seen a high number for heap-unclassified as well, and my Nightly build was from June 5th:

3,389.05 MB (100.0%) -- explicit
├──2,537.66 MB (74.88%) ── heap-unclassified

I'll keep an eye out on the heap-unclassified usage over the next days. Maybe I should start using DMD builds again as well. Thanks for the fix!

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I still have a high percentage for heap-unclassified in my current build from June 26th:

792.71 MB (100.0%) -- explicit
├──335.20 MB (42.29%) ── heap-unclassified

I'll start running Nightly with DMD to see if that is related or another issue.