1595285 - Do not track TRANSITION_EMBED visits for link-coloring purposes.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

Other browsers don't, plus it blocks work I want to do to query multiple links at the same time.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1595285 - Do not track TRANSITION_EMBED visits for link-coloring purposes. r=mak

Other browsers don't, plus it blocks work I want to do to query multiple links
at the same time.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1595285 - Minor cleanup of the visited query code. r=mak

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9cba679cc50d
Minor cleanup of the visited query code. r=mak

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e19c5df398de
Do not track TRANSITION_EMBED visits for link-coloring purposes. r=mak

Comment 5

[Dorel Luca [:dluca]]

Backed out changeset e19c5df398de (bug 1595285) for causing leaks on browser-chrome in toolkit/components/places/tests/browser/browser_bug461710.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=275638635&repo=autoland&lineNumber=17806

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=e19c5df398de58747cceb53d36db418d1d63f8e3

Backout:
https://hg.mozilla.org/integration/autoland/rev/56036a1ba7814f521e0027e1857d1639d2810827

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Continuing from bug 1595573 comment 4:

• So what we're leaking is the initial about blank window for both windows (that's not a lot to say, as as soon as browser.xhtml is loaded we re-use that window for that).

• The window is very active during the test. There's a lot of stuff holding onto it, including one strong pointer to itself via mTopInnerWindow.

• There's only one cc log that contains that window's address, and it's the one during shutdown. So I don't know what's going on yet, really...

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9cba679cc50d

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1595285 - Fix TestUtils.waitForCondition to not use setInterval. r=mak

The test that is timing out with these patches does something relatively simple:

await TestUtils.waitForCondition(async function() {
let color = await ContentTask.spawn(browserWindow, async function() {
/* Do stuff... */
});
return color == something;
});

await closeWindow(browserWindow);

Turns out that this can intermittently leak the window due to waitForCondition
using setInterval. setInterval can schedule multiple tasks while awaiting for
the inner ContentTask.

What this means, is that we may still have a ContentTask awaiting us when we get
to close the window. Closing the window makes the ContentTask not finish, and
thus we leak a promise keeping alive the window in gPromises:

https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/testing/mochitest/BrowserTestUtils/ContentTask.jsm#24

Which means that we keep alive the window all the way until shutdown.

Fix it by ensuring that we only run one task at a time.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffc59a6dfbd5
Fix TestUtils.waitForCondition to not use setInterval. r=mak

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b73d74bcafda
Do not track TRANSITION_EMBED visits for link-coloring purposes. r=mak

Comment 11

[Cristian Brindusan [:cbrindusan]]

Backed out changeset ffc59a6dfbd5 (Bug 1595285) for multiple bc failures:

Backout: https://hg.mozilla.org/integration/autoland/rev/2c748c06bd9b294017bd5b5d6b0967b86b580d7c

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=ffc59a6dfbd58dcc28114acc38a7ef29f55d0aab&selectedJob=276022974

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276022974&repo=autoland&lineNumber=14032
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276022677&repo=autoland&lineNumber=12475
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276022982&repo=autoland&lineNumber=6214
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276022904&repo=autoland&lineNumber=14876
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276022888&repo=autoland&lineNumber=1690

There are also dt failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276023018&repo=autoland&lineNumber=2203

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef88e9db7c08
Backed out changeset b73d74bcafda for multiple bc failures. CLOSED TREE

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Third time's the charm? Should be green now :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f30f256f9b67
Do not track TRANSITION_EMBED visits for link-coloring purposes. r=mak

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da8d5a96f3c8
Fix TestUtils.waitForCondition to not use setInterval. r=mak

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/f30f256f9b67
https://hg.mozilla.org/mozilla-central/rev/da8d5a96f3c8