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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Presumaby we havent implemented the UpdateStyleOfOwnedAnonBoxes thing for them? I don't see why that'd necessarily leak though...

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Oh, so anonymous content is destroyed async via a runnable... May that be why? It shouldn't...

Comment 5

[Cameron McCormack (:heycam)]

Attachment: WIP

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

Comment 6

[Cameron McCormack (:heycam)]

Try run with the tests disabled in bug 1321754 part 2 re-enabled:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=827dbc46c504886b87c7d3122f30835fdeb6857b

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1361235 - Part 1: Clear ServoElementData from doomed NAC before adding the script runner that would destroy it.

Review commit: https://reviewboard.mozilla.org/r/138328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138328/

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1361235 - Part 2: Re-enable some tests.

Review commit: https://reviewboard.mozilla.org/r/138330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138330/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Great! There are more testes in autoland that I did skip in bug 1356916.  Please enable them too. Thank you!

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8866703 [details]
Bug 1361235 - Part 1: Clear ServoElementData from doomed NAC before adding the script runner that would destroy it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138328/diff/1-2/

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8866704 [details]
Bug 1361235 - Part 2: Re-enable some tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138330/diff/1-2/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

Comment 14

[Cameron McCormack (:heycam)]

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

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

Comment 16

[Cameron McCormack (:heycam)]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Emilio Cobos Álvarez (:emilio)]

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

Comment 19

[Cameron McCormack (:heycam)]

Oh, I see what you mean now.  Filed bug 1364361.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7641287c389f
https://hg.mozilla.org/mozilla-central/rev/a7d409e11272