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

RESOLVED FIXED in Firefox 58

Status

()

Core
Networking: Cache
P3
normal
RESOLVED FIXED
26 days ago
18 days ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {mlk})

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [MemShrink:P2][necko-triaged])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

26 days ago
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)
Keywords: mlk
(Assignee)

Comment 1

26 days ago
The leaked string is "example.org".
(Assignee)

Comment 2

26 days ago
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)
(Assignee)

Comment 3

26 days ago
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.
Flags: needinfo?(erahm)
Whiteboard: [MemShrink]
(Assignee)

Comment 4

26 days ago
Eric pointed out that something is probably getting a reference via the etld cache and leaking that way. I'll investigate some more.
Flags: needinfo?(erahm)
(Assignee)

Comment 5

26 days ago
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)
(Assignee)

Comment 6

26 days ago
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.
Assignee: nobody → continuation
Component: XPCOM → Networking: Cache
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → wontfix
Priority: -- → P3
Whiteboard: [MemShrink] → [MemShrink][necko-triaged]
Comment hidden (mozreview-request)
(Assignee)

Updated

19 days ago
Blocks: 1411259
Attachment #8922353 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

19 days ago
Keywords: checkin-needed
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
Keywords: checkin-needed
(Assignee)

Comment 9

19 days ago
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.
Keywords: checkin-needed
Whiteboard: [MemShrink][necko-triaged] → [MemShrink:P2][necko-triaged]

Comment 10

19 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc99deed64c5
Don't leak nsOfflineCacheUpdateService::mAllowedDomains. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bc99deed64c5
Status: NEW → RESOLVED
Last Resolved: 18 days ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.