Closed Bug 1361235 Opened 7 years ago Closed 7 years ago

stylo: <select> and other tags leak during shutdown after scrollbar sheet is enabled

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: TYLin, Assigned: heycam)

References

Details

Attachments

(3 files)

After applying patches in Bug 1321754, there are leaks during shutdown like 

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       39      688| 1331127       10|
 434 |URLExtraData                          |       32       64|     583        2|
 532 |nsAuthURLParser                       |       24       24|       2        1|
 740 |nsJSPrincipals                        |       24       24|     393        1|
 832 |nsStandardURL                         |      272      544|    4037        2|
 840 |nsStringBuffer                        |        8       32|   61823        4|

So, I disabled a bunch of crashtests in Part 2. They're fall into three categories.
1. <select> tag. (Either static or dynamic added by js)
2. <keygen> tag.
3. <fieldset> tag with "overflow" property.

All of these tags creates SliderFrame(slider) and ButtonBoxFrame(thumb) under ScrollbarFrame.
Summary: stylo: <select> and other tags leaks during shutdown after scrollbar sheet is enabled → stylo: <select> and other tags leak during shutdown after scrollbar sheet is enabled
Presumaby we havent implemented the UpdateStyleOfOwnedAnonBoxes thing for them? I don't see why that'd necessarily leak though...
Those aren't anon boxes.  They're just NAC with normal styles on it.  But it's possible that we don't have a way to reach that NAC.

This looks a heck of a lot like the leak in bug 1341973, by the say: NAC bits are involved, the same kinds of objects are leaked, etc.
Depends on: 1341973
Hmm, oh, good point...

So I just tweaked Element::List to use AllChildrenIterator, and this is a content tree dump for dom/base/crashtests/595606-1.html after load.

html@0x7f2503f78880 state=[100020000000] flags=[00100008] primaryframe=0x7f2505a439e8 refcount=13<
  head@0x7f2503f78940 state=[100020000000] flags=[00100000] primaryframe=(nil) refcount=2<>
  body@0x7f2503c4eba0 onload="
    document.body.removeChild(document.getElementById('x'));
    document.documentElement.removeAttribute('class');" state=[100020000000] flags=[00100000] primaryframe=0x7f2505a43d40 refcount=7<
    Text@0x7f2505a5d110 flags=[1c000000] primaryframe=0x7f2505ae6218 refcount=2<\u000a\u000a    >
    Text@0x7f2505a5d740 flags=[1c000000] primaryframe=0x7f2505ae7160 refcount=2<\u000a\u000a    >
    select@0x7f2503ccbaa0 form="a" state=[100020400408] flags=[40101004] primaryframe=0x7f2505ae6748 refcount=9<
      Text@0x7f2505a5dfb0 flags=[08000470] primaryframe=0x7f2505ae6cc0 refcount=2<\u00a0>
      button@0x7f2505a4c400 type="button" tabindex="-1" state=[100020000008] flags=[00100474] primaryframe=0x7f2505ae6d80 refcount=3<>
    >
    Text@0x7f2505a5d7d0 flags=[1e000000] primaryframe=(nil) refcount=1<\u000a  \u000a\u000a>
  >
  XUL scrollbar@0x7f2503c4ece0 orient="horizontal" clickthrough="always" root="true" curpos="0" disabled="true" maxpos="0" pageincrement="488" increment="50" state=[20000000] flags=[00000478] primaryframe=0x7f2505a43688 refcount=2<>
  XUL scrollbar@0x7f2503c4ed80 orient="vertical" clickthrough="always" root="true" curpos="0" disabled="true" maxpos="0" pageincrement="305" increment="57" state=[20000000] flags=[00000478] primaryframe=0x7f2505a43738 refcount=2<>
  XUL scrollcorner@0x7f2503c4ee20 state=[20000000] flags=[00000470] primaryframe=0x7f2505a437e8 refcount=2<>
>


And here the frame tree dump:

Viewport(-1)@7f2505a430c8 [view=7f2505a88700] {0,0,36600,21720} [state=0100062000042220] [sc=7f2505a43140:-moz-viewport]<
  HTMLScroll(html)(-1)@7f2505a43330 {0,0,36600,21720} [state=0080020800040010] [content=7f2503f78880] [sc=7f2505a43018:-moz-viewport-scroll]<
    ScrollbarFrame(scrollbar)(-1)@7f2505a43688 next=7f2505a43738 {0,21720,36600,0} [state=0000100080c80008] [content=7f2503c4ece0] [sc=7f2505a431f0]<>
    ScrollbarFrame(scrollbar)(-1)@7f2505a43738 next=7f2505a437e8 {36600,0,0,21720} [state=0000100080880008] [content=7f2503c4ed80] [sc=7f2505a43528]<>
    Box(scrollcorner)(-1)@7f2505a437e8 next=7f2505a432a0 {36600,21720,0,0} [state=0000100080c00208] [content=7f2503c4ee20] [sc=7f2505a435d8]<>
    Canvas(html)(-1)@7f2505a432a0 {0,0,36600,21720} [state=0100006800040210] [content=7f2503f78880] [sc=7f2505a43888:-moz-scrolled-canvas]<
      Block(html)(-1)@7f2505a439e8 {0,0,36600,2370} [state=0000100800d40210] [content=7f2503f78880] [sc=7f2505a43938]<
        line 7f2505a43dd8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x48] bm=480 {480,480,35640,1410} <
          Block(body)(1)@7f2505a43d40 {480,480,35640,1410} [state=0000120800140210] [content=7f2503c4eba0] [sc=7f2505a43b30]<
            line 7f2505ae7050: count=3 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,2200,1410} <
              Text(0)"\n\n    "@7f2505ae6218 next=7f2505ae7160 {0,990,0,0} [state=4000000028200000] [content=7f2505a5d110] [sc=7f2505a43e28:-moz-text] [run=7f2503cceaf0][0,6,T] 
              Text(1)"\n\n    "@7f2505ae7160 next=7f2505ae6748 {0,990,0,0} [state=4000000028200000] [content=7f2505a5d740] [sc=7f2505a44230:-moz-text] [run=7f2503cceaf0][0,6,T] 
              ComboboxControl(select)(2)@7f2505ae6748 {0,0,2200,1410} [state=0180004800940030] [content=7f2503ccbaa0] [sc=7f2505a43f88]<
                line 7f2505ae7000: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1c0] {120,120,3160,1170} <
                  ComboboxDisplay(select)(2)@7f2505ae6c20 next=7f2505ae6d80 {120,120,1960,1170} [state=0000000800100008] [content=7f2503ccbaa0] [sc=7f2505a44428:-moz-display-comboboxcontrol-frame]<
                    line 7f2505ae6d30: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {240,60,240,1050} <
                      Text(-1)"\u00a0"@7f2505ae6cc0 {240,60,240,1050} [state=01000048b0600000] [content=7f2505a5dfb0] [sc=7f2505a44830:-moz-text] [run=7f2505a5e4c0][0,1,T] 
                    >
                  >
                  HTMLButtonControl(button)(-1)@7f2505ae6d80 {2080,120,0,1170} [state=0080000000000208] [content=7f2505a4c400] [sc=7f2505a446d0]<
                    Block(button)(-1)@7f2505ae6f68 {600,120,0,0} [state=0000100000d00000] [content=7f2505a4c400] [sc=7f2505ae6eb8:-moz-button-content]<
                    >
                  >
                >
                SelectPopupList 0x7f2505ae6810 <
                  ListControl(select)(2)@7f2505ae6890 [view=7f25125acb80] {0,1410,2200,1170} [state=0084001000006010] [content=7f2503ccbaa0] [sc=7f2505a442e0:-moz-dropdown-list]<
                    ScrollbarFrame(scrollbar)(-1)@7f2505ae6b70 next=7f2505ae6ad0 {2140,60,0,1050} [state=0004100080884008] [content=7f2503f6fe80] [sc=7f2505a44180]<>
                    Block(select)(2)@7f2505ae6ad0 {120,60,2020,0} vis-overflow=0,0,2020,1050 scr-overflow=0,0,2020,1050 [state=0104000000904000] [content=7f2503ccbaa0] [sc=7f2505a44588:-moz-scrolled-content]<
                    >
                  >
                >
              >
            >
          >
        >
      >
    >
  >
>


So at a glance the content we're missing is the one of the ScrollbarFrame... If that's an element, it should clear the servo data from UnbindFromTree when detached, and if not, it should not have any data attached to it, so it should not leak anything I know of... I'd find weird we'd miss an UnbindFromTree there, but maybe... I'll keep digging.
Oh, so anonymous content is destroyed async via a runnable... May that be why? It shouldn't...
See Also: → 1363000
Attached patch WIPSplinter Review
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Oh, so anonymous content is destroyed async via a runnable... May that be
> why? It shouldn't...

I think you're right, at least for <select>.  I tried with this patch, and it fixes the leak from min-intrinsic-with-max-width-percents-across-form-controls.html (from bug 1341973) for me locally.
Great! There are more testes in autoland that I did skip in bug 1356916.  Please enable them too. Thank you!
Comment on attachment 8866703 [details]
Bug 1361235 - Part 1: Clear ServoElementData from doomed NAC before adding the script runner that would destroy it.

https://reviewboard.mozilla.org/r/138328/#review141550

::: dom/base/nsContentUtils.cpp:5308
(Diff revision 2)
>  /* static */
>  void
>  nsContentUtils::DestroyAnonymousContent(nsCOMPtr<nsIContent>* aContent)
>  {
>    if (*aContent) {
> +    // Don't wait until UnbindFromTree to clear ServoElementData, since

This looks fine, but we should really reach this from AllChildrenIterator. Could you file a followup pointing to this bug so it's eventually investigated?
Attachment #8866703 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8866704 [details]
Bug 1361235 - Part 2: Re-enable some tests.

https://reviewboard.mozilla.org/r/138330/#review141552

Thanks for going over these :)
Attachment #8866704 - Flags: review?(emilio+bugs) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> This looks fine, but we should really reach this from AllChildrenIterator.
> Could you file a followup pointing to this bug so it's eventually
> investigated?

If we changed ServoRestyleManager::ClearServoDataFromSubtree (as called from ServoStyleSet::BeginShutdown) to use AllChildrenIterator?  I think that still won't help, since AIUI (although I haven't checked) we've destroyed the frame tree before the ServoStyleSet::BeginShutdown call, and each frame's nsFrame::DestroyFrom call will clear the element's primary frame pointer.  This means an AllChildrenIterator wouldn't be able to find the NAC.
(In reply to Cameron McCormack (:heycam) from comment #14)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> > This looks fine, but we should really reach this from AllChildrenIterator.
> > Could you file a followup pointing to this bug so it's eventually
> > investigated?
> 
> If we changed ServoRestyleManager::ClearServoDataFromSubtree (as called from
> ServoStyleSet::BeginShutdown) to use AllChildrenIterator?  I think that
> still won't help, since AIUI (although I haven't checked) we've destroyed
> the frame tree before the ServoStyleSet::BeginShutdown call, and each
> frame's nsFrame::DestroyFrom call will clear the element's primary frame
> pointer.  This means an AllChildrenIterator wouldn't be able to find the NAC.

Well, you don't need AllChildrenIterator, StyleChildrenIterator is basically it (we don't skip stuff anymore). But you might be right. Still need to find it for restyling though, right?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> Well, you don't need AllChildrenIterator, StyleChildrenIterator is basically
> it (we don't skip stuff anymore). But you might be right. Still need to find
> it for restyling though, right?

Restyling will happen while the frame exists, which is where the StyleChildrenIterator grabs the anonymous content through.
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7641287c389f
Part 1: Clear ServoElementData from doomed NAC before adding the script runner that would destroy it. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a7d409e11272
Part 2: Re-enable some tests. r=emilio
Assignee: nobody → cam
Status: NEW → ASSIGNED
(In reply to Cameron McCormack (:heycam) from comment #16)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> > Well, you don't need AllChildrenIterator, StyleChildrenIterator is basically
> > it (we don't skip stuff anymore). But you might be right. Still need to find
> > it for restyling though, right?
> 
> Restyling will happen while the frame exists, which is where the
> StyleChildrenIterator grabs the anonymous content through.

Right, but AFAIK (see comment 3, grep for "7f2503f6fe80"), we still don't find that NAC with AllChildrenIterator, even when we have the frame, which is what I find surprising.
Oh, I see what you mean now.  Filed bug 1364361.
https://hg.mozilla.org/mozilla-central/rev/7641287c389f
https://hg.mozilla.org/mozilla-central/rev/a7d409e11272
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: