Closed Bug 1254296 Opened 4 years ago Closed 4 years ago

[e10s] Fix browser_CTP_plugins.js to run in e10s

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
This test passes locally for me, and appears to have been fixed by bug 899347. Pushing to tryserver now to confirm...
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+
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/
Attachment #8727604 - Flags: review?(jaws)
Attachment #8727604 - Flags: review?(jaws)
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+
https://hg.mozilla.org/mozilla-central/rev/cdc495ec3550
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.