1229760 - CustomElement registry keeps strong reference to the unresolved elements

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug]]

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?

Comment 1

[Olli Pettay [:smaug]]

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.

Comment 2

[Olli Pettay [:smaug]]

Testcase here is
data:text/html,<script>document.createElement("leaky-element").expando = "foobar";</script>

Comment 3

[Olli Pettay [:smaug]]

(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.)

Comment 4

[Olli Pettay [:smaug]]

[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.

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.  :(

Comment 6

[Olli Pettay [:smaug]]

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'.

Comment 7

[Olli Pettay [:smaug]]

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>

Comment 8

[Olli Pettay [:smaug]]

Would need to handle also adopt() if NS_EVENT_STATE_UNRESOLVED is used.

Comment 9

[Olli Pettay [:smaug]]

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?

Comment 10

[Olli Pettay [:smaug]]

Attachment: nsWeakPtr

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.)

Comment 11

[Olli Pettay [:smaug]]

er, that is missing a null check.

Comment 12

[Olli Pettay [:smaug]]

Attachment: +null check

https://treeherder.mozilla.org/#/jobs?repo=try&revision=268b793ca06a

Comment 13

[William Chen [:wchen]]

(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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f297b2b18be7

Comment 15

[Olli Pettay [:smaug]]

Attachment: for branches

[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

Comment 16

[Ritu Kothari (:ritu)]

Tracked since memleaks are bad.

Comment 17

[Wes Kocher (:KWierso)]

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

Comment 18

[Liz Henry (:lizzard)]

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.

Comment 19

[Ritu Kothari (:ritu)]

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!

Comment 20

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

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.

Comment 21

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

(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.)

Comment 22

[Liz Henry (:lizzard)]

Adding tracking flags so this stays on my radar next week.

Comment 23

[Carsten Book [:Tomcat]]

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)

Comment 24

[Olli Pettay [:smaug]]

The patch for branches apply locally just fine to aurora. Did you perhaps use the patch for nightly?

Comment 25

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fcbcabd1ca3b

Comment 26

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Is it still possible to get this in to 43?

Comment 27

[Liz Henry (:lizzard)]

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.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fcbcabd1ca3b

Comment 29

[Liz Henry (:lizzard)]

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

Comment 30

[Carsten Book [:Tomcat]]

(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!

Comment 31

[Olli Pettay [:smaug]]

Attachment: for beta

Sorry about that. There was nsRefPtr -> RefPtr renaming in Aurora, so beta has
still nsRefPtr

Comment 32

[Carsten Book [:Tomcat]]

since this got lost in irc somehow: lizzard, shall this still get into beta and release ? or did this missed the deadlines ?

Comment 33

[Liz Henry (:lizzard)]

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.

Comment 34

[Olli Pettay [:smaug]]

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.

Comment 35

[Liz Henry (:lizzard)]

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 36

[Liz Henry (:lizzard)]

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.

Comment 37

[Liz Henry (:lizzard)]

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.

Comment 38

[Liz Henry (:lizzard)]

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.

Comment 39

[Liz Henry (:lizzard)]

After talking with smaug: let's aim for 38.7.0.

Comment 40

[Carsten Book [:Tomcat]]

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)

Comment 41

[Olli Pettay [:smaug]]

The patch for beta should work with esr38
attachment 8697054 [details] [diff] [review]

You may need to use some --fuzz

Comment 42

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/11e661475655

Comment 43

[Wes Kocher (:KWierso)]

(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.