1189911 - [e10s] Enable browser_plugin_infolink test

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Trevor Rowbotham [:rowbot]]

With Bug 1160166 fixed and a workaround implemented in Bug 1184276, it should be safe to enable this test on e10s.  It is passing locally on my Win 7 machine.

Comment 1

[Trevor Rowbotham [:rowbot]]

Attachment: bug1189911_reenable_browser_plugin_infolink_test.diff

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 8641835 [details] [diff] [review]
bug1189911_reenable_browser_plugin_infolink_test.diff

Thanks!

Comment 3

[Trevor Rowbotham [:rowbot]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e17945324e3

Comment 4

[Trevor Rowbotham [:rowbot]]

Thanks for the quick review!  Try looks good.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a78aef5ef0d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for Win8 e10s browser_plugin_reloading.js (and more) permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=12418306&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/43b640c7daaf

Comment 7

[Trevor Rowbotham [:rowbot]]

Figures, I don't include win64 in the try push and that blows up in my face.  I'm looking into it.

Comment 8

[Trevor Rowbotham [:rowbot]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=497b2f446aaa

Comment 9

[Trevor Rowbotham [:rowbot]]

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

Comment 10

[Trevor Rowbotham [:rowbot]]

Jim, the problem here is the |promiseForCondition| call[1] (see the try push in comment 9). Commenting out that line seems to fix things here, but what I don't understand is why. I tried changing the test to use |waitForMs| instead of |promiseForCondition|, but all the subsequent tests still failed. For whatever strange reason, I was able to reproduce this on my machine yesterday, but can not reproduce it today... If you have some spare time in the next few days, I would appreciate it if you could take a look.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_plugin_infolink.js?from=browser_plugin_infolink.js#70

Comment 11

[Jim Mathies [:jimm]]

(In reply to Trevor Rowbotham from comment #10)
> Jim, the problem here is the |promiseForCondition| call[1] (see the try push
> in comment 9). Commenting out that line seems to fix things here, but what I
> don't understand is why. I tried changing the test to use |waitForMs|
> instead of |promiseForCondition|, but all the subsequent tests still failed.
> For whatever strange reason, I was able to reproduce this on my machine
> yesterday, but can not reproduce it today... If you have some spare time in
> the next few days, I would appreciate it if you could take a look.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> plugins/browser_plugin_infolink.js?from=browser_plugin_infolink.js#70

I remember this test condition was unreliable when we ported these. A way to debug this would be to put info statements in |condition| to see what doesn't show up. Also you can use code like this in there as well and push to try:

let snap = SpecialPowers.snapshotWindow(window, false);
info(snap.toDataURL());

This snaps a bunch of screenshots and dumps them to the logs so you can see if that addons page actually loads properly.

Comment 12

[Trevor Rowbotham [:rowbot]]

Turns out the test failures are caused by the workaround implemented in Bug 1184276, however, without the workaround in Bug 1184276 we just crash.  Adding a dependency on the followup bug.

Comment 13

[Trevor Rowbotham [:rowbot]]

Attachment: test_log

For reference, here is the log from the test run with the patch in this bug applied and backing out the patch from Bug 1184276 using:

mach mochitest browser/base/content/test/plugins/ --e10s --start-at browser_plugin_infolink.js

Relevant bits:

IPDL protocol error: Handler for GetBlocklistState returned error code
###!!! [Parent][DispatchSyncMessage] Error: (msgtype=0x3A004E,name=PContent::Msg_GetBlocklistState) Processing error: message was deserialized, but the handler returned false (indicating failure)
###!!! [Parent][MessageChannel] Error: (msgtype=0x200081,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
###!!! [Parent][MessageChannel] Error: (msgtype=0x200081,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
43 INFO Console message: [JavaScript Error: "remote browser crashed while on http://127.0.0.1:8888/browser/browser/base/content/test/plugins/plugin_test.html
" {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
44 INFO Console message: [JavaScript Error: "remote browser crashed while on about:blank
" {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
wheel input enabled, wheelWarning, undefined
touch input enabled, touchWarning, undefined
45 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_plugin_reloading.js | Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:100 - Error: Timed out while waiting for a 'load'' event
Stack trace:
    promiseTabLoadEvent/</timeout<@chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:100:14
    setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:632:12
    promiseTabLoadEvent/<@chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:98:19
    promiseTabLoadEvent@chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:81:1
    @chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_plugin_reloading.js:47:9
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
    Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
    this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
    this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
    Tester_execTest@chrome://mochikit/content/browser-test.js:745:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:668:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:745:59

Comment 14

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

Attachment: cleaned up test, narrowed failure down

There were a lot of moving parts in this test that didn't need to be, so I was able to reduce this test a by a good amount.

The test still causes failures on e10s Win8+ though, and it only happens when we wait for the addons://list/plugin to load. If we exit the test before the view has loaded we don't get the subsequent failures anymore. When the addon view loads, it lists the addons, and indirectly calls the `blocklistState` getter on each addon. Note that I've removed setting the blocklist in this test and I still see the errors.

It seems to me that the blocklist from previous tests have lasting effects, but those effects aren't persistent until the add-ons manager is opened. Are we sure that we are properly removing the blocklist when tests complete? head.js includes a `resetBlocklist` function that only clears the extensions.blocklist.url pref.

nsBlocklistService.js has a pref observer for the extensions.blocklist. root, which supposedly could update the blocklist, but I don't see any code within Blocklist.prototype.observe to do so when that specific pref is changed. Adding a simple case statement for extensions.blocklist.url and calling `this._blocklistUpdated(null,null)` caused other test failures.

It is unclear to me what makes this Windows8+ specific and having it not affect other platforms or other versions of Windows.

Georg, does anything here stand out to you?

Comment 15

[Georg Fritzsche [:gfritzsche]]

I can't see anything obvious really, but then again i haven't really worked on those things for a while.
Is this still the same error or a different one?

Comment 16

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

Attachment: MozReview Request: Bug 1189911 - [e10s] Enable browser_plugin_infolink test. r?gfritzsche

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

Comment 17

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

I worked around the issue by not faking the BrowserOpenAddonsMgr function so the Add-ons Manager isn't actually opened, but we can test that the correct view was requested.

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

Comment 18

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

Comment on attachment 8726390 [details]
MozReview Request: Bug 1189911 - [e10s] Enable browser_plugin_infolink test. r?gfritzsche

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

Comment 19

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

Updated try with head.js changes reverted,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c261b69549

Comment 20

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

Comment on attachment 8726390 [details]
MozReview Request: Bug 1189911 - [e10s] Enable browser_plugin_infolink test. r?gfritzsche

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

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8726390 [details]
MozReview Request: Bug 1189911 - [e10s] Enable browser_plugin_infolink test. r?gfritzsche

https://reviewboard.mozilla.org/r/37963/#review34663

If we need to go with a work-around now, we need to file a follow-up on removing it later.
Or is opening the functionality of opening the addon manager sufficiently covered elsewhere?

::: browser/base/content/test/plugins/browser_plugin_infolink.js:18
(Diff revision 3)
> -  let promise = waitForEvent(gBrowser.tabContainer, "TabOpen", null, true);
> +    window.BrowserOpenAddonsMgr = function BrowserOpenAddonsMgrShim(view) {

Nit: Does that function need a name?

::: browser/base/content/test/plugins/browser_plugin_infolink.js:39
(Diff revision 3)
> +  is(requestedView, "addons://list/plugin", "The Add-ons Manager should open to the plugin view");

Nit: "should open the"

::: browser/base/content/test/plugins/browser_plugin_infolink.js:44
(Diff revision 3)
> +add_task(function* cleanup() {

In contrast to `registerCleanupFunction()`, this will not run if there were failures before etc., which could make later tests fail in strange ways.
Is that acceptable?

::: browser/base/content/test/plugins/browser_plugin_infolink.js:47
(Diff revision 3)
> +  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_ENABLED, "Test Plug-in");

This is potentially surprising - `setTestPluginEnabledState()` registers a cleanup function that resets this to the previous value, so this seems redundant.

Comment 22

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

https://reviewboard.mozilla.org/r/37963/#review34663

Opening the addon manager is covered by at least three tests, as shown by this search http://mxr.mozilla.org/mozilla-central/search?string=browseropenaddonsmgr&find=test&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

> Nit: Does that function need a name?

No, I'll remove it.

> In contrast to `registerCleanupFunction()`, this will not run if there were failures before etc., which could make later tests fail in strange ways.
> Is that acceptable?

Good point, I'll use registerCleanupFunction.

> This is potentially surprising - `setTestPluginEnabledState()` registers a cleanup function that resets this to the previous value, so this seems redundant.

The implementation at http://hg.mozilla.org/mozilla-central/annotate/b6acf4d4fc20/widget/tests/utils.js#l20 uses a registerCleanupFunction, but this code uses the implementation defined at http://hg.mozilla.org/mozilla-central/annotate/b6acf4d4fc20/browser/base/content/test/plugins/head.js#l160 which doesn't use registerCleanupFunction.

Comment 23

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

Comment on attachment 8726390 [details]
MozReview Request: Bug 1189911 - [e10s] Enable browser_plugin_infolink test. r?gfritzsche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37963/diff/3-4/

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8726390 [details]
MozReview Request: Bug 1189911 - [e10s] Enable browser_plugin_infolink test. r?gfritzsche

https://reviewboard.mozilla.org/r/37963/#review35013

Thanks.

Comment 25

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

Attachment: Patch for check-in

Comment 26

[Pulsebot]

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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c575eb64d8fa

Comment 28

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

Comment on attachment 8727386 [details] [diff] [review]
Patch for check-in

Approval Request Comment
[Feature/regressing bug #]: e10s test fix
[User impact if declined]: n/a
[Describe test coverage new/current, TreeHerder]: automated test
[Risks and why]: no risk, doesn't touch production code
[String/UUID change made/needed]: none

Comment 29

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

Comment on attachment 8727386 [details] [diff] [review]
Patch for check-in

Test-only fix.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/699175c7b637