Closed Bug 1229760 Opened 4 years ago Closed 4 years ago

CustomElement registry keeps strong reference to the unresolved elements

Categories

(Core :: DOM: Core & HTML, defect)

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- affected
firefox43 + wontfix
firefox44 + fixed
firefox45 + fixed
firefox-esr38 45+ fixed
b2g-v2.5 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

I'm debugging a runtime leak dbaron had on slack.com and there the CC log had lots of ts-message elements keeping subgraphs alive.
Based on Release calls the unknown edge is
#0  0x00007fd3d85ffdcd in mozilla::dom::FragmentOrElement::Release() (this=0x7fd3a50496b0) at /home/smaug/mozilla/hg/push-m-i/dom/base/FragmentOrElement.cpp:1976
#1  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (aPtr=0x7fd3a50496b0) at ../../dist/include/mozilla/RefPtr.h:362
#2  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (aPtr=0x7fd3a50496b0) at ../../dist/include/mozilla/RefPtr.h:372
#3  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (this=<optimized out>) at ../../dist/include/mozilla/RefPtr.h:56
#4  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (aE=<optimized out>) at ../../dist/include/nsTArray.h:520
#5  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (this=0x7fd3a845f5e0, aStart=0, aCount=<optimized out>)
    at ../../dist/include/nsTArray.h:2010
#6  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (this=0x7fd3a845f5e0, aStart=0, aCount=<optimized out>)
    at ../../dist/include/nsTArray.h:1652
#7  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (this=0x7fd3a845f5e0) at ../../dist/include/nsTArray.h:1662
#8  0x00007fd3d86c18af in nsTArray_Impl<RefPtr<mozilla::dom::Element>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() (this=0x7fd3a845f5e0) at ../../dist/include/nsTArray.h:825
#9  0x00007fd3d86c288b in nsTHashtable<nsBaseHashtableET<mozilla::dom::CustomElementHashKey, nsAutoPtr<nsTArray<RefPtr<mozilla::dom::Element> > > > >::s_ClearEntry(PLDHashTable*, PLDHashEntryHdr*) (this=<optimized out>) at ../../dist/include/nsAutoPtr.h:74
#10 0x00007fd3d86c288b in nsTHashtable<nsBaseHashtableET<mozilla::dom::CustomElementHashKey, nsAutoPtr<nsTArray<RefPtr<mozilla::dom::Element> > > > >::s_ClearEntry(PLDHashTable*, PLDHashEntryHdr*) (this=<optimized out>) at ../../dist/include/nsBaseHashtable.h:295
#11 0x00007fd3d86c288b in nsTHashtable<nsBaseHashtableET<mozilla::dom::CustomElementHashKey, nsAutoPtr<nsTArray<RefPtr<mozilla::dom::Element> > > > >::s_ClearEntry(PLDHashTable*, PLDHashEntryHdr*) (aTable=<optimized out>, aEntry=0x7fd3a98fac90) at ../../dist/include/nsTHashtable.h:407
#12 0x00007fd3d79e6301 in PLDHashTable::~PLDHashTable() (this=0x7fd3a51593b8) at /home/smaug/mozilla/hg/push-m-i/xpcom/glue/PLDHashTable.cpp:335
#13 0x00007fd3d867c091 in mozilla::dom::Registry::~Registry() (this=<optimized out>) at ../../dist/include/nsTHashtable.h:349
#14 0x00007fd3d867c091 in mozilla::dom::Registry::~Registry() (this=0x7fd3a5159380) at /home/smaug/mozilla/hg/push-m-i/dom/base/nsDocument.cpp:407
#15 0x00007fd3d867c091 in mozilla::dom::Registry::~Registry() (this=0x7fd3a5159380) at /home/smaug/mozilla/hg/push-m-i/dom/base/nsDocument.cpp:405
#16 0x00007fd3d797a283 in SnowWhiteKiller::~SnowWhiteKiller() (this=0x7ffc14530030) at /home/smaug/mozilla/hg/push-m-i/xpcom/base/nsCycleCollector.cpp:2655
#17 0x00007fd3d79751e8 in nsPurpleBuffer::RemoveSkippable(nsCycleCollector*, bool, bool, void (*)()) (this=<optimized out>) at /home/smaug/mozilla/hg/push-m-i/xpcom/base/nsCycleCollector.cpp:2759
#18 0x00007fd3d79751e8 in nsPurpleBuffer::RemoveSkippable(nsCycleCollector*, bool, bool, void (*)()) (this=<optimized out>, aCollector=<optimized out>, aRemoveChildlessNodes=<optimized out>, aAsyncSnowWhiteFreeing=<optimized out>, aCb=<optimized out>) at /home/smaug/mozilla/hg/push-m-i/xpcom/base/nsCycleCollector.cpp:2800
#19 0x00007fd3d79753a9 in nsCycleCollector::ForgetSkippable(bool, bool) (this=0x7fd3c1be7000, aRemoveChildlessNodes=false, aAsyncSnowWhiteFreeing=false)
    at /home/smaug/mozilla/hg/push-m-i/xpcom/base/nsCycleCollector.cpp:2847

Why are we using Registry for anything on web pages even when web components are disabled?
mCandidatesMap keeps a strong reference to some elements and RegisterUnresolvedElement seems to add to that hashtable.
For some reason mRegistry is created for all the documents.

The lifetime management setup here is quite wrong. mCandidatesMap shouldn't keep anything alive.
WeakPtr there might be ok. But then, when web components are disabled for the document, 
mRegistry should be null.
Testcase here is
data:text/html,<script>document.createElement("leaky-element").expando = "foobar";</script>
(that expando is the thing which makes the element to show up in CC graph. Without it, CC doesn't know about it, and there is just a leak until the tab is closed.)
[Tracking Requested - why for this release]:
This is a bad leak, and shows up in real world web sites.
Makes cycle collection and garbage collection slower too.
So this only affects elements whose tagnames contain '-' or which have an "is" attribute, right?

I agree that it makes sense to just not set up mRegistry if web components are not enabled.  And yes, mCandidatesMap should definitely not keep things alive.  :(
The previous testcase was about '-' usage, 
data:text/html,<script> var e = document.createElement("div"); e.setAttribute("is", "foo-bar"); e.cloneNode().expand = "bar"; </script>
works for 'is'.
I'm not super happy with nsWeakPtr usage either, since it causes quite a bit more memory usage.
Could we use NS_EVENT_STATE_UNRESOLVED state here?

mCandidatesMap would keep raw reference to Elements and then when an element is about to die, 
NodeUtils::LastRelease or Element::~Element would check that state and if set, remove element from the hashtable.
wchen, would that work?


Looks like :unresolved CSS pseudo class is enabled currently :/
data:text/html,<style>div { border: 5px solid green; } :unresolved { border: 5px solid red; } </style><div>foo</div><div is="foo-bar">bar</div>
Flags: needinfo?(wchen)
Would need to handle also adopt() if NS_EVENT_STATE_UNRESOLVED is used.
Hmm, but I don't understand what is the key in the hashtable.
Is is guaranteed that the element is only once in that hashtable?
Attached patch nsWeakPtr (obsolete) — Splinter Review
Given that we want this for branches too (including b2g which uses web components), perhaps this is the safest option after all. I guess
elements with '-' aren't used that often.
This doesn't clear the array after elements are deleted, so there is still a small runtime leak until the relevant page is closed, but at least we aren't keeping large object graphs alive anymore, nor causing higher CC/GC times.
But I can't think of anything safer for now. This keeps :unresolved working (whether or not we want that is a separate issue, but changing that for beta for example has some risk)

(Relying on NS_EVENT_STATE_UNRESOLVED looks scarier to me.)
Assignee: nobody → bugs
Attachment #8694827 - Flags: review?(wchen)
er, that is missing a null check.
Attached patch +null checkSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=268b793ca06a
Attachment #8694827 - Attachment is obsolete: true
Attachment #8694827 - Flags: review?(wchen)
Attachment #8694831 - Flags: review?(wchen)
Depends on: 1229846
Summary: CustomElement registry seems to be used even without web components being enabled → CustomElement registry keeps strong reference to the unresolved elements
Attachment #8694831 - Flags: review?(wchen) → review+
(In reply to Olli Pettay [:smaug] from comment #7)
> I'm not super happy with nsWeakPtr usage either, since it causes quite a bit
> more memory usage.
> Could we use NS_EVENT_STATE_UNRESOLVED state here?
> 
> mCandidatesMap would keep raw reference to Elements and then when an element
> is about to die, 
> NodeUtils::LastRelease or Element::~Element would check that state and if
> set, remove element from the hashtable.
> wchen, would that work?
I think so, although type extension makes it a bit trickier. If an unresolved element is a candidate via type extension (i.e. using the "is" attribute) then it will be put in the candidate table for the custom element type and changing the "is" attribute later is specified to have no effect on the custom element type. When the element is destroyed, the "is" attribute may have been removed or changed so we don't know what key to use in the candidate table and would have to walk through all the entries.
Flags: needinfo?(wchen)
Attached patch for branchesSplinter Review
[Feature/regressing bug #]:Bug 984712
[User impact if declined]: leaks and higher CC and GC pauses
[Describe test coverage new/current, TreeHerder]: "doesn't break existing tests"
[Risks and why]: Should be safe. web components aren't enabled by default anyway
                 and I tried to take the safest possible fix here so that it could land to branches too
                 (the fix isn't perfect since it leaves nsWeakPtrs to the arrays, but at least we have a lot
                  smaller runtime leak, which is released when tab is closed, and CC/GC times should stay low).
[String/UUID change made/needed]: NA
Attachment #8695348 - Flags: approval-mozilla-esr38?
Attachment #8695348 - Flags: approval-mozilla-beta?
Attachment #8695348 - Flags: approval-mozilla-aurora?
Tracked since memleaks are bad.
https://hg.mozilla.org/mozilla-central/rev/f297b2b18be7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8695348 [details] [diff] [review]
for branches

Fix for memory leak, last minute but I'd like this in aurora and beta. It should be in the RC build next Monday.
Attachment #8695348 - Flags: approval-mozilla-beta?
Attachment #8695348 - Flags: approval-mozilla-beta+
Attachment #8695348 - Flags: approval-mozilla-aurora?
Attachment #8695348 - Flags: approval-mozilla-aurora+
DBaron, would you be able to verify on a the latest Nightly build whether the leaks you were seeing on slack.com are gone? Trying to get a quick verification on this one as Liz and I are planning to uplift this to Beta and Aurora soon. Thanks!
Flags: needinfo?(dbaron)
Verification is not trivial; it requires going through CC logs, which smaug is more qualified to do than I am, and which I think he's already done.
Flags: needinfo?(dbaron)
(I think you should just uplift; it's much easier for me to conclude that the patch will fix leaks by looking at the patch than by testing.)
Adding tracking flags so this stays on my radar next week.
this has problems during uplift to aurora like :
grafting 318308:f297b2b18be7 "Bug 1229760 - CustomElement registry keeps strong reference to the unresolved elements, r=wchen"
merging dom/base/nsDocument.cpp
merging dom/base/nsDocument.h
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(bugs)
The patch for branches apply locally just fine to aurora. Did you perhaps use the patch for nightly?
Flags: needinfo?(bugs) → needinfo?(cbook)
Is it still possible to get this in to 43?
Flags: needinfo?(lhenry)
I already started the RC build. We are definitely going to be doing a dot release quickly (for SHA-1 deprecation).  I can approve this for m-b and m-r now. If we end up with an RC2 this can be in 43.0; if not then it can make it into 43.0.1 as a ridealong.
Flags: needinfo?(lhenry)
Attachment #8695348 - Flags: approval-mozilla-release?
Comment on attachment 8695348 [details] [diff] [review]
for branches

Approved for m-r; this should go into either an rc2 or the planned 43.0.1 next week
Attachment #8695348 - Flags: approval-mozilla-release? → approval-mozilla-release+
(In reply to David Baron [:dbaron] ⌚UTC-5 from comment #25)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/fcbcabd1ca3b

when moving this to beta i get :

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r fcbcabd1ca3b
grafting 318876:fcbcabd1ca3b "Bug 1229760 - CustomElement registry keeps strong reference to the unresolved elements, r=wchen, a=lizzard"
merging dom/base/nsDocument.cpp
merging dom/base/nsDocument.h
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsDocument.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take look, thanks!
Flags: needinfo?(cbook)
Attached patch for betaSplinter Review
Sorry about that. There was nsRefPtr -> RefPtr renaming in Aurora, so beta has
still nsRefPtr
since this got lost in irc somehow: lizzard, shall this still get into beta and release ? or did this missed the deadlines ?
Flags: needinfo?(lhenry)
Comment on attachment 8697054 [details] [diff] [review]
for beta





We have a lot going on right now with 43; since this patch bounced and we don't have a lot of time or resources to verify it at the moment, I think it  is better to keep this in 44. Sorry.
Flags: needinfo?(lhenry)
Attachment #8697054 - Flags: approval-mozilla-release-
Attachment #8697054 - Flags: approval-mozilla-beta-
Comment on attachment 8697054 [details] [diff] [review]
for beta

Just saw some more usage for slack even for mozilla stuff
aframevr.slack.com

This bug makes FF behave really badly with slack.com, so if we take any uplifts to FF43, this should be considered.
Attachment #8697054 - Flags: approval-mozilla-release- → approval-mozilla-release?
Discussed with dbaron. This would be nice to fix, but I'd like to minimize new risks in the coming 43.0.2 release which is happening just at the holidays.   Let's keep this in 44.
Comment on attachment 8697054 [details] [diff] [review]
for beta

Wontfix for 43, we want to keep risk as low as we can for this dot release.
Attachment #8697054 - Flags: approval-mozilla-release? → approval-mozilla-release-
Comment on attachment 8695348 [details] [diff] [review]
for branches

Fixing a major memory leak seems good to get on ESR, for stability. Please uplift for 38.6.0esr.
Attachment #8695348 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
This missed uplift as it wasn't marked as affected for esr38. smaug are you sure it is an issue for esr?  Even if so we may want to delay this till 45esr.
Flags: needinfo?(bugs)
After talking with smaug: let's aim for 38.7.0.
still has problems to uplift to esr38

grafting 318876:fcbcabd1ca3b "Bug 1229760 - CustomElement registry keeps strong reference to the unresolved elements, r=wchen, a=lizzard"
merging dom/base/nsDocument.cpp
merging dom/base/nsDocument.h
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsDocument.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
The patch for beta should work with esr38
attachment 8697054 [details] [diff] [review]

You may need to use some --fuzz
Flags: needinfo?(bugs)
(In reply to Wes Kocher (:KWierso) from comment #42)
> https://hg.mozilla.org/releases/mozilla-esr38/rev/11e661475655

Whoops, forgot to make sure the patch author was set correctly.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.