1411416 - browser_siteData.js leaks an nsStringBuffer allocated by layout calling into networking

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P3)

Description

[Andrew McCreight [:mccr8]]

This front end test ends up leaking a string buffer. I'm not sure what is going wrong.

Here's the allocation stack for the nsStringBuffer:

#00: already_AddRefed (/media/ssd/mc/obj-dbg.noindex/dist/include/mozilla/AlreadyAddRefed.h:72)
#01: already_AddRefed<nsStringBuffer>::take() (/media/ssd/mc/obj-dbg.noindex/dist/include/mozilla/AlreadyAddRefed.h:121)
#02: nsTSubstring<char>::ReplacePrepInternal(unsigned int, unsigned int, unsigned int, unsigned int) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:228)
#03: nsTSubstring<char>::ReplacePrep(unsigned int, unsigned int, unsigned int) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:219)
#04: nsTSubstring<char>::Assign(char const*, unsigned int, mozilla::fallible_t const&) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:389)
#05: nsTSubstring<char>::Assign(nsTSubstring<char> const&, mozilla::fallible_t const&) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:484 (discriminator 2))
#06: nsTSubstring<char>::Assign(nsTSubstring<char> const&) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:440)
#07: nsEffectiveTLDService::GetBaseDomainInternal(nsTString<char>&, int, nsTSubstring<char>&) (/media/ssd/mc/obj-dbg.noindex/dist/include/nsTString.h:141)
#08: nsEffectiveTLDService::GetBaseDomain(nsIURI*, unsigned int, nsTSubstring<char>&) (/media/ssd/mc/netwerk/dns/nsEffectiveTLDService.cpp:133)
#09: nsLayoutUtils::IsInStyloBlocklist(nsIPrincipal*) (/media/ssd/mc/layout/base/nsLayoutUtils.cpp:8048)

Comment 1

[Andrew McCreight [:mccr8]]

The leaked string is "example.org".

Comment 2

[Andrew McCreight [:mccr8]]

I got a new stack using a no-opt build, and the allocation is happening on this line in nsEffectiveTLDService::GetBaseDomainInternal:
  entry->mBaseDomain = aBaseDomain;

better stack:
#00: NS_LogAddRef (/media/ssd/mc/xpcom/base/nsTraceRefcnt.cpp:1037)
#01: nsStringBuffer::Alloc(unsigned long) (/media/ssd/mc/xpcom/string/nsSubstring.cpp:246)
#02: nsTSubstring<char>::MutatePrep(unsigned int, char**, mozilla::detail::StringDataFlags*) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:165)
#03: nsTSubstring<char>::ReplacePrepInternal(unsigned int, unsigned int, unsigned int, unsigned int) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:228)
#04: nsTSubstring<char>::ReplacePrep(unsigned int, unsigned int, unsigned int) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:217)
#05: nsTSubstring<char>::Assign(char const*, unsigned int, mozilla::fallible_t const&) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:389)
#06: nsTSubstring<char>::Assign(nsTSubstring<char> const&, mozilla::fallible_t const&) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:484)
#07: nsTSubstring<char>::Assign(nsTSubstring<char> const&) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:440)
#08: nsTString<char>::operator=(nsTSubstring<char> const&) (/media/ssd/mc/xpcom/string/nsTString.h:142)
#09: nsEffectiveTLDService::GetBaseDomainInternal(nsTString<char>&, int, nsTSubstring<char>&) (/media/ssd/mc/netwerk/dns/nsEffectiveTLDService.cpp:314)
#10: nsEffectiveTLDService::GetBaseDomain(nsIURI*, unsigned int, nsTSubstring<char>&) (/media/ssd/mc/netwerk/dns/nsEffectiveTLDService.cpp:133)

Comment 3

[Andrew McCreight [:mccr8]]

Eric, do you have any ideas? I don't know how this could leak, unless something is going weird inside the string stuff, which I am not familiar with.

Comment 4

[Andrew McCreight [:mccr8]]

Eric pointed out that something is probably getting a reference via the etld cache and leaking that way. I'll investigate some more.

Comment 5

[Andrew McCreight [:mccr8]]

I used refcount logging, with XPCOM_MEM_LOG_OBJECTS=16100-16300 to limit it only to a few hundred objects. Then I used make-tree.pl --ignore-balanced. (The ignore-balanced option works amazingly well when you don't have tons of weird places where you addref, then assign to a ** argument.)

Anyways, AFAICT this is the stack forthe reference that is never released:

<nsStringBuffer> 0x7f43a3ef7660 16200 AddRef 4 [thread 0x7f43d02995c0]
    #01: nsTSubstring<char>::Assign(nsTSubstring<char> const&) (/media/ssd/mc/xpcom/string/nsTSubstring.cpp:440)
    #02: nsEffectiveTLDService::GetBaseDomainInternal(nsTString<char>&, int, nsTSubstring<char>&) (/media/ssd/mc/netwerk/dns/nsEffectiveTLDService.cpp:224)
    #03: nsEffectiveTLDService::GetBaseDomain(nsIURI*, unsigned int, nsTSubstring<char>&) (/media/ssd/mc/netwerk/dns/nsEffectiveTLDService.cpp:135)
    #04: ThirdPartyUtil::GetBaseDomain(nsIURI*, nsTSubstring<char>&) (/media/ssd/mc/dom/base/ThirdPartyUtil.cpp:294 (discriminator 2))
    #05: ContentPrincipal::GetBaseDomain(nsTSubstring<char>&) (/media/ssd/mc/caps/ContentPrincipal.cpp:416 (discriminator 2))
    #06: nsOfflineCacheUpdateService::AllowOfflineApp(nsIPrincipal*) (/media/ssd/mc/uriloader/prefetch/nsOfflineCacheUpdateService.cpp:718)
    #07: nsContentUtils::MaybeAllowOfflineAppByDefault(nsIPrincipal*) (/media/ssd/mc/dom/base/nsContentUtils.cpp:2156)
    #08: nsContentSink::ProcessOfflineManifest(nsTSubstring<char16_t> const&) (/media/ssd/mc/dom/base/nsContentSink.cpp:1173)
    #09: nsHtml5TreeOpExecutor::FlushSpeculativeLoads() (/media/ssd/mc/parser/html/nsHtml5TreeOpExecutor.cpp:305)
    #10: nsHtml5LoadFlusher::Run() (/media/ssd/mc/parser/html/nsHtml5StreamParser.cpp:148)

Comment 6

[Andrew McCreight [:mccr8]]

That stack let me to this bit of code:
  nsAutoCString domain;
  rv = aPrincipal->GetBaseDomain(domain);
  ...
  nsOfflineCacheUpdateService::AllowedDomains()->PutEntry(domain);

That stores domain into a hashtable, nsOfflineCacheUpdateService::mAllowedDomains:
   static nsTHashtable<nsCStringHashKey>* mAllowedDomains;

Nothing ever deletes that hash table, so we leak. Deleting it in the dtor of nsOfflineCacheUpdateService fixes the leak.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1411416 - Don't leak nsOfflineCacheUpdateService::mAllowedDomains.

Review commit: https://reviewboard.mozilla.org/r/193406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193406/

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this because it doesn't meet the MozReview review requirements.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits

Comment 9

[Andrew McCreight [:mccr8]]

Yeah, I was hoping somebody would land this on inbound for me, which is why I set the flag. MozReview doesn't seem to know that the flag has been r+'d.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc99deed64c5
Don't leak nsOfflineCacheUpdateService::mAllowedDomains. r=mayhemer

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bc99deed64c5