Closed Bug 1595285 Opened 3 months ago Closed 3 months ago

Do not track TRANSITION_EMBED visits for link-coloring purposes.

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: emilio, Assigned: emilio)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

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

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

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9cba679cc50d
Minor cleanup of the visited query code. r=mak
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e19c5df398de
Do not track TRANSITION_EMBED visits for link-coloring purposes. r=mak
Keywords: leave-open

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

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.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffc59a6dfbd5
Fix TestUtils.waitForCondition to not use setInterval. r=mak
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b73d74bcafda
Do not track TRANSITION_EMBED visits for link-coloring purposes. r=mak
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef88e9db7c08
Backed out changeset b73d74bcafda for multiple bc failures. CLOSED TREE
Depends on: 1596165

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

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f30f256f9b67
Do not track TRANSITION_EMBED visits for link-coloring purposes. r=mak
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da8d5a96f3c8
Fix TestUtils.waitForCondition to not use setInterval. r=mak
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.