1652228 - Perma layout/style/test/test_flexbox_reflow_counts.html | bug in test; 'childToPreserve' should be child of 'parent' when Gecko 80 switches to late beta on 2020-08-07

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

central-as-late-beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=911887fd1f3b6f8ab1a5caf5ddc991ec5c7bbb09&selectedTaskRun=AJ7DtxzqSsSP9_aWFbxWQA.0

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=309301573&repo=try

TEST-UNEXPECTED-FAIL | layout/style/test/test_flexbox_reflow_counts.html | bug in test; 'childToPreserve' should be child of 'parent'
TEST-UNEXPECTED-FAIL | layout/style/test/test_flexbox_reflow_counts.html | uncaught exception - TypeError: parent.appendChild is not a function at removeChildrenExcept@http://mochi.test:8888/tests/layout/style/test/test_flexbox_reflow_counts.html:79:12

layout.dynamic-reflow-roots.enabled is only enabled for EARLY_BETA_OR_EARLIER.

Comment 1

[Daniel Holbert [:dholbert]]

Interesting! This wasn't intended to depend on layout.dynamic-reflow-roots.enabled. Do we know for sure that that's the pref involved here?

(I just tried running this mochitest manually, with that pref set to false, and it passed without issues. So I suspect there might be something else involved instead/also.)

In any case, I'll take a look here.

Comment 2

[Daniel Holbert [:dholbert]]

It looks like the problem is that the automatically-defined varialble content is actually a pointer to window, in Firefox release & late-beta (but not in Firefox Nightly or in Chrome).

Whereas, the test is expecting it to be a pointer to the <div id="content"> node.

I don't know what the explanation is for this magic window behavior, but maybe it's a quirk that we're getting rid of? (hence the maybe-new nightly-specific behavior that happens to match Chrome)

In any case, we can easily fix this by just explicitly declaring the content variable, instead of relying on the automagic behavior.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)

I don't know what the explanation is for this magic window behavior, but maybe it's a quirk that we're getting rid of? (hence the maybe-new nightly-specific behavior that happens to match Chrome)

Yeah, looks like this is the thing in bug 1632116. The relevant pref that changes behavior here (in the late beta simulation) is dom.window.content.untrusted.enabled.

Comment 4

[Daniel Holbert [:dholbert]]

Not that we need it, but just to illustrate for anyone curious: here's a minimized testcase for demonstrating the problem here -- load this data URI, with web console (ctrl+shift+k) open:

data:text/html,<div id="content"></div><script>console.log(content)</script>

In Nightly, this logs the <div> (as the mochitest expects that it would).
In Firefox release (v78), this logs the Window, and then it prints the following warning about this behavior being deprecated:
The ‘content’ attribute of Window objects is deprecated. Please use ‘window.top’ instead.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Daniel Holbert [:dholbert] from comment #1)

Interesting! This wasn't intended to depend on layout.dynamic-reflow-roots.enabled. Do we know for sure that that's the pref involved here?

(I just tried running this mochitest manually, with that pref set to false, and it passed without issues. So I suspect there might be something else involved instead/also.)

No. Thank you for identifying the related pref.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1652228: In test_flexbox_reflow_counts.html, don't bother pruning (nonexistent) children from 'content' node, and use consistent vars. r?TYLin

Before this patch, the test tries to remove all children from the 'content'
node except for one, as part of cleaning up. This is unnecessary, because none
of the subtests ever add any additional children to the 'content' element.
(It's also problematic because in late beta & release, 'content' is a special
variable name by default.)

While we're at it, this patch also makes us address the other nodes more
consistently, using the explicit variable declarations at the top of the
script section rather than their implicit ID-granted variable names.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afca8223767c
In test_flexbox_reflow_counts.html, don't bother pruning (nonexistent) children from 'content' node, and use consistent vars. r=TYLin

Comment 8

[Andreea Pavel [:apavel]]

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

Comment 9

[Andreea Pavel [:apavel]]

Verified fix in latest beta sim: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=d9ddb487dc8fd97c1d64b4e386bcf4ee0690dda1&searchStr=mochitest%2Cplain