1246667 - Enable browser_clearplugindata.js in e10s

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect)

Description

[Neil Deakin]

Attachment: browser_clearplugindata

Comment 1

[Neil Deakin]

Comment on attachment 8716983 [details] [diff] [review]
browser_clearplugindata

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

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8716983 [details] [diff] [review]
browser_clearplugindata

Review of attachment 8716983 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/forgetaboutsite/test/browser/browser_clearplugindata.js
@@ +36,5 @@
>      plugin.enabledState = oldEnabledState;
>    });
>  }
>  
> +add_task(function* () {

please name this "setup" so it's clearer should come first

@@ +63,3 @@
>  
> +  // Clear data for "foo.com" and its subdomains.
> +  yield ForgetAboutSite.removeDataFromDomain("foo.com");

Argh, I just looked at removeDataFromDomain and it's a nightmare, filed Bug 1247201.

Comment 3

[Neil Deakin]

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

Comment 4

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4719dd204d128686f9063bcd8a9a44e82d83e65
Bug 1246667, enable browser_clearplugindata.js in e10s, r=mak

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Justin Dolske [:Dolske]]

Neil - seems you fixed the test, but didn't enable it for E10S? The browser.ini still shows this as being disabled... http://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/test/browser/browser.ini#4

Is there further work needed beyond just enabling it?

Comment 7

[Neil Deakin]

I think it can just be enabled.

Comment 8

[Justin Dolske [:Dolske]]

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

Comment 9

[Justin Dolske [:Dolske]]

Neil: I see a lot of intermittent failures of this test in my try push to enable it on E10S (comment 8). Linux seems worst off (4/4 failures on Linux x64 opt), and even 1 instance on Windows 8 x64 opt. I just did a few more retriggers, as it's not the same test chunk on all platforms.

Can you investigate this?

Comment 10

[Neil Deakin]

Attachment: Possible fix

Comment 11

[Neil Deakin]

Comment on attachment 8753437 [details] [diff] [review]
Possible fix

Review of attachment 8753437 [details] [diff] [review]:
-----------------------------------------------------------------

This patch just makes the initialization happen outside of the load event

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8753437 [details] [diff] [review]
Possible fix

Review of attachment 8753437 [details] [diff] [review]:
-----------------------------------------------------------------

This likely wants a Try run with some retriggers before landing.

::: toolkit/forgetaboutsite/test/browser/browser_clearplugindata.js
@@ +55,5 @@
>    setTestPluginEnabledState(Ci.nsIPluginTag.STATE_ENABLED, pluginTag);
>  });
>  
>  add_task(function* () {
>    // Load page to set data for the plugin.

I'd probably move this comment after the openNewForegroundTab call and change it to

// Set data for the plugin after the page load.

Comment 13

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb4a840f81ed3e560d6395f61ad15f3a7c01e7e
Bug 1246667, properly enable browser_clearplugindata.js in e10s, r=mak

Comment 14

[Carsten Book [:Tomcat]]

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