1254148 - [e10s] Fix leak in browser_privacypane_8.js that happens in e10s runs

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1254148 - Fix browser_privacypane_8.js to run in e10s. r?gijs

Review commit: https://reviewboard.mozilla.org/r/38487/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38487/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8727484 [details]
MozReview Request: Bug 1254148 - Fix browser_privacypane_8.js to run in e10s. r?gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38487/diff/1-2/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8727484 [details]
MozReview Request: Bug 1254148 - Fix browser_privacypane_8.js to run in e10s. r?gijs

https://reviewboard.mozilla.org/r/38487/#review35139

::: browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js
(Diff revision 2)
> -    browser.addEventListener("Initialized", function(aEvent) {
> -      browser.removeEventListener("Initialized", arguments.callee, true);
> -      is(browser.contentWindow.location.href, "about:preferences", "Checking if the preferences tab was opened");

Do we no longer need this event and/or did you trypush + retrigger this to see this won't go intermittently-orange racing with that event?

::: browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js:324
(Diff revision 2)
> -  dump("subset: " + Array.from(subset, x => x.name).join(",") + "\n");
> +  SpecialPowers.pushPrefEnv({"set": [["browser.preferences.instantApply", true]]});

Is this necessary still?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/38487/#review35139

> Do we no longer need this event and/or did you trypush + retrigger this to see this won't go intermittently-orange racing with that event?

The "Initialized" event is fired towards the end of the DOMContentLoaded event handler. I'm using BrowserTestUtils.openNewForegroundTab with waitForLoad and waitForStop to make sure that the page has fully loaded and stopped loading external resources before continuing. Waiting for "Initialized" here isn't necessary, and is in fact a bit harder when using the BrowserTestUtils functions because they resolve after DOMContentLoaded has fired. I also retriggered this (bc4) nine times on Linux Opt and didn't see any intermittents.

> Is this necessary still?

Are you referring to the dump or the instantApply pref? firefox.js has instantApply set to false on Windows by default, because otherwise we would break add-ons. Many tests set instantApply to true for their test, and this could probably be revisited because the in-content preferences set document.instantApply=true so this may not be necessary. I don't want to visit that in this bug, as we should probably tackle all of that in one concise bug. The dump here is useful for debugging and shouldn't cause any harm sticking around.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8d9db8c94c99

Comment 8

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/38487/#review35139

> Are you referring to the dump or the instantApply pref? firefox.js has instantApply set to false on Windows by default, because otherwise we would break add-ons. Many tests set instantApply to true for their test, and this could probably be revisited because the in-content preferences set document.instantApply=true so this may not be necessary. I don't want to visit that in this bug, as we should probably tackle all of that in one concise bug. The dump here is useful for debugging and shouldn't cause any harm sticking around.

The instantApply setting was what I meant. Can you file a followup for auditing our usage of instantApply?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/38487/#review35139

> The instantApply setting was what I meant. Can you file a followup for auditing our usage of instantApply?

Filed bug 1254518.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d9db8c94c99

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8727484 [details]
MozReview Request: Bug 1254148 - Fix browser_privacypane_8.js to run in e10s. r?gijs

Approval Request Comment
[Feature/regressing bug #]: e10s test
[User impact if declined]: none, will add expanded e10s test coverage
[Describe test coverage new/current, TreeHerder]: it is an automated test
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jared, test-only uplifts don't need to be nominated for approval. Those are auto-approved for landing. I can a+ the ones in aurora queue so far. For future, please go ahead and land them directly (I am assuming you have rights to do that).

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8727484 [details]
MozReview Request: Bug 1254148 - Fix browser_privacypane_8.js to run in e10s. r?gijs

test-only changes are auto-approved.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Ok, will do. Thanks

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66c7e14adba6