[e10s] Fix browser_CTP_plugins.js to run in e10s

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(e10s+, firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
This test passes locally for me, and appears to have been fixed by bug 899347. Pushing to tryserver now to confirm...
Created attachment 8727604 [details]
MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche

Review commit: https://reviewboard.mozilla.org/r/38541/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38541/
Attachment #8727604 - Flags: review?(gfritzsche)
Comment on attachment 8727604 [details]
MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche

https://reviewboard.mozilla.org/r/38541/#review35301

Nice, "fixed already" is the best kind of fixed.
Attachment #8727604 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed

Updated

3 years ago
tracking-e10s: --- → +
The try push has multiple perma-failures.

Linux opt M-e10s(bc4)
 15:21:32 INFO - 388 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | part8: state menu should have 'Never Activate' selected - Got [object XULElement], expected [object XULElement]
2660 15:21:35 INFO - 390 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | part9: waited too long for plugin to activate -
2665 15:21:35 INFO - 392 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | part10: plugin should be activated - 

Same failures on Linux debug M-e10s(bc1) and OSX 10.10 debug M-e10s(bc1). Passes on Windows opt/debug and OSX opt.
Keywords: checkin-needed
Comment on attachment 8727604 [details]
MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38541/diff/1-2/
Comment on attachment 8727604 [details]
MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche

>https://reviewboard.mozilla.org/r/38541/diff/#index_header
Attachment #8727604 - Flags: review+ → review?(gfritzsche)
Attachment #8727604 - Flags: review?(gfritzsche)
Comment on attachment 8727604 [details]
MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche

https://reviewboard.mozilla.org/r/38541/#review38129

This overhaul makes things much more readable, thanks.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:433
(Diff revision 2)
>    windowClosed(win)  {
>      let domWinClosedPromise = BrowserTestUtils.domWindowClosed(win);
>      let promises = [domWinClosedPromise];
>      let winType = win.document.documentElement.getAttribute("windowtype");
>  
> +    dump(`windowClosed for ${winType}`);

Is this intentional or a left-over?

::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:4
(Diff revision 2)
> -var gManagerWindow;
> -var gTestPluginId;
> -var gPluginBrowser;
>  
>  function updateBlocklist(aCallback) {
> +  info("entered updateBlocklist");

Are all the `info()` calls in this file left-overs?

::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:160
(Diff revision 2)
> +
> +  yield BrowserTestUtils.removeTab(pluginTab);
>  
>    // causes appDisabled to be set
> +  managerWindow = yield new Promise(resolve => {
>      setAndUpdateBlocklist(gHttpTestRoot + "blockPluginHard.xml",

is `asyncSetAndUpdateBlocklistasyncSetAndUpdateBlocklist()` potentially needed here?

::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:182
(Diff revision 2)
> -function part13() {
> +  menu = managerWindow.document.getElementById("detail-state-menulist");
> -  let menu = gManagerWindow.document.getElementById("detail-state-menulist");
>    is(menu.disabled, true, "part13: detail state menu should be disabled");
>  
> -  setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", function() {
> -    run_next_test();
> +  yield new Promise(resolve => {
> +    setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", resolve);

Could we move that to a cleanup function to avoid affecting later tests in case of failures?

::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:186
(Diff revision 2)
> -    run_next_test();
> +    setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", resolve);
>    });
> -}
>  
> -function end_test() {
> -  Services.prefs.clearUserPref("plugins.click_to_play");
> +  info("part14: about to close the window");
> +  //yield BrowserTestUtils.closeWindow(managerWindow);

Left-over?
Attachment #8727604 - Flags: review?(gfritzsche)
Comment on attachment 8727604 [details]
MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38541/diff/2-3/
https://reviewboard.mozilla.org/r/38541/#review38129

> Is this intentional or a left-over?

Leftover debugging, as were the info's mentioned below. They've been removed.

> is `asyncSetAndUpdateBlocklistasyncSetAndUpdateBlocklist()` potentially needed here?

It's not in this directory's head.js, and I don't want to duplicate it there. We could maybe move it to BrowserTestUtils, though I'm also not sure if that is the right place for such as a specific method. I'd prefer to leave this for another bug, but I'm curious as to how you think we should handle the duplication.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> > is `asyncSetAndUpdateBlocklistasyncSetAndUpdateBlocklist()` potentially needed here?
> 
> It's not in this directory's head.js, and I don't want to duplicate it
> there. We could maybe move it to BrowserTestUtils, though I'm also not sure
> if that is the right place for such as a specific method. I'd prefer to
> leave this for another bug, but I'm curious as to how you think we should
> handle the duplication.

BrowserTestUtils sounds like a reasonable place to me; if that one becomes too big we could still break it into more specific modules later (PluginTestUtils, etc.)?
I'm happy to leave it for another bug, this could potentially cause intermittent failures later.
Comment on attachment 8727604 [details]
MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche

https://reviewboard.mozilla.org/r/38541/#review38727
Attachment #8727604 - Flags: review?(gfritzsche) → review+

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cdc495ec3550
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.