Closed Bug 1129040 Opened 5 years ago Closed 4 years ago

[e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm

Categories

(Toolkit :: Add-ons Manager, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: Gijs, Assigned: jimm)

References

(Blocks 3 open bugs)

Details

Attachments

(5 files, 23 obsolete files)

8.98 KB, patch
Details | Diff | Splinter Review
9.61 KB, patch
mossop
: review+
Details | Diff | Splinter Review
3.92 KB, patch
mossop
: review+
Details | Diff | Splinter Review
2.61 KB, patch
billm
: review+
Details | Diff | Splinter Review
279.19 KB, patch
Details | Diff | Splinter Review
Clean profile on current fx-team tip, shows:

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] FileUtils.jsm:63:0

as soon as you show a web-based page (the mozilla update/welcome page that shows when creating a new profile will trigger this, but otherwise http://www.apple.com/it/itunes/download/ will do it)

which is:

    var dir = gDirService.get(key, Ci.nsIFile);

in FileUtils.getDir.

On the second startup, you can trigger it by restoring the session from about:home (which itself doesn't trigger this), and catch it in the browser content toolbox if you open that before restoring (you'll need to Components.utils.import("resource://gre/modules/FileUtils.jsm") from the console in order to set a breakpoint there. Then you'll see that it's triggered by the plugin blocklist service.

Georg, can you investigate?
Flags: needinfo?(gfritzsche)
I don't really have time to investigate this at the moment.

Do you know what the impact of this is? Is this breaking plugin blocks?
Do you have a stack handy that shows what components are involved?
Flags: needinfo?(gfritzsche) → needinfo?(gijskruitbosch+bugs)
Is this with e10s?

AFAIK the blocklist service, if it is used at all in the content process, should be getting its data from the chrome process. Perhaps that's the bug here.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Is this with e10s?

Ugh @ me for leaving out key information. Yes, this is with e10s (although the content toolbox process maybe gave it away, I should have been explicit)

I don't have a stack handy because the js debugger doesn't make it easily copy-able. :-(

I can look into it again tomorrow, I've moved on now and am rushing to get some more urgent things done today.

I don't know if/what it breaks because I actually don't know what the plugin blocklist service *is*, I just know that that's what I saw on the stack so that's where I moved this.
So this might require you to have a plugin globally installed? I have Adobe Acrobat Reader on my machine, and so it seems that:

nsBlockListService.js'

  /* See nsIBlocklistService */
  getPluginBlocklistState: function Blocklist_getPluginBlocklistState(plugin,
                           appVersion, toolkitVersion) {
    if (!this._isBlocklistLoaded())
      this._loadBlocklist();
    return this._getPluginBlocklistState(plugin, this._pluginEntries,
                                         appVersion, toolkitVersion);
  },

gets called with plugin having .description "Adobe PDF Plug-In For Firefox and Netscape 11.0.10" (filename nppdf32.dll)

which calls _loadBlockList, which wants to get the profile dir, which calls FileUtils.getFile, which calls FileUtils.getDir, which throws.

So I guess that comment #2 is right and this needs to be reimplemented so it gets the blocklist data from the chrome process? It looks like this code wants an answer synchronously, though, so that might be interesting...
Flags: needinfo?(gijskruitbosch+bugs)
Summary: Plugin block list triggers exception in FileUtils.jsm getDIr, for nsIProperties.get passing "ProfD" and nsIFile → [e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm getDIr, for the directory service's get method passing "ProfD" and nsIFile
Component: Plug-ins → Add-ons Manager
Product: Core → Toolkit
Thanks for checking Gijs :)
Assignee: nobody → jmathies
Attached patch patch (obsolete) — Splinter Review
cleaned up
Attachment #8576809 - Attachment is obsolete: true
No longer blocks: 1130165
Duplicate of this bug: 1130165
Summary: [e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm getDIr, for the directory service's get method passing "ProfD" and nsIFile → [e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm
https://treeherder.mozilla.org/#/jobs?repo=try&revision=090ff8636230

Hey Kamil, would you mind trying to reproduce bug 1130165 using this try build when you have a chance?
Flags: needinfo?(kjozwiak)
(In reply to Jim Mathies [:jimm] from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=090ff8636230

Mostly green, the leaks are probably caused by a lack of unregistration of the factory on shutdown.
Attached patch patch (obsolete) — Splinter Review
Dave, I think this is in your neck of the woods. Feel free to reassign.
Attachment #8576834 - Attachment is obsolete: true
Attachment #8577310 - Flags: review?(dtownsend)
Comment on attachment 8577310 [details] [diff] [review]
patch

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

Some mxr searches tell me that the only method we actually need to care about child processes being able to call is getPluginBlocklistState so I think that is all we should implement in the child. The rest either don't implement or just have them throw an exception.

Defining this in browser-child is the wrong place, as a frame script that will be run many times in the same process and so attempt to recreate a new blocklist service each time. Instead we should just define a version of the blocklist service that only loads in the child process, nsBlocklistServiceChild.js, and have that send messages to nsBlocklistService.js.

We probably don't need to send the plugin tag across as a CPOW, all the service cares about is a few string properties on it so you could just copy those to a plain JS object.

All the browser-chrome plugin tests are disabled in e10s right now. Some of them do blocklist checking so it would be interesting to see if any can be enabled: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser.ini
Attachment #8577310 - Flags: review?(dtownsend) → review-
(In reply to Dave Townsend [:mossop] from comment #13)
> Comment on attachment 8577310 [details] [diff] [review]
> patch
> 
> Review of attachment 8577310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some mxr searches tell me that the only method we actually need to care
> about child processes being able to call is getPluginBlocklistState so I
> think that is all we should implement in the child. The rest either don't
> implement or just have them throw an exception.

Ok, no problem. I was wondering about this as well, I ended up going the other way and implementing these. Will remove that and just error out.


> Defining this in browser-child is the wrong place, as a frame script that
> will be run many times in the same process and so attempt to recreate a new
> blocklist service each time. Instead we should just define a version of the
> blocklist service that only loads in the child process,
> nsBlocklistServiceChild.js, and have that send messages to
> nsBlocklistService.js.

I think I had browser-child.js mixed up with broiwser-content.js, I was going for load once behavior. Will look into the separate module. 

> We probably don't need to send the plugin tag across as a CPOW, all the
> service cares about is a few string properties on it so you could just copy
> those to a plain JS object.

That's what's I'm doing with flattenObject.

> All the browser-chrome plugin tests are disabled in e10s right now. Some of
> them do blocklist checking so it would be interesting to see if any can be
> enabled:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> plugins/browser.ini

Thanks for the reminder.
Used the following m-c build for verification:

* http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-090ff8636230/
** http://download.macromedia.com/pub/flashplayer/installers/archive/fp_16.0.0.296_archive.zip
** http://download.macromedia.com/pub/flashplayer/installers/archive/fp_11.2.202.440_archive.zip

Went through the following test cases while e10s was enabled/disabled:

- visited ign.com and ensured that the vulnerable plugin is being blocked
- ensured that flash appears as vulnerable under about:addons
- ensured that selecting "Activate Flash" works correctly and plays the video
- selected "Allow Now" and ensured that when fx is restarted, the video is still blocked
- selected "Allow and Remember" and ensured that the site isn't blocked when fx is restarted

OS's:

- Win 8.1 x64 - PASSED
- Ubuntu 14.04.1 x64 - PASSED

Looks like it's working Jim, once this lands into m-c I'll do more thorough testing with different versions of vulnerable flash versions. Let me know if there's anything else that you need!
Flags: needinfo?(kjozwiak)
(In reply to Kamil Jozwiak [:kjozwiak] from comment #15)
> OS's:
> 
> - Win 8.1 x64 - PASSED
> - Ubuntu 14.04.1 x64 - PASSED
> 
> Looks like it's working Jim, once this lands into m-c I'll do more thorough
> testing with different versions of vulnerable flash versions. Let me know if
> there's anything else that you need!

Great, thanks!
Attachment #8577310 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 8578938 [details] [diff] [review]
patch


This is the core blocklist patch. Per discussion in triage, I will file a follow up on updating tests in:

browser/base/content/tests/plugins/*
Attachment #8578938 - Flags: review?(dtownsend)
Comment on attachment 8578938 [details] [diff] [review]
patch

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

A few ways this can be simplified otherwise this looks great.

::: toolkit/mozapps/extensions/extensions.manifest
@@ +3,5 @@
> +category profile-after-change nsBlocklistService @mozilla.org/extensions/blocklist;1  process=main
> +
> +component {e0a106ed-6ad4-47a4-b6af-2f1c8aa4712d} nsBlocklistServiceContent.js process=content
> +contract @mozilla.org/extensions/blocklist;1 {e0a106ed-6ad4-47a4-b6af-2f1c8aa4712d} process=content
> +category app-startup nsBlocklistService @mozilla.org/extensions/blocklist;1 process=content

I don't think we need this line. If something tries to use the blocklist service it will load and work but otherwise there is no need to load this on startup.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +298,5 @@
>    gPref.addObserver(PREF_EM_LOGGING_ENABLED, this, false);
>    this.wrappedJSObject = this;
> +  // requests from child processes come in here, see receiveMessage.
> +  this._ppmm = Cc['@mozilla.org/parentprocessmessagemanager;1'].
> +    getService(Ci.nsIMessageBroadcaster);

Just use Services.ppmm instead of keeping our own reference.

@@ +371,5 @@
> +      default:
> +        throw new Error("Unknown blocklist message received from content: " + aMsg.name);
> +    }
> +  },
> +    

Nit: whitespace at end of line

::: toolkit/mozapps/extensions/nsBlocklistServiceContent.js
@@ +30,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsIBlocklistService]),
> +
> +  init: function () {
> +    this._mm = Cc['@mozilla.org/childprocessmessagemanager;1'].
> +      getService(Ci.nsISyncMessageSender);

Rather than retaining this just use Services.cpmm.

@@ +34,5 @@
> +      getService(Ci.nsISyncMessageSender);
> +    Services.obs.addObserver(this, "xpcom-shutdown", false);
> +  },
> +
> +  uninit: function () {

We're not doing anything here so no reason to listen for xpcom-shutdown or have this be an nsIObserver. That probably makes the init method unnecessary too.
Attachment #8578938 - Flags: review?(dtownsend) → review+
right patch this time.
Attachment #8581598 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cee8f9d83b7e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Jim, looks like the flash blocklisting is still not working in e10s. I downloaded fp_16.0.0.296 which is blocklisted but flash video's are still being played automatically even though the plugin is set to "Ask to Activate". The original try build from comment # 10 works as expected and blocked the flash video in both e10s and none-e10s modes. However with the latest nightly, the flash video will play automatically start playing on e10s despite being set as "Ask to Activate". If you switch over to none-e10s, it behaves correctly.

STR:

* installed fp_16.0.0.296
* downloaded and installed the latest nightly (build linked below)
* opened nightly and visited ign.com and played a random video which played automatically
* disabled e10s through the Options and re-started the browser
* same video was blocked with the "Plugin is vulnerable" error message

Builds Used:

Working -> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-090ff8636230/
Not Working -> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-03-26-03-02-12-mozilla-central/

Win 8.1 x64 -> Reproduced using fp_16.0.0.296
Ubuntu 14.04.1 -> Reproduced using 11.2.202.440
Flags: needinfo?(jmathies)
Status: RESOLVED → REOPENED
Flags: needinfo?(jmathies)
Resolution: FIXED → ---
(In reply to Kamil Jozwiak [:kjozwiak] from comment #24)
> Jim, looks like the flash blocklisting is still not working in e10s.

Thanks for checking back Kamil!
Depends on: 1148776
Attachment #8581601 - Attachment description: patch r=Mossop → patch r=Mossop [checked in]
Attached patch fix for blocklist updates v.1 (obsolete) — Splinter Review
We cache blocklist values in plugin tags, unless we receive a blocklist-updated observer.. in which case the next time we need the blocklist value, we ask the service. This propagates that observer over to the content process so its tags get invalidated as well.
Attachment #8585077 - Flags: review?(dtownsend)
If the blocklist forwarding service in the content process gets highjacked or breaks, we need to check directly with chrome at least once (I'm doing it twice) before instantiating a plugin instance.

Basically we shouldn't trust that the nsBlocklistService in the content process is right.
Attached patch shim updates (obsolete) — Splinter Review
tighten this up a bit. We want most of this interface to fail so that its use is noticed.
Attached patch tests (obsolete) — Splinter Review
Still working on getting all these tests functional. Currently dealing with some linux failures on try which do not reproduce locally. Also I'm getting a mochitest-plain 3 failure which seems to be caused by something in these patches, but I'm not sure what it is. The tests can't reach the plugin native apis. I ran into this in the tests I actually had to fix, the key was to get rid of xray vision on the object. I'm not sure how my patches here would cause a failure of that in tests I haven't touched. :) Need to keep digging. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc51523e4dc5
No longer blocks: 1145208
Duplicate of this bug: 1145208
Comment on attachment 8585077 [details] [diff] [review]
fix for blocklist updates v.1

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

I think we have a few cases where the parent just wants to forward observer notifications to the child. I wonder if at some point we might want a more generic way to do that. Oh well, something for the future.
Attachment #8585077 - Flags: review?(dtownsend) → review+
Latest wip, hopefully with the odd plugin related test failures fixed -

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5252f2e9031e
(In reply to Jim Mathies [:jimm] from comment #33)
> Latest wip, hopefully with the odd plugin related test failures fixed -
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5252f2e9031e

Updated push with some changes in nsIPluginTag removed - 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c183218ef539

My original changes made it so that we default to blocked for plugins if there's anything wrong with querying the blocklist. Apparently while running tests we run into this, need to dig in a bit to understand why.
No longer blocks: 1148827
Same patch as last time except that I've added an async message sent back from content to chrome when the blocklist updates. The event isn't of much use to anything except tests, which need a way of confirming the blocklist is fully initialized and ready in both processes.
Attachment #8585077 - Attachment is obsolete: true
Attachment #8585078 - Attachment is obsolete: true
Attachment #8585079 - Attachment is obsolete: true
Attachment #8585081 - Attachment is obsolete: true
Attachment #8587580 - Flags: review?(dtownsend)
This adds a chrome side blocklist query method to PContent.
Attachment #8587583 - Flags: review?(wmccloskey)
Attached patch shim updates v.1Splinter Review
Updates to the blocklist shim that exists in the content process:

1) generally fail on unexpected calls
2) add a couple component interfaces. Not sure if this is needed but I added it while debugging my try run test failures. the blocklist service supports these, so seems correct that we implement this on the shim as well.
Attachment #8587584 - Flags: review?(dtownsend)
Attached patch tests part 1 (obsolete) — Splinter Review
This is the head.js utility changes. I tried to comment most everything I touched or added. Generally pretty confident I'm leaving this in better shape than it was in.
Attachment #8587591 - Flags: review?(mconley)
Attached patch tests part 2 (obsolete) — Splinter Review
Misc. tests that didn't fall into a test category.
Attachment #8587592 - Flags: review?(mconley)
Attached patch tests part 3 (obsolete) — Splinter Review
updated bugx tests.
Attachment #8587593 - Flags: review?(felipc)
Attachment #8587592 - Attachment description: test part 2 → tests part 2
Attached patch tests block 4 (obsolete) — Splinter Review
click to play tests.
Attachment #8587594 - Flags: review?(mconley)
Attached patch tests block 5 (obsolete) — Splinter Review
new tests I've added to test blocklisting.
Attachment #8587595 - Flags: review?(felipc)
Attached patch whitespace changes in html files (obsolete) — Splinter Review
Attachment #8587597 - Attachment is obsolete: true
Almost forgot the most important patch - actually checking the chrome service before instantiating plugins.
Attachment #8587603 - Flags: review?(wmccloskey)
Attachment #8587580 - Flags: review?(dtownsend) → review+
Comment on attachment 8587584 [details] [diff] [review]
shim updates v.1

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

r+ but if we can easily get rid of the nsITimerCallback interface then please do.

::: toolkit/mozapps/extensions/nsBlocklistServiceContent.js
@@ +28,5 @@
>    classID: Components.ID("{e0a106ed-6ad4-47a4-b6af-2f1c8aa4712d}"),
>  
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
> +                                         Ci.nsIBlocklistService,
> +                                         Ci.nsITimerCallback]),

I'd rather not add nsITimerCallback here if we can avoid it. What is calling it like that? If it's the app update service then making this line main process only should solve it: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/extensions.manifest#8. That would also indicate that the update service was running in the content process which might be a bug in itself.

@@ +89,4 @@
>    },
>  
>    getAddonBlocklistState: function (aAddon, aAppVersion, aToolkitVersion) {
> +    return Components.interfaces.nsIBlocklistService.STATE_BLOCKED;

Ci.nsIBlocklistService.STATE_BLOCKED;
Attachment #8587584 - Flags: review?(dtownsend) → review+
Improving these two patches a bit - moving some e10s related logic in with common calls in plugin host.
Attachment #8587583 - Attachment is obsolete: true
Attachment #8587603 - Attachment is obsolete: true
Attachment #8587583 - Flags: review?(wmccloskey)
Attachment #8587603 - Flags: review?(wmccloskey)
Attachment #8587923 - Flags: review?(wmccloskey)
a much simpler patch now.
Attachment #8587924 - Flags: review?(wmccloskey)
Attachment #8587924 - Attachment is patch: true
even simpler after some whitespace cleanup.
Attachment #8587924 - Attachment is obsolete: true
Attachment #8587924 - Flags: review?(wmccloskey)
Attachment #8587926 - Flags: review?(wmccloskey)
sorry for the spam, I just noticed a bug in that last rev.
Attachment #8587923 - Attachment is obsolete: true
Attachment #8587923 - Flags: review?(wmccloskey)
Attachment #8587927 - Flags: review?(wmccloskey)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4535bb99ebb7

This work apparently leaks the world on OSX 10.6, probably due to a test. Other than this, looking good.
Comment on attachment 8587591 [details] [diff] [review]
tests part 1

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

::: browser/base/content/test/plugins/browser.ini
@@ +63,5 @@
> +[browser_CTP_crashreporting.js]
> +skip-if = e10s || !crashreporter
> +[browser_CTP_drag_drop.js]
> +skip-if = e10s # misc. issues with tab drag out and e10s
> +

Nit - why the newlines here, at 76 and 85?

::: browser/base/content/test/plugins/head.js
@@ +28,5 @@
> + * @param aSynthEvents listen for synth events too
> + * @param aTimeoutMs the number of miliseconds to wait before giving up
> + * @returns a Promise that resolves to the received event, or to an Error
> + */
> +function promiseForEvent(aSubject, aEventName, aCapture, aSynthEvents, aTimeoutMs, aTarget) {

Does this offer anything that https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#l189 doesn't? If so, we might want to augment the code in BrowserTestUtils.jsm instead and get rid of this specialized function so that other tests can take advantage of it.

@@ +38,5 @@
> +    aSubject.removeEventListener(aEventName, listener, aEventName, aCapture);
> +    eventDeferred.reject( new Error(aEventName + " event timeout at " + stack) );
> +  }, timeoutMs);
> +
> +  var listener = function (aEvent) {

let, not var

@@ +47,5 @@
> +    clearTimeout(timerID);
> +    eventDeferred.resolve(aEvent);
> +  }
> +
> +  function cleanup(aEventOrError) {

I know we don't have "use strict"; on in this file, but we should probably define this like the listener:

let cleanup = function(aEventOrError) {
  //...
};

In the event that we turn strictness on.

@@ +75,5 @@
> +
> +  function done() {
> +    deferred.resolve(true);
> +    info("waitForMs finished waiting, waited for "
> +       + (Date.now() - startTime)

Can't we just print out aMs instead of calculating the difference? Or do we need to be really specific with the output here for some reason?

@@ +86,5 @@
> +// The blocklist shim running in the content process does not initialize at
> +// start up, so it's not active until we load content that needs to do a
> +// check. This helper bypasses the delay to get the svc up and running
> +// immediately. Note, call this after remote content has loaded.
> +function promiseInitContentBlocklistSvc()

We might want to pass in a browser here, since (eventually) we'll support multiple separate content processes.

@@ +90,5 @@
> +function promiseInitContentBlocklistSvc()
> +{
> +  return ContentTask.spawn(gTestBrowser, {}, function* () {
> +    try {
> +      let bls = Components.classes["@mozilla.org/extensions/blocklist;1"]

ContentTask's have access to Cc and Ci, so let's use those shortcuts.

@@ +193,3 @@
>  function setTestPluginEnabledState(newEnabledState, pluginName) {
> +  let name = pluginName || "Test Plug-in";
> +  var plugin = getTestPlugin(name);

let, not var

@@ +199,5 @@
> +// Get the 'enabledState' on the nsIPluginTag stored in the main or chrome
> +// process.
> +function getTestPluginEnabledState(pluginName) {
> +  let name = pluginName || "Test Plug-in";
> +  var plugin = getTestPlugin(name);

let, not var

@@ +207,5 @@
> +// Returns a promise for the nsIObjectLoadingContent 'pluginFallbackType'
> +// property.
> +function promiseForObjectFallbackType(aId, aBrowser) {
> +  let browser = aBrowser || gTestBrowser;
> +  return ContentTask.spawn(browser, {id: aId}, function* () {

Might as well just pass the ID instead of an object with the id as a property.

Then, have the function accept those arguments, instead of digging into arguments[0].

@@ +210,5 @@
> +  let browser = aBrowser || gTestBrowser;
> +  return ContentTask.spawn(browser, {id: aId}, function* () {
> +    let plugin = content.document.getElementById(arguments[0].id);
> +    if (!(plugin instanceof Ci.nsIObjectLoadingContent))
> +      return -1;

Should we be throwing/rejecting in this case?

@@ +216,5 @@
> +  });
> +}
> +
> +// Returns a promise for the nsIObjectLoadingContent 'activated' property
> +function promiseForObjectActivationState(aId, aBrowser) {

Instead of individual functions that resolve to these plugin properties, can we have a single function that returns the values for all interesting properties, and then have the caller pick and choose which ones they care about?

@@ +255,5 @@
> +// Return a promise and call the plugin's nsIObjectLoadingContent
> +// playPlugin() method.
> +function promisePlayObject(aId, aBrowser) {
> +  let browser = aBrowser || gTestBrowser;
> +  return ContentTask.spawn(browser, {id: aId}, function* () {

As mentioned elsewhere, just pass the ID as the second argument to spawn, and accept it as the first (and only parameter) in the function - no need to dig about in arguments[0].

@@ +267,5 @@
> +function promiseObjectValueResult(aId, aBrowser) {
> +  let browser = aBrowser || gTestBrowser;
> +  return ContentTask.spawn(browser, {id: aId}, function* () {
> +    let plugin = content.document.getElementById(arguments[0].id);
> +    dump("---"+Components.utils.waiveXrays(plugin).getObjectValue());

Nit - remove dump, unless we really need this.

@@ +275,5 @@
> +
> +// Return a promise and reload the target plugin in the page
> +function promiseReloadPlugin(aId, aBrowser) {
> +  let browser = aBrowser || gTestBrowser;
> +  return ContentTask.spawn(browser, {id: aId}, function* () {

Same as above, re: passing ID as the second argument.

@@ +288,5 @@
>    let perms = Services.perms.enumerator;
>    while (perms.hasMoreElements()) {
>      let perm = perms.getNext();
>      if (perm.type.startsWith('plugin')) {
> +      dump("perm " + perm.host + " " + perm.type + "\n");

Is this useful? If not, remove it. If so, maybe use info() instead.

@@ +343,5 @@
>  function resetBlocklist() {
>    Services.prefs.setCharPref("extensions.blocklist.url", _originalTestBlocklistURL);
>  }
>  
> +// Insure there's a popup notification present and that it's displayed 

Nit - trailing ws

@@ +344,5 @@
>    Services.prefs.setCharPref("extensions.blocklist.url", _originalTestBlocklistURL);
>  }
>  
> +// Insure there's a popup notification present and that it's displayed 
> +function promisePopupNotificationShown(aName, aAction) {

What's the point of action? Why can't the caller just do action after the promise resolves? And shouldn't we make it possible to pass in the browser here in case we're dealing with multiple browser tabs? Then pass the browser as the second argument to getNotification.

@@ +403,5 @@
> + * @param aObsEvent the observer event to wait for
> + * @param aTimeoutMs the number of miliseconds to wait before giving up
> + * @returns a Promise that resolves to true, or to an Error
> + */
> +function promiseForObserver(aObsEvent, aTimeoutMs, aObsData) {

We have https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/testing/modules/TestUtils.jsm#l48 to do this.

However, that one doesn't have a timeout, which I think is a good idea. We might want to augment topicObserved with timeout capabilities instead of adding a new function that does essentially the same thing.

@@ +512,5 @@
>    notification.reshow();
>  }
>  
> +function promiseForNotificationShown(notification)
> +{

Nit - brace on line 515

@@ +519,5 @@
> +  return deferred.promise;
> +}
> +
> +function promiseToReshowNotfication(aNotification)
> +{

Nit - brace on line 522

@@ +525,5 @@
> +  let condition = function() {
> +    return !aNotification.dismissed && PopupNotifications.panel.firstChild;
> +  }
> +  aNotification.reshow();
> +  return promiseForCondition(condition);

Shouldn't this be promisePopupNotificationShown?

@@ +562,5 @@
>   *        The URI to load.
>   *
>   * @return Promise
>   */
> +function promiseLoadPage(browser, uri) {

Isn't this a subset of what promiseTabLoadEvent does?

Maybe we should just add a method to do load a URI and wait for a particular event to BrowserTestUtils instead of doing it here.

BrowserTestUtils is where all of the super-common browser testing code should go, as a way to stop reinventing the wheel. :)
Attachment #8587591 - Flags: review?(mconley) → review-
Comment on attachment 8587592 [details] [diff] [review]
tests part 2

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

Hey jimm! Thanks so much for cleaning this stuff up. These are ending up wayyyyyyyyyy better than their current state, so big thumbs up. :)

I was a bit repetitive during parts of my review, since the same issues kept coming up. I don't blame you though - the tests you were working off of were pretty old and crufty. This couldn't have been a whole lot of fun.

::: browser/base/content/test/plugins/browser_clearplugindata.js
@@ +1,2 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + *  License, v. 2.0. If a copy of the MPL was not distributed with this

Test files are all supposed to be public domain, I believe.

@@ +4,2 @@
>  
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");

I know we're in the global scope here, but let is (in my books) greatly preferred over var. Please try not to introduce new vars.

@@ +4,3 @@
>  
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);

You should have access to Cc and Ci here.

@@ +4,4 @@
>  
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);
> +var gTestBrowser = null;

I can't see much purpose to gTestBrowser in our add_task world.

@@ +19,5 @@
>  var pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
>  pluginHost.QueryInterface(pluginHostIface);
>  
>  var pluginTag = getTestPlugin();
> +var sanitizer = null;

Hrm. Now we have sanitizer and Sanitizer. I think we should maybe name these two different things... Maybe this is gSanitizer?

@@ +36,5 @@
>    }
>    return true;
>  }
>  
> +Components.utils.import("resource://gre/modules/Services.jsm");

Services should already be available to the test.

@@ +41,3 @@
>  
> +add_task(function* () {
> +  registerCleanupFunction(Task.async(function*() {

Why is this an async generator function? None of the method calls in this function, as far as I can tell, are async or yield.

@@ +62,5 @@
> +
> +  sanitizer = new Sanitizer();
> +  sanitizer.ignoreTimespan = false;
> +  sanitizer.prefDomain = "privacy.cpd.";
> +  let itemPrefs = gPrefService.getBranch(sanitizer.prefDomain);

Services.prefs

@@ +89,4 @@
>  
> +  // Clear 20 seconds ago
> +  let now_uSec = Date.now() * 1000;
> +  sanitizer.range = [now_uSec - 20*1000000, now_uSec];

Spaces on either side of *

@@ +104,2 @@
>  
> +  gBrowser.removeCurrentTab();

Instead of adding the tab and removing it at the end of the test, use BrowserTestUtils.withNewTab.

Example: https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/toolkit/components/passwordmgr/test/browser/browser_notifications.js#l147

This will take care of opening the tab, waiting for it to load, and then closing it for you when it's done.

@@ +113,2 @@
>  
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, testURL2);

Same as above, re: BrowserTestUtils.withNewTab.

::: browser/base/content/test/plugins/browser_plugin_infolink.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Public domain license.

@@ +2,5 @@
> + *  License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);

gPluginHost is never used in this file, and Cc / Ci are available to you.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);
> +var gTestBrowser = null;

No need for gTestBrowser.

@@ +5,5 @@
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);
> +var gTestBrowser = null;
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");

No need to import Services.jsm.

@@ +8,5 @@
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +add_task(function* () {
> +  registerCleanupFunction(Task.async(function*() {

No need for this to be async, nor for it to reference gTestBrowser or to remove the current tab.

@@ +19,5 @@
> +  }));
> +});
> +
> +add_task(function* () {
> +  gBrowser.selectedTab = gBrowser.addTab();

Let's use BrowserTestUtils.withNewTab.

@@ +35,5 @@
> +
> +  yield asyncSetAndUpdateBlocklist(gTestRoot + "blockNoPlugins.xml", gTestBrowser);
> +});
> +
> +add_task(function* () {

Can you quickly add some documentation to describe what this test tests?

@@ +36,5 @@
> +  yield asyncSetAndUpdateBlocklist(gTestRoot + "blockNoPlugins.xml", gTestBrowser);
> +});
> +
> +add_task(function* () {
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_test.html");

BrowserTestUtils.withNewTab

@@ +42,5 @@
> +  yield promiseUpdatePluginBindings(gTestBrowser);
> +
> +  let fallbackType = yield promiseForObjectFallbackType("test");
> +  is(fallbackType, Ci.nsIObjectLoadingContent.PLUGIN_DISABLED,
> +     "Test 1a, plugin fallback type should be PLUGIN_DISABLED");

Remove "Test 1a"

@@ +64,5 @@
> +  info("TabOpen received.");
> +
> +  promise = promiseForEvent(gBrowser.tabContainer, "TabClose", true);
> +
> +  // in-process page, no cpows here

For now, anyway. Not sure if we ever plan to make about:addons remote, but it wouldn't surprise me.

@@ +68,5 @@
> +  // in-process page, no cpows here
> +  let win = gBrowser.selectedBrowser.contentWindow;
> +  let condition = function() {
> +    win = gBrowser.selectedBrowser.contentWindow;
> +    if (!!win && !!win.wrappedJSObject && !!win.wrappedJSObject.gViewController) {

You can probably just:

if (win && win.wrappedJSObject && win.wrappedJSObject.gViewController) {
  // ...
}

::: browser/base/content/test/plugins/browser_plugin_reloading.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Public domain.

@@ +2,5 @@
> + *  License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);

gPluginHost is never used in this file. And Cc and Ci are available to you.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);
> +var gTestBrowser = null;

No gTestBrowser please.

@@ +5,5 @@
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);
> +var gTestBrowser = null;
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");

No need to import this.

@@ +13,5 @@
> +  setTestPluginEnabledState(aState, "Second Test Plug-in");
> +}
> +
> +add_task(function* () {
> +  registerCleanupFunction(Task.async(function*() {

No need to be async or generator. Or to remove the tab. Or to reset gTestBrowser.

@@ +27,5 @@
> +  }));
> +});
> +
> +add_task(function* () {
> +  gBrowser.selectedTab = gBrowser.addTab();

Use BrowserTestUtils.withNewTab.

@@ +49,5 @@
> +  clearAllPluginPermissions();
> +
> +  updateAllTestPlugins(Ci.nsIPluginTag.STATE_CLICKTOPLAY);
> +
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_test.html");

Use BrowserTestUtils.withNewTab

@@ +70,5 @@
> +  let displayType = yield promiseForObjectDisplayType("test");
> +  is(displayType, Ci.nsIObjectLoadingContent.TYPE_PLUGIN,
> +    "Test 3, plugin should have started");
> +  let isActivated = yield promiseForObjectActivationState("test");
> +  ok(isActivated, "Test 4, plugin node should not be activated");

Please remove "Test 1", "Test 2", etc from your checks / messages.

@@ +74,5 @@
> +  ok(isActivated, "Test 4, plugin node should not be activated");
> +
> +  let result = yield ContentTask.spawn(gTestBrowser, {}, function* () {
> +    let plugin = content.document.getElementById("test");
> +    let npobj1 = Components.utils.waiveXrays(plugin).getObjectValue();

Cu is available to you in this scope.

::: browser/base/content/test/plugins/browser_pluginnotification.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Public domain.

@@ +4,3 @@
>  
> +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);

This is never used.

@@ +7,1 @@
>  var gTestBrowser = null;

No gTestBrowser please.

@@ +7,4 @@
>  var gTestBrowser = null;
> +
> +// the main test content tab.
> +let gTestTab = null;

Let's not hold a global on tabs either.

@@ +11,2 @@
>  
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import this.

@@ +11,4 @@
>  
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  
> +function updateAllTestPlugins(aState) {

Maybe this is something that could go into head.js? I've seen you use it a few times.

@@ +17,4 @@
>  }
>  
> +add_task(function* () {
> +  registerCleanupFunction(Task.async(function*() {

No need for async or generator. Also, no need to reference gTestBrowser, gTestTab or to remove the tab if you use BrowserTestUtils.withNewTab.

@@ +33,3 @@
>  
> +add_task(function* () {
> +  let newTab = gBrowser.addTab();

Use BrowserTestUtils.withNewTab.

@@ +52,4 @@
>  
>  // Tests a page with an unknown plugin in it.
> +add_task(function* () {
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_unknown.html");

Use BrowserTestUtils.withNewTab

@@ +59,4 @@
>  
> +  let fallbackType = yield promiseForObjectFallbackType("unknown");
> +  is(fallbackType, Ci.nsIObjectLoadingContent.PLUGIN_UNSUPPORTED,
> +     "Test 1a, plugin fallback type should be PLUGIN_UNSUPPORTED");

Remove "Test #" prefixes from messages.

@@ +87,5 @@
>  // Tests that the "Allow Always" permission works for click-to-play plugins
> +add_task(function* () {
> +  updateAllTestPlugins(Ci.nsIPluginTag.STATE_CLICKTOPLAY);
> +
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_test.html");

Use BrowserTestUtils.withNewTab

@@ +111,5 @@
>  
>  // Test that the "Always" permission, when set for just the Test plugin,
>  // does not also allow the Second Test plugin.
> +add_task(function* () {
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_two_types.html");

Use BrowserTestUtils.withNewTab. I won't mention this again for the rest of this file.

@@ +195,5 @@
> +    let left = (bounds.left + bounds.right) / 2;
> +    let top = (bounds.top + bounds.bottom) / 2;
> +    let utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                       .getInterface(Components.interfaces.nsIDOMWindowUtils);
> +    utils.sendMouseEvent("mousedown", left, top, 0, 1, 0, false, 0, 0);

I wonder if we should just add a ContentTaskUtils.clickAtCenter(element); to ContentTaskUtils.jsm?

::: browser/base/content/test/plugins/browser_plugins_added_dynamically.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Tests go into the public domain.

@@ +8,2 @@
>  
>  let gTestBrowser = null;

We likely don't need gTestBrowser. Just use BrowserTestUtils.withNewTab (whose function gets passed the browser), or gBrowser.selectedBrowser if you just need the current browser.

@@ +10,2 @@
>  
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import this - Services is available already.

@@ +10,5 @@
>  
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  
> +add_task(function* () {
> +  registerCleanupFunction(Task.async(function*() {

Why is this async? And a generator?

@@ +30,1 @@
>    let newTab = gBrowser.addTab();

Why are we adding a tab here? It's not being used until the next test. If we need to do this before we setTestPluginEnabledState, can't the calls in the next add_task just be added into this test?

Otherwise, let's just open the new tab in the next addTask (and probably use BrowserTestUtils.withNewTab while we're at it).

@@ +43,3 @@
>  
> +  let notification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser);
> +  ok(!notification, "Test 1a, Should not have a click-to-play notification");

"Test 1a" doesn't really have much meaning here...

@@ +46,4 @@
>  
> +  // Add a plugin of type test
> +  yield ContentTask.spawn(gTestBrowser, {}, function* () {
> +    new XPCNativeWrapper(XPCNativeWrapper.unwrap(content).addPlugin("pluginone", "application/x-test"));

I'm curious - why do we have to wrap this in the construction of a new XPCNativeWrapper? I don't see the old addPlugin doing this...

@@ +53,3 @@
>  
> +  notification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser);
> +  ok(notification, "Test 1a, Should not have a click-to-play notification");

Remove "Test 1a"

@@ +62,5 @@
>        centerAction = action;
>        break;
>      }
>    }
> +  ok(centerAction, "Test 2, found center action for the Test plugin");

Remove "Test 2"

@@ +67,4 @@
>  
>    let centerItem = null;
>    for (let item of PopupNotifications.panel.firstChild.childNodes) {
> +    is(item.value, "block", "Test 3, all plugins should start out blocked");

Remove "Test 3"

@@ +72,5 @@
>        centerItem = item;
>        break;
>      }
>    }
> +  ok(centerItem, "Test 4, found center item for the Test plugin");

Remove "Test 4" - basically, just remove the old test subfunction name things.

@@ +91,2 @@
>  
> +  isActivated |= yield promiseForObjectActivationState("plugintwo");

Why |=? Why not just =? We already know whether or not pluginone is activated from line 85.

@@ +100,2 @@
>  
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_add_dynamically.html");

BrowserTestUtils.withNewTab.

@@ +119,5 @@
> +
> +  let isActivated = yield promiseForObjectActivationState("pluginone");
> +  ok(!isActivated, "Test 8, test plugin should be activated");
> +
> +  let condition = function() !notification.dismissed &&

Why are we checking this? What did we expect to have changed this?
Attachment #8587592 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #53)
> Comment on attachment 8587591 [details] [diff] [review]
> tests part 1
> 
> Review of attachment 8587591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/plugins/browser.ini
> @@ +63,5 @@
> > +[browser_CTP_crashreporting.js]
> > +skip-if = e10s || !crashreporter
> > +[browser_CTP_drag_drop.js]
> > +skip-if = e10s # misc. issues with tab drag out and e10s
> > +
> 
> Nit - why the newlines here, at 76 and 85?

Just breaking up the different blocks of test for organizational purposes.

> ::: browser/base/content/test/plugins/head.js
> @@ +28,5 @@
> > + * @param aSynthEvents listen for synth events too
> > + * @param aTimeoutMs the number of miliseconds to wait before giving up
> > + * @returns a Promise that resolves to the received event, or to an Error
> > + */
> > +function promiseForEvent(aSubject, aEventName, aCapture, aSynthEvents, aTimeoutMs, aTarget) {
> 
> Does this offer anything that
> https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/testing/mochitest/
> BrowserTestUtils/BrowserTestUtils.jsm#l189 doesn't? If so, we might want to
> augment the code in BrowserTestUtils.jsm instead and get rid of this
> specialized function so that other tests can take advantage of it.

I wasn't aware of this file but there's not much in here so no major harm. I'll see if I can reuse what we have here and repost.
Comment on attachment 8587580 [details] [diff] [review]
fix for blocklist updates v.2

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

::: toolkit/mozapps/extensions/nsBlocklistServiceContent.js
@@ +34,5 @@
> +    Services.obs.addObserver(this, "xpcom-shutdown", false);
> +  },
> +
> +  uninit: function () {
> +    Services.cpmm.removeMessageListener("Blocklist::blocklistInvalidated", this);

Just wanted to point out that the usual convention for message names is Foo:bar (only one colon).
Comment on attachment 8587926 [details] [diff] [review]
query the chrome blocklist service v.2

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

::: dom/plugins/base/nsPluginHost.cpp
@@ +1282,5 @@
>    if (pluginTag) {
> +    // When setting up a bridge, double check with chrome to see if this plugin
> +    // is blocked hard. Note this does not protect against vulnerable plugins
> +    // that the user has explicitly allowed. :(
> +    if (pluginTag->IsBlocklistedInChrome()) {

Since you're already in the parent process, couldn't you just use IsBlocklisted() here?
Attachment #8587926 - Flags: review?(wmccloskey) → review+
Comment on attachment 8587927 [details] [diff] [review]
implement remote blocklist query v.2

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

::: dom/ipc/ContentParent.cpp
@@ +1017,5 @@
>      return mozilla::plugins::SetupBridge(aPluginId, this, true, aRv);
>  }
>  
>  bool
> +ContentParent::RecvGetBlocklistState(const nsCString& aPluginName,

Each nsPluginTag has an mId field that is shared between content and chrome. It would be better to use that instead of the name. Then you can use nsPluginHost::PluginWithId here and you don't need to add FindTagWithName.

::: dom/plugins/base/nsPluginHost.h
@@ +28,5 @@
>  #include "nsIIDNService.h"
>  #include "nsCRT.h"
>  
>  #ifdef XP_WIN
> +#include <minwindef.h>

What's this for?

::: dom/plugins/base/nsPluginTags.cpp
@@ +469,5 @@
> +  return bls == nsIBlocklistService::STATE_BLOCKED;
> +}
> +
> +nsresult
> +nsPluginTag::GetBlocklistedStateInChrome(uint32_t *aResult)

If it's possible, it seems cleaner to me to change nsPluginTag::GetBlocklistedState to have a check at the top to see if we're in the content process. If we are, it could do the IPC thing and return. Then we wouldn't need the special *InChrome functions. I think the patch would be smaller, since you wouldn't have to change any callers.

The way you have it now, it's more likely that people will call GetBlocklistedState in the content process and get the wrong answer. For example, it looks like nsPluginHost::GetPermissionStringForType does the wrong thing.
Attachment #8587927 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #58)
> Comment on attachment 8587927 [details] [diff] [review]
> implement remote blocklist query v.2
> 
> Review of attachment 8587927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentParent.cpp
> @@ +1017,5 @@
> >      return mozilla::plugins::SetupBridge(aPluginId, this, true, aRv);
> >  }
> >  
> >  bool
> > +ContentParent::RecvGetBlocklistState(const nsCString& aPluginName,
> 
> Each nsPluginTag has an mId field that is shared between content and chrome.
> It would be better to use that instead of the name. Then you can use
> nsPluginHost::PluginWithId here and you don't need to add FindTagWithName.

Ah, nice. Thanks.

> ::: dom/plugins/base/nsPluginHost.h
> @@ +28,5 @@
> >  #include "nsIIDNService.h"
> >  #include "nsCRT.h"
> >  
> >  #ifdef XP_WIN
> > +#include <minwindef.h>
> 
> What's this for?

I had to remove napi.h from nsPluginHost.h so that I was able include it in ContentParent.cpp. The napi header pulls in some osx sdk junk specific to a certain sdk version. This broke the build. Removing that include solved the problem. 


> ::: dom/plugins/base/nsPluginTags.cpp
> @@ +469,5 @@
> > +  return bls == nsIBlocklistService::STATE_BLOCKED;
> > +}
> > +
> > +nsresult
> > +nsPluginTag::GetBlocklistedStateInChrome(uint32_t *aResult)
> 
> If it's possible, it seems cleaner to me to change
> nsPluginTag::GetBlocklistedState to have a check at the top to see if we're
> in the content process. If we are, it could do the IPC thing and return.
> Then we wouldn't need the special *InChrome functions. I think the patch
> would be smaller, since you wouldn't have to change any callers.
>
> The way you have it now, it's more likely that people will call
> GetBlocklistedState in the content process and get the wrong answer. For
> example, it looks like nsPluginHost::GetPermissionStringForType does the
> wrong thing.

This will trigger a lot more traffic over this call.. I'm not sure what effect that'll have. I'll push it at try though, see what happens.
Comment on attachment 8587593 [details] [diff] [review]
tests part 3

since head.js is changes, these files will change as well.
Attachment #8587593 - Flags: review?(felipc)
Attachment #8587594 - Flags: review?(mconley)
Attachment #8587595 - Flags: review?(felipc)
I'm kind of surprised these changes didn't trigger bug 1150861:

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

even with this patch, which changed the default in a number of locations:

https://hg.mozilla.org/try/rev/414dfb3b448a

Which is good news since we can move forward.
Attached patch tests-wip (obsolete) — Splinter Review
Attachment #8587591 - Attachment is obsolete: true
Attachment #8587592 - Attachment is obsolete: true
Attachment #8587593 - Attachment is obsolete: true
Attachment #8587594 - Attachment is obsolete: true
Attachment #8587595 - Attachment is obsolete: true
Attachment #8587601 - Attachment is obsolete: true
Attachment #8587927 - Attachment is obsolete: true
Attachment #8595422 - Attachment is obsolete: true
Attached file MozReview Request: bz://1129040/jimm (obsolete) —
/r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop
/r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service.
/r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop
/r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm
/r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley

Pull down these commits:

hg pull -r 960bd216241dbdfe69f2be3b24d42b005bb7561e https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8595517 [details]
MozReview Request: bz://1129040/jimm

/r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop
/r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service.
/r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop
/r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm
/r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley

Pull down these commits:

hg pull -r 960bd216241dbdfe69f2be3b24d42b005bb7561e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8595517 - Flags: review?(wmccloskey)
Attachment #8595517 - Flags: review?(mconley)
Comment on attachment 8595517 [details]
MozReview Request: bz://1129040/jimm

/r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop
/r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service.
/r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop
/r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm
/r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley

Pull down these commits:

hg pull -r 960bd216241dbdfe69f2be3b24d42b005bb7561e https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/7393/#review6161

::: dom/ipc/ContentParent.cpp:1074
(Diff revision 1)
> +  

Extra whitespace.

::: dom/ipc/ContentParent.cpp:1075
(Diff revision 1)
> +    return NS_SUCCEEDED(tag->GetBlocklistState(aState)) ? true : false;

No need for the ? true : false stuff.

::: dom/plugins/base/nsPluginHost.h:225
(Diff revision 1)
> +  nsPluginTag* FindTagWithName(const nsCString& aName);

This isn't used anymore.

::: dom/ipc/ContentParent.cpp:159
(Diff revision 1)
> +#include "nsPluginHost.h"

Now I remember the problem you mentioned with including plugin stuff in ContentParent.cpp. I fixed it by creating a new file, PluginBridge.h, that exports stuff that ContentParent.cpp can use. That's not really any better than what you've done in nsPluginHost.h, but it's an option if you prefer it.

::: dom/plugins/base/nsPluginHost.cpp:1722
(Diff revision 1)
> +nsPluginHost::FindTagWithName(const nsCString& aName)

Also unused.

::: dom/plugins/base/nsPluginTags.cpp:655
(Diff revision 1)
> +    if (cp) {

I don't think there's any need to null-check |cp|. It's always there.

::: dom/plugins/base/nsPluginTags.cpp:650
(Diff revision 1)
> +  // can fail quite easily.

Uhhh, what?
Comment on attachment 8595517 [details]
MozReview Request: bz://1129040/jimm

...yet another confusing ReviewBoard experience. Isn't it supposed to set the r+ flag somehow?
Attachment #8595517 - Flags: review?(wmccloskey) → review+
https://reviewboard.mozilla.org/r/7399/#review6267

::: browser/base/content/test/plugins/blocklist_proxy.js:5
(Diff revision 1)
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;
> +let Cu = Components.utils;
> +let Cm = Components.manager;

These should probably be consts.

::: browser/base/content/test/plugins/blocklist_proxy.js:20
(Diff revision 1)
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,

You never import XPCOMUtils in this file - you should probably do that just in case.

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:1
(Diff revision 1)
> -var rootDir = getRootDirectory(gTestPath);
> -const gTestRoot = rootDir;
> -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit - I don't think tests are supposed to be MPL'd - I think they're supposed to be public domain. I'm pretty sure you can just strip the license from any of these tests.

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:12
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

The generator that you're passing to Task.async doesn't yield. Shouldn't this just be a normal function?

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:63
(Diff revision 1)
> -
> +  yield promiseForCondition(() => window.document.getElementById("context-ctp-play"));

No need for "window." prefix here or on line 65.

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:66
(Diff revision 1)
> -  ok(activate, "Test 2, Should have a context menu entry for activating the plugin");
> +  ok(actMenuItem, "Should have a context menu entry for activating the plugin");

Isn't this implied by line 63?

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:7
(Diff revision 1)
> -var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);
> +let gTestBrowser = null;

Let's get rid of gTestBrowser - there's only one function now.

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:20
(Diff revision 1)
> +    gTestBrowser = null;

Let's get rid of gTestBrowser.

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:30
(Diff revision 1)
>    gTestBrowser = gBrowser.selectedBrowser;

This can probably just be "browser" - no need to share gTestBrowser between functions now.

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:1
(Diff revision 1)
> -var rootDir = getRootDirectory(gTestPath);
> -const gTestRoot = rootDir;
> -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Remove license.

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:10
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

Services is just available to all tests - I think this can be removed.

::: browser/base/content/test/plugins/browser_CTP_context_menu.js:9
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

Services should be available to all tests - this can just be removed, I think.

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:13
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

Just use a normal function instead of a generator passed to Task.async.

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:29
(Diff revision 1)
>    Services.prefs.setBoolPref("plugins.click_to_play", true);
> -  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY);
> +  Services.prefs.setBoolPref("extensions.blocklist.suppressUI", true);
> -  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Second Test Plug-in");
>  
> -  prepareTest(test1a, gHttpTestRoot + "plugin_data_url.html");
> -}
> +  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Test Plug-in");
> +  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Second Test Plug-in");

This setup stuff can probably just be added to the same add_task that adds your cleanup function. Might as well keep setup and teardown together.

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:67
(Diff revision 1)
> +    let plugin = content.document.getElementById("test");
> +    let bounds = plugin.getBoundingClientRect();
> +    let left = (bounds.left + bounds.right) / 2;
> +    let top = (bounds.top + bounds.bottom) / 2;
> +    let utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                       .getInterface(Components.interfaces.nsIDOMWindowUtils);
> +    utils.sendMouseEvent("mousedown", left, top, 0, 1, 0, false, 0, 0);
> +    utils.sendMouseEvent("mouseup", left, top, 0, 1, 0, false, 0, 0);

A note for future reference that Enn landed some tools to do clicking on the content process in bug 1131818. I don't think you should worry about using those here though - just for the future.

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:79
(Diff revision 1)
> -  waitForNotificationShown(popupNotification, function() {
> +  let condition = function() !PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser).dismissed &&
> +    PopupNotifications.panel.firstChild;

I think we can effectively abandon putting this on one line, and just use:

```
let condition = () => {
  let notification =
    PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser);
  return !notification.dismissed && PopupNotifications.panel.firstChild;
}
```

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:164
(Diff revision 1)
>  function test2c() {
> -  let plugin = gTestBrowser.contentDocument.getElementById("test1");
> +  // We click activated above

This test looks like it's never run now...

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:175
(Diff revision 1)
>  function test3a() {

This test looks like it's never run now...

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:217
(Diff revision 1)
> -function test3c() {
> +function test3b() {

This test looks like it's never run now...
https://reviewboard.mozilla.org/r/7399/#review6275

::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:8
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import Services.

::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:1
(Diff revision 1)
> -/* Any copyright is dedicated to the Public Domain.
> - * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

No MPL - the public domain license was correct.

::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:11
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

No need for a generator / Task.async. Just use a normal function here.

::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:42
(Diff revision 1)
> -  waitForCondition(condition, part3, "Waited too long for click-to-play notification");
> -}
> +  // XXX technically can't load fire before we get this call???
> +  yield waitForEvent(gNewWindow, "load", null, true);

Maybe. You could work around that possibility by setting the promise before you call replaceTabWithWindow, and then yielding it.

::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:1
(Diff revision 1)
> -var rootDir = getRootDirectory(gTestPath);
> -const gTestRoot = rootDir;
> -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

No MPL.

::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:8
(Diff revision 1)
> +let gTestBrowser = null;

There's just a single add_task - no need for a global reference to the browser.

::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:10
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import Services.

::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:13
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

Just use a normal function here.

::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:19
(Diff revision 1)
> +    gBrowser.removeCurrentTab();

Instead of removing the current tab in the teardown, since this is a short test, just use BrowserTestUtils.withNewTab.

::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:30
(Diff revision 1)
>    gTestBrowser = gBrowser.selectedBrowser;

Just use a local variable instead of the gTestBrowser global.

::: browser/base/content/test/plugins/browser_CTP_iframe.js:1
(Diff revision 1)
> -let rootDir = getRootDirectory(gTestPath);
> -const gTestRoot = rootDir;
> -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

No MPL.

::: browser/base/content/test/plugins/browser_CTP_iframe.js:9
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import Services.

::: browser/base/content/test/plugins/browser_CTP_iframe.js:12
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

Just use a normal function here. No need to pass a generator or use Task.async.

::: browser/base/content/test/plugins/browser_CTP_iframe.js:7
(Diff revision 1)
>  let gTestBrowser = null;

No need for gTestBrowser global.

::: browser/base/content/test/plugins/browser_CTP_iframe.js:29
(Diff revision 1)
>    gTestBrowser = gBrowser.selectedBrowser;

Just use a browser local variable, or even better, use BrowserTestUtils.withNewTab.

::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:4
(Diff revision 1)
> -
> +let gTestBrowser = null;

No need for gTestBrowser.

::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:6
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import Services.

::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:9
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

We can just pass a normal function to registerCleanupFunction.

::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:26
(Diff revision 1)
>    gTestBrowser = gBrowser.selectedBrowser;

No need to set a gTestBrowser global - better yet, use BrowserTestUtils.withNewTab.

::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:63
(Diff revision 1)
> -
> +  

Trailing whitespace.

::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:4
(Diff revision 1)
> -
> +let gTestBrowser = null;

No need for gTestBrowser global.

::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:6
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import Services.

::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:9
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

Just pass a normal function to registerCleanupFunction.

::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:27
(Diff revision 1)
>    gTestBrowser = gBrowser.selectedBrowser;

No need to use gTestBrowser global - better yet, use BrowserTestUtils.withNewTab.

::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:5
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");

No need to import Services.

::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:8
(Diff revision 1)
> -  waitForExplicitFinish();
> +  registerCleanupFunction(Task.async(function*() {

Just pass a normal function to registerCleanupFunction.

::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:40
(Diff revision 1)
> +  

Trailing whitespace.

::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:52
(Diff revision 1)
> +  

Trailing whitespace.

::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:78
(Diff revision 1)
>       "Test 3b, plugin fallback type should be PLUGIN_CLICK_TO_PLAY");

Nit - these "Test X" prefixes no longer make sense in our add_task-y world.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #70)
> ::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:42
> (Diff revision 1)
> > -  waitForCondition(condition, part3, "Waited too long for click-to-play notification");
> > -}
> > +  // XXX technically can't load fire before we get this call???
> > +  yield waitForEvent(gNewWindow, "load", null, true);
> 
> Maybe. You could work around that possibility by setting the promise before
> you call replaceTabWithWindow, and then yielding it.

There's no window to set it on, the call to replaceTabWithWindow creates it.
Updated except the following

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #69)
> ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:7
> (Diff revision 1)
> > -var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost);
> > +let gTestBrowser = null;
> 
> Let's get rid of gTestBrowser - there's only one function now.

I've updated the test you've mentioned here to remove gTestBrowser.  However be careful if you work on these, head.js relies on gTestBrowser in a few helpers if you don't pass a browser to them so removing more of these will likely have to include updating the head.js callers that are currently relying on this hidden behavior.


> ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:67
> (Diff revision 1)
> > +    let plugin = content.document.getElementById("test");
> > +    let bounds = plugin.getBoundingClientRect();
> > +    let left = (bounds.left + bounds.right) / 2;
> > +    let top = (bounds.top + bounds.bottom) / 2;
> > +    let utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> > +                       .getInterface(Components.interfaces.nsIDOMWindowUtils);
> > +    utils.sendMouseEvent("mousedown", left, top, 0, 1, 0, false, 0, 0);
> > +    utils.sendMouseEvent("mouseup", left, top, 0, 1, 0, false, 0, 0);
> 
> A note for future reference that Enn landed some tools to do clicking on the
> content process in bug 1131818. I don't think you should worry about using
> those here though - just for the future.

- skipped

> ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:79
> (Diff revision 1)
> > -  waitForNotificationShown(popupNotification, function() {
> > +  let condition = function() !PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser).dismissed &&
> > +    PopupNotifications.panel.firstChild;
> 
> I think we can effectively abandon putting this on one line, and just use:
> 
> ```
> let condition = () => {
>   let notification =
>     PopupNotifications.getNotification("click-to-play-plugins",
> gTestBrowser);
>   return !notification.dismissed && PopupNotifications.panel.firstChild;
> }
> ```

- skipped (kinda bike sheddy imo)

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #70)
> ::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:27
> (Diff revision 1)
> >    gTestBrowser = gBrowser.selectedBrowser;
> 
> No need to use gTestBrowser global - better yet, use
> BrowserTestUtils.withNewTab.

- skipped

> ::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:78
> (Diff revision 1)
> >       "Test 3b, plugin fallback type should be PLUGIN_CLICK_TO_PLAY");
> 
> Nit - these "Test X" prefixes no longer make sense in our add_task-y world.

- skipped
Comment on attachment 8595517 [details]
MozReview Request: bz://1129040/jimm

/r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop
/r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service.
/r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop
/r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm
/r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley

Pull down these commits:

hg pull -r 222b533e2239c4f972ab18ff5673adad3580e5a3 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8595517 - Flags: review+ → review?(wmccloskey)
Attachment #8595517 - Flags: review?(wmccloskey)
(In reply to Jim Mathies [:jimm] from comment #73)
> Comment on attachment 8595517 [details]
> MozReview Request: bz://1129040/jimm
> 
> /r/7391 - Bug 1129040 - Forward blocklist update notifications to the
> content process. r=Mossop
> /r/7393 - Bug 1129040 - Provide a way for content processes to query the
> chrome side blocklist service.
> /r/7395 - Bug 1129040 - In the content process blocklist shim fail in all
> addon related calls. r=Mossop
> /r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to
> instantiating plugins in the content process. r=billm
> /r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable
> for e10s. r=mconley
> 
> Pull down these commits:
> 
> hg pull -r 222b533e2239c4f972ab18ff5673adad3580e5a3
> https://reviewboard-hg.mozilla.org/gecko/

I'm concerned that this didn't create an interdiff. Please let me know if it didn't and we can try and figure out why.
Comment on attachment 8595517 [details]
MozReview Request: bz://1129040/jimm

https://reviewboard.mozilla.org/r/7389/#review6407

Thanks jimm - and sorry for my pedantry. I think you've made these tests strictly better. Let's drop the hammer. :)

::: browser/base/content/test/plugins/browser_CTP_data_urls.js:216
(Diff revisions 1 - 2)
> +// Fails, bug XXX. Plugins plus a data url don't fire a load event.

We should get a bug filed on this, and just remove the dead code.
Attachment #8595517 - Flags: review?(mconley) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9401441&repo=mozilla-inbound
Flags: needinfo?(jmathies)
Strange, the fix for this test was in that push, and it all came up green on try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d76547017b3
Flags: needinfo?(jmathies)
Pushed another to try with no changes to confirm things are ok there:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f1dd77da52
new try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=069dd04e659f

changes:
- disabled browser_plugin_infolink.js under e10s
- filed follow up bug 1160166
Attachment #8595517 - Attachment is obsolete: true
Used the following build for verification:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-04-03-02-10-mozilla-central/

For each OS, I went through the following test case on both e10s and non-e10s modes:

* ensured that flash appears blocked under about:addons
** ensure that "Ask to Activate" is being used when flash is being blocked
* ensured that the "Nightly has prevented the outdated plugin "Adobe Flash" from running..." message correctly appears at the top of the page
* ensured that the "This plugin is vulnerable and should be update" message appears when attempting to play a flash video
* ensure that clicking on a blocked video displays the doorhanger that allows you to either "Allow Now" or "Allow and Remember"
** ensure "Allow Now" is working (should only work for the current session)
** ensure "Allow and Remember" is working (should always work, even after closing/re-opening fx)
* ensured that "Always Activate" is grayed out via about:addons when flash is being blocked

Websites used during testing:

- http://ca.ign.com
- https://apps.facebook.com/candycrush/
- http://espn.go.com
- http://www.espnfc.us

OS's Used:

* Win 7 x64 Pro - PASSED
** Used the following blocked version of Flash: Flash 16.0.0.296
** Used the following working version of flash: Flash 17.0.0.169

* Ubuntu 14.04.1 - PASSED
** Used the following blocked version of Flash: Flash 11.2.202.425
** Used the following working version of flash: Flash 11.2.202.457

* OSX 10.10.3 - PASSED
** Used the following blocked version of Flash: Flash 13.0.0.259 
** Used the following working version of flash: Flash 17.0.0.169

Jim, let me know if this is good enough coverage. Looks like it's working on m-c.
Flags: needinfo?(jmathies)
Great! Thank you!
Flags: needinfo?(jmathies)
Depends on: 1252870
Depends on: 1463088
You need to log in before you can comment on or make changes to this bug.