Closed
Bug 1229760
Opened 7 years ago
Closed 7 years ago
CustomElement registry keeps strong reference to the unresolved elements
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(3 files, 1 obsolete file)
4.11 KB,
patch
|
wchen
:
review+
|
Details | Diff | Splinter Review |
4.93 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-release+
lizzard
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
4.94 KB,
patch
|
lizzard
:
approval-mozilla-beta-
lizzard
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
Testcase here is data:text/html,<script>document.createElement("leaky-element").expando = "foobar";</script>
Assignee | ||
Comment 3•7 years ago
|
||
(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.)
Assignee | ||
Comment 4•7 years ago
|
||
[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.
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
tracking-firefox43:
--- → ?
tracking-firefox44:
--- → ?
tracking-firefox45:
--- → ?
![]() |
||
Comment 5•7 years ago
|
||
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. :(
Assignee | ||
Comment 6•7 years ago
|
||
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'.
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
Would need to handle also adopt() if NS_EVENT_STATE_UNRESOLVED is used.
Assignee | ||
Comment 9•7 years ago
|
||
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?
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
er, that is missing a null check.
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Summary: CustomElement registry seems to be used even without web components being enabled → CustomElement registry keeps strong reference to the unresolved elements
Updated•7 years ago
|
Attachment #8694831 -
Flags: review?(wchen) → review+
Comment 13•7 years ago
|
||
(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)
Assignee | ||
Comment 15•7 years ago
|
||
[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.
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f297b2b18be7
Status: NEW → RESOLVED
Closed: 7 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)
Comment 20•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
(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.
Comment 23•7 years ago
|
||
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)
Assignee | ||
Comment 24•7 years ago
|
||
The patch for branches apply locally just fine to aurora. Did you perhaps use the patch for nightly?
Flags: needinfo?(bugs) → needinfo?(cbook)
Updated•7 years ago
|
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)
Updated•7 years ago
|
Attachment #8695348 -
Flags: approval-mozilla-release?
Comment 28•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fcbcabd1ca3b
status-b2g-v2.5:
--- → fixed
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+
Comment 30•7 years ago
|
||
(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)
Assignee | ||
Comment 31•7 years ago
|
||
Sorry about that. There was nsRefPtr -> RefPtr renaming in Aurora, so beta has still nsRefPtr
Comment 32•7 years ago
|
||
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-
Assignee | ||
Comment 34•7 years ago
|
||
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.
status-firefox-esr38:
--- → affected
tracking-firefox-esr38:
--- → 45+
Comment 40•7 years ago
|
||
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)
Assignee | ||
Comment 41•7 years ago
|
||
The patch for beta should work with esr38 attachment 8697054 [details] [diff] [review] You may need to use some --fuzz
Flags: needinfo?(bugs)
Comment 42•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-esr38/rev/11e661475655
(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.
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•