1547517 - Enable fxmonitor test on debug

Status: NEW

(Firefox :: Firefox Monitor, task, P3)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

From bug 1528978 comment 13:

The tests seem to be leaking windows and I can't figure out why. In the interest of landing some test coverage quickly, I vote we disable the test on debug for now and file a follow-up bug to re-enable it. I'm going to go ahead and make a patch and bug to this effect.

This is the follow-up bug mentioned above.

Comment 1

[Johann Hofmann [:johannh]]

Hm, are you 100% sure your test is leaking, not your code? What's the minimal example test you can reconstruct that leaks?

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Johann Hofmann [:johannh] from comment #1)

Hm, are you 100% sure your test is leaking, not your code? What's the minimal example test you can reconstruct that leaks?

Didn't get a chance to do this on Friday. I filed this proactively because at the moment, the fxmonitor code doesn't have test coverage, so I'd like to land something asap and fix the issues reactively.

From bug 1528978 comment 15:

For posterity, here's a summary of my investigation into the leaking windows:

• The failure is reproducible locally.
• The log suggests a URL "_generated_background_page.html" - this is the "page" in whose scope the addon's background.js script runs.
• FWIW, I couldn't find any instances of references to windows/documents beyond the appropriate lifetime.
• Putting the contents of background.js in a closure didn't help.
• Since the add-on is enabled, there must be something going on that's specifically relevant to this test - i.e. it's because of the doorhanger, or the test is keeping a reference it shouldn't, etc. Need to investigate further.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

New findings from bug 1528978 comment 16:

The warned hosts pref migration test reloads the addon. This is the culprit. The following code is sufficient to cause this leak:

add_task(async function foobar() {
  let addon = await AddonManager.getAddonByID("[e-mail]");
  await addon.reload();
});

In light of this, I think it's ok to skip this test on debug for now, and investigate this issue further. I'm curious if this leak is reproducible with a minimal add-on (manifest + empty background script).

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

This is reproducible with a minimal webextension (manifest + empty background script). I filed bug 1548006 to track.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Seems like we're going to have to work around this limitation by either putting the warned hosts migration test in xpcshell, or expose the migration code for the test to manually trigger instead of reloading the add-on, or maybe both.

Comment 6

[Noman farhan]

Sure, are you absolutely certain that it's your test leaking and not your code? Could you provide the simplest test case that demonstrates the leak?
https://www-ehallpass.com