Bobby Holley (:bholley) Reporter
	Description • 7 years ago

I just landed a patch in bug 1373725 to trigger an assertion if we ever leak rule nodes. I had a green try push based on a revision from June 10th:

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

However, when it hit autoland, the assertion fired on two crashtests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1f216a97ac0633e2ffd5f9af2893e4610f7206f7

So something must have landed within the last week that causes us to leak on those tests. We might be able to bisect it, or maybe we could just debug it.

The crashtests are:
* layout/forms/crashtests/1228670.xhtml
* layout/generic/crashtests/25888-2.html

I'm disabling the crashtests for now.

	Emilio Cobos Álvarez (:emilio)
	Comment 1 • 7 years ago

Those smell a lot like the kind of thing bug 1364361 should have fixed, specially the <select>/<optgroup> one.

	Bobby Holley (:bholley) Reporter
	Comment 2 • 7 years ago

Ok, NI heycam.

	Bobby Holley (:bholley) Reporter
	Comment 3 • 7 years ago

Correction: it's actually layout/forms/crashtests/1182414.html, not layout/forms/crashtests/1228670.xhtml. The leak was just getting blamed on a later test, since that's when things got torn down.

	Bobby Holley (:bholley) Reporter
	Comment 4 • 7 years ago

And that was a red herring too. The actual failing tests are:

layout/forms/crashtests/1182414.html
layout/forms/crashtests/1279354.html

They are annotated in-tree with this bug number.

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 7 years ago

They're both reftest-print tests, and removing the class="reftest-print" makes them pass.

	Cameron McCormack (:heycam) Assignee
	Comment 6 • 7 years ago

Pretty sure the issue is because of position:fixed replication of the <range> frame.

	Cameron McCormack (:heycam) Assignee
	Comment 7 • 7 years ago

> Pretty sure the issue is because of position:fixed replication of the <range> frame.

The <input type="range"> frame, that is.


So ideally, in AllChildIterator, we would know that the <input type="range"> frame was replicated across pages, and we would be able to find those replicated frames easily, so that we could grab their NAC to iterate over them.  But I don't know whether we can easily jump from the primary nsRangeFrame to the second page's nsRangeFrame.

To solve the leak, we just need to find these NAC elements and clear their Servo data out.  For that we don't need to iterate over them at the right time (i.e. when we're iterating the children of the actual <input type="range">).  Maybe we don't need to worry about modifying AllChildIterator, because we don't really need to support any kind of re-styling during print preview?

	Cameron McCormack (:heycam) Assignee
	Comment 8 • 7 years ago

If we did need to handle restyling, then we'd also need to make sure we restyle anonymous boxes on replicated frames.

	Cameron McCormack (:heycam) Assignee
	Comment 9 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=011b6ed807c4af054463193a89583cef0762814e

	Cameron McCormack (:heycam) Assignee
	Comment 10 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4649233f838b385f5f1589976b828d510e1610c9

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8878873 [details]
Bug 1373874 - stylo: Clear Servo data from NAC created by position:fixed replicated frames too.

https://reviewboard.mozilla.org/r/150126/#review155276

::: layout/style/ServoStyleSet.cpp:154
(Diff revision 1)
> +    if (nsIFrame* pageSeq = shell->FrameConstructor()->GetPageSequenceFrame()) {
> +      auto iter = pageSeq->PrincipalChildList().begin();
> +      if (*iter) {
> +        ++iter;  // skip past first page
> +        while (*iter) {
> +          ClearServoDataFromNAC(*iter);

OK, we skipped the first page, but on non-first pages we'll end up walking everything, not just the replicated frames, right?

Can we restrict this to just the replicated frames?

	Cameron McCormack (:heycam) Assignee
	Comment 14 • 7 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8878873 [details]
Bug 1373874 - stylo: Clear Servo data from NAC created by position:fixed replicated frames too.

https://reviewboard.mozilla.org/r/150126/#review155422

Thank you!  r=me

	Pulsebot
	Comment 16 • 7 years ago

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/121b01dc9fde
stylo: Clear Servo data from NAC created by position:fixed replicated frames too. r=bz

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/121b01dc9fde