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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks 1 bug)

Trunk
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

No description provided.
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 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?
Attachment #8727484 - Flags: review?(gijskruitbosch+bugs) → review+
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.
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?
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.
https://hg.mozilla.org/mozilla-central/rev/8d9db8c94c99
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
Attachment #8727484 - Flags: approval-mozilla-aurora?
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).
Flags: needinfo?(jaws)
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.
Attachment #8727484 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ok, will do. Thanks
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.