Closed Bug 1246667 Opened 8 years ago Closed 8 years ago

Enable browser_clearplugindata.js in e10s

Categories

(Toolkit :: Data Sanitization, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fce-active-legacy])

Attachments

(2 files)

      No description provided.
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.
Attachment #8716983 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/e4719dd204d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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?
Flags: needinfo?(enndeakin)
I think it can just be enabled.
Flags: needinfo?(enndeakin)
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(enndeakin)
Resolution: FIXED → ---
Attached patch Possible fixSplinter Review
Flags: needinfo?(enndeakin)
Whiteboard: [fct-active]
Whiteboard: [fct-active] → [fce-active]
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
Attachment #8753437 - Flags: review?(mak77)
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.
Attachment #8753437 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb4a840f81ed3e560d6395f61ad15f3a7c01e7e
Bug 1246667, properly enable browser_clearplugindata.js in e10s, r=mak
https://hg.mozilla.org/mozilla-central/rev/cbb4a840f81e
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [fce-active] → [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.