899347 - Make click-to-play work in e10s

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Comment 1

[Jim Mathies [:jimm]]

STR:

- enable remote tabs, remote auto start
- restart and visit a page with plugins

expected: you should get a click-to-play overlay on plugin windows
results: you get a gray background without the activate message/link

Comment 3

[Daniel "warmth" Delgado]

The fix was reverted and now it isn't working again.

Comment 4

[Achwaq Khalid]

So e10s-m2 will ship with this not fixed?

Comment 5

[Matt Brubeck (:mbrubeck)]

I can work on this, though I won't be working on it full time so I can't promise how soon I can finish.  (If anyone else wants to help out, just let me know.)

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This is patch is about 1/3 complete and not very functional yet, but it's getting there.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

Slightly farther along, but broken by the fact that Services.perms.addFromPrincipal is not available in the content process, while content.document.nodePrincipal is not available in the parent process.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 3

Basic click-to-play functionality now works in e10s windows!  Still need to port other types of notifications and behaviors, and fix some breakage in non-e10s windows.

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 4

Fixed some more bugs; click-to-play now works in both e10s and non-e10s windows.  Overall this is about 50% done.  Most of the remaining work is in the "install" and "play preview" code.

Comment 10

[Achwaq Khalid]

Hi Matt!

the problem when E10S is enabled now is you can not switch between "Ask to activate, Always activate, never activate" or "check for update, view recent update...." in the about:addons tab!!

let me know if you want me to file a bug file for that, cheers

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 5

Now with working click-to-install and fixes for some errors.

TODO: Play-preview and crash plugin notifications.

(In reply to Achwaq Khalid from comment #10)
> the problem when E10S is enabled now is you can not switch between "Ask to
> activate, Always activate, never activate" or "check for update, view recent
> update...." in the about:addons tab!!
> 
> let me know if you want me to file a bug file for that, cheers

Yes, that sounds like a separate bug.  Could you file a new bug report, please?  (Note: I couldn't reproduce that bug in my nightly build on Linux.)

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 6

Almost done. I think the only thing left to touch is crash reporting.

Comment 13

[Achwaq Khalid]

Holla Matt

here i just reported it https://bugzilla.mozilla.org/show_bug.cgi?id=1031606

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 7

A fully functioning patch!  Next steps: Rebase on m-c; fix mochitest failures.

Comment 15

[Achwaq Khalid]

looking forward to check it! Flashblock was really misbehaving recently

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: patch

I believe this patch is ready to review, although I still need to make a bunch of test changes (which I'll do as a separate patch).

Review notes:

* This patch uses "hg cp" for the code that was split into multiple files, so you can compare each new file against the original.

* The majority of code in browser-plugins.js was using non-prefixed identifiers (e.g. "browser" instead of "aBrowser") so I tried consistently follow that style in the new JSM file, and take some steps toward consistency in other code I added.

* I changed the name of "_setNotificationIcon" to "updateHiddenPluginUI" since it changes more than just the icon.

Comment 17

[Benjamin Smedberg]

Comment on attachment 8448888 [details] [diff] [review]
patch

I'm going to delegate this to gfritzsche.

Comment 18

[Georg Fritzsche [:gfritzsche]]

I will probably not get to this today. I will see that i get to looking over it this week though.

Comment 19

[Matt Brubeck (:mbrubeck)]

No rush; it will take me a while to get through the tests since I'm doing this in my spare time.  (Anyone want to help? ;)  And the underlying code is not churning much at all.  Sorry for the giant patch; I didn't find a good way to split it into smaller logical pieces.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8448888 [details] [diff] [review]
patch

This looks good to me, although I've really only looked at the IPC aspects. Right now I think the main challenge is to find a way to make this easier to review. It seems like it might be possible to split out the "core" features from things that are more optional. For example, an initial patch without plugin installation or crash reporting would probably be simpler.

I'm not sure if it would help in this case, but here's how I split up the patches for session restore. First I split out all the content process-specific code into a separate JSM but without using the message manager. It's a more straightforward refactoring that way, and it doesn't make the code async, so you generally don't have to fix up all the tests. Then I slowly introduced the message manager between the two pieces of code.

As a side note, you should be able to send principals over the message manager once bug 1032592 lands.

Comment 21

[Matt Brubeck (:mbrubeck)]

(In reply to Bill McCloskey (:billm) from comment #20)
> This looks good to me, although I've really only looked at the IPC aspects.
> Right now I think the main challenge is to find a way to make this easier to
> review. It seems like it might be possible to split out the "core" features
> from things that are more optional. For example, an initial patch without
> plugin installation or crash reporting would probably be simpler.

Attachment 8443747 [details] [diff] is basically this, except that enough stuff was broken in each of my intermediate "WIP" patches to make them kind of iffy candidates for review.

> I'm not sure if it would help in this case, but here's how I split up the
> patches for session restore. First I split out all the content
> process-specific code into a separate JSM but without using the message
> manager.

Unfortunately I didn't do anything nearly so sane in my actual development process, but I could try to retroactively split the big patch up into phases like this if it would help gfritze review it.

> As a side note, you should be able to send principals over the message
> manager once bug 1032592 lands.

Thanks!

Comment 22

[Georg Fritzsche [:gfritzsche]]

(In reply to Matt Brubeck (:mbrubeck) from comment #21)
> Unfortunately I didn't do anything nearly so sane in my actual development
> process, but I could try to retroactively split the big patch up into phases
> like this if it would help gfritze review it.

Splitting it up would be helpful, but please don't go for it if it takes too much effort.
The major blocker for me to start here are other high-priority bugs.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Note that I just touched browser-plugins.js in bug 1009760 which added crash-report support for a new type of plugin. It was a very simple change though: We still receive a PluginCrashed event for those plugins, however they don't have a related DOM object. So the target of the event is the `window`, and on that case we go directly to the "show infobar" path.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Matt, do you intend to break this into multiple patches or rebase it soon?
Or should i plan on making a first pass already?

Comment 26

[Matt Brubeck (:mbrubeck)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Matt, do you intend to break this into multiple patches or rebase it soon?
> Or should i plan on making a first pass already?

I don't have any current plans to split up the patch, though I can do it if it would significantly speed reviewing.  I'm happy to rebase if the patch no longer applies.  I do think it's ready for a first pass whenever you have bandwidth.  Thanks!

Comment 27

[Georg Fritzsche [:gfritzsche]]

Ok, i'll try to get to a first pass here next week - sorry for the long delay.

Comment 28

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8448888 [details] [diff] [review]
patch

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

A first pass here looks good. I didn't try to trace important paths yet, but it looks sensible.

::: browser/base/content/browser-plugins.js
@@ +70,5 @@
> +      return Services.scriptSecurityManager.getSystemPrincipal();
> +    }
> +    let uri = Services.io.newURI(remotePrincipal.URI.spec, null, null);
> +    return Services.scriptSecurityManager
> +       .getAppCodebasePrincipal(uri, remotePrincipal.appId, remotePrincipal.isInBrowserElement);

There should be more places that need to pass principals - does this have potential to be a general helper?

@@ +210,2 @@
>      if (!browser.missingPlugins)
>        browser.missingPlugins = new Map();

Hm, is this a COWS |browser|? Do we need to set on that?

::: browser/base/content/browser.js
@@ -241,5 @@
> -  // The PluginClickToPlay events are not fired when navigating using the
> -  // BF cache. |persisted| is true when the page is loaded from the
> -  // BF cache, so this code reshows the notification if necessary.
> -  if (persisted)
> -    gPluginHandler.reshowClickToPlayNotification();

Do we still need to worry about this? Is there an equivalent for this in this patch?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Hey mbrubeck,

Not sure if you're reading bugmail while on PTO, but was wondering if you wanted me to take the helm to drive this bug to completion seeing as how you've been doing it in your spare cycles while working on Servo.

Let me know!

All the best,

-Mike

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Tentatively nabbing until I hear back from mbrubeck...

Comment 31

[Mike Conley (:mconley) (:⚙️)]

This has bitrotted a bit, especially due to the work with GMP from bug 1009760. I'm cleaning this up right now...

Comment 34

[Matt Brubeck (:mbrubeck)]

(In reply to Mike Conley (:mconley) from comment #29)
> Not sure if you're reading bugmail while on PTO, but was wondering if you
> wanted me to take the helm to drive this bug to completion seeing as how
> you've been doing it in your spare cycles while working on Servo.

Yes, that's great. Thanks!

Comment 35

[Matt Brubeck (:mbrubeck)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> There should be more places that need to pass principals - does this have
> potential to be a general helper?

If I remember correctly, there is/was work in progress to add general principal-passing support to the platform, which should make the code here unnecessary.

> > -  // The PluginClickToPlay events are not fired when navigating using the
> > -  // BF cache. |persisted| is true when the page is loaded from the
> > -  // BF cache, so this code reshows the notification if necessary.
> > -  if (persisted)
> > -    gPluginHandler.reshowClickToPlayNotification();
> 
> Do we still need to worry about this? Is there an equivalent for this in
> this patch?

Yes, this logic has been moved to the "onPageShow" method in PluginContent.jsm.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Alright, I've got all tests passing now. I'll have a patch posted tomorrow once I clean things up a bit (and figure out a nice way of splitting up this massive change).

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP: Make click-to-play work in e10s

Comment 39

[Mike Conley (:mconley) (:⚙️)]

So with this test, all mochitest-browser tests under browser/base/content/test/plugins now pass.

I had to do some gross hacks to make some of the tests pass, and I'm going to try to refactor some of that a bit.

I'm also going to try to split this patch up into smaller chunks:

1) Changes to browser-plugins.js
2) Changes to browser-plugins.js after it was copied to PluginsContent.jsm, and other things to get the new PluginsContent.jsm module built.
3) Changes to other bits of browser
4) Changes to tests

I'll have patches 1, 2 and 3 up today, and can probably get some reviews kicked off. For 4, it might have to wait until next Tuesday.

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Attachment: Part 1: Update browser-plugins to use message passing for plugin events. r=?

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Attachment: Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=?

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Attachment: Part 3: Misc. browser fixups to deal with the new browser-plugins.js

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP: Make CTP tests work again. r=?

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Attachment: Difference between browser-plugins.js and PluginContent.jsm

This is the difference between the old browser-plugins.js and PluginContent.jsm.

This should help with reviewing PluginContent.jsm, which is really just the old browser-plugins.js, but heavily modified to send messages instead.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8481499 [details] [diff] [review]
Part 1: Update browser-plugins to use message passing for plugin events. r=?

I've broken down the overall change here by "subject". The patches I've posted are not atomic, unfortunately, so you might need to refer back and forth to the other ones to get the full pictures.

Basically, we've introduced a PluginContent.jsm that content scripts can instantiate which listens for plugin events, and CTP stuff, and sends messages up to the parent for it to react. The parent can also send messages back down to the child about things like a plugin was activated, or a context menu was clicked, etc.

So anything that has directly fiddled with content has been removed from browser-plugins, and put into PluginContent.jsm instead. The majority of _this_ patch is those removals, but also changes for sending and receiving messages from the child.

jaws, I've seen you reviewing patches in the past for browser-plugins stuff - let me know if you're not the right guy to review this, or if you're swamped with other things and I'll redirect.

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8481500 [details] [diff] [review]
Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=?

So this is the other half of the puzzle, where we introduce PluginContent.jsm which hears / reacts to Plugin events in content, and sends and receives messages from the parent process.

This is basically browser-plugins.js, but rejigged so that it's a JSM that returns objects that can do these reactions per browser. Naturally, this script gets full access to the content DOM, but no access to the browser chrome - it just sends messages to it's browser-plugins counterpart.

Giant green walls of text suck to review, and like I said - this is basically a rejigged browser-plugins.js, so I've also attached a diff between this new PluginContent.jsm and the original browser-plugins.js so you can see what exactly changed: https://bugzilla.mozilla.org/attachment.cgi?id=8481518

You'll probably want to look at that instead, and then just flag this patch r+ or r- depending on what you find.

Again, jaws, pointing this at you based on the log of browser-plugins.js, but if you're overloaded, just say the word.

Comment 47

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make most CTP tests work again. r=?

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make browser_CTP_crashreporting.js and browser_pluginCrashCommentAndURL.js tests work again. r=?

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make browser plugin tests work again. r=?

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8481499 [details] [diff] [review]
Part 1: Update browser-plugins to use message passing for plugin events. r=?

Hey Georg - jaws said you'd be the right guy to review these. Mind taking a look?

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8481500 [details] [diff] [review]
Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=?

Same with this one? (Note my comment to jaws in comment 46).

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make mozapps CTP tests work again. r=?

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make browser plugin tests work again. r=?

Comment 54

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8481518 [details] [diff] [review]
Difference between browser-plugins.js and PluginContent.jsm

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

This looks pretty sane. Thanks for this diff, that was helpful to get started on.
I still want to cross-read the two actual patches and peek into the test patches, but generally this looks good.

::: browser-plugins.js
@@ +28,2 @@
>    PREF_SESSION_PERSIST_MINUTES: "plugin.sessionPermissionNow.intervalInMinutes",
>    PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays",

These two are unused here.

@@ +601,5 @@
>      }
>    },
>  
> +  reshowClickToPlayNotification: function () {
> +    // Ignore events that aren't from the main document.

This doesn't match the following code.

@@ +633,5 @@
>    },
>  
>    /**
> +   * Manually activate the plugins that would have been automatically
> +   * activated.

Out of the original context this is not quite clear - can you clarify a bit?

@@ +917,5 @@
>      let isShowing = false;
>  
>      if (plugin) {
>        // If there's no plugin (an <object> or <embed> element), this call is
> +      // for a window-global plugin. I©n this case, there's no overlay to show.

Copy'n'paste massacre? ;)

@@ +939,5 @@
> +        });
> +        // Remove the notfication when the page is reloaded.
> +        doc.defaultView.top.addEventListener("unload", event => {
> +          this.hideNotificationBar("plugin-crashed");
> +        }, false);

Doesn't it make more sense to handle this kind of cleanup on the chrome side, in case of content crashes etc.?

@@ +945,3 @@
>      }
>  
> +    // TODO: Make this this.setupPluginOverlay?

What's up with this?

Comment 55

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8481500 [details] [diff] [review]
Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=?

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

Moving the comments over here.

::: browser/modules/PluginContent.jsm
@@ +25,5 @@
> +}
> +
> +PluginContent.prototype = {
> +  PREF_SESSION_PERSIST_MINUTES: "plugin.sessionPermissionNow.intervalInMinutes",
> +  PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays",

These two are unused here.

@@ +601,5 @@
> +    }
> +  },
> +
> +  reshowClickToPlayNotification: function () {
> +    // Ignore events that aren't from the main document.

This doesn't match the following code.

@@ +633,5 @@
> +  },
> +
> +  /**
> +   * Manually activate the plugins that would have been automatically
> +   * activated.

Out of the original context this is not quite clear - can you clarify a bit?

@@ +917,5 @@
> +    let isShowing = false;
> +
> +    if (plugin) {
> +      // If there's no plugin (an <object> or <embed> element), this call is
> +      // for a window-global plugin. I©n this case, there's no overlay to show.

Copy'n'paste massacre? ;)

@@ +939,5 @@
> +        });
> +        // Remove the notfication when the page is reloaded.
> +        doc.defaultView.top.addEventListener("unload", event => {
> +          this.hideNotificationBar("plugin-crashed");
> +        }, false);

Doesn't it make more sense to handle this kind of cleanup on the chrome side, in case of content crashes etc.?

@@ +943,5 @@
> +        }, false);
> +      }
> +    }
> +
> +    // TODO: Make this this.setupPluginOverlay?

What's up with this?

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Attachment: Part 1: Update browser-plugins to use message passing for plugin events. r=?

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Attachment: Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=?

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make browser plugin tests work again. r=?

Comment 59

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make mozapps CTP tests work again. r=?

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8485178 [details] [diff] [review]
Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=?

(In reply to Georg Fritzsche [:gfritzsche] from comment #55)
> Comment on attachment 8481500 [details] [diff] [review]
> Part 2: Introduce a new PluginContent.jsm that lets content scripts message
> plugin events to the parent. r=?
> 
> Review of attachment 8481500 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Moving the comments over here.
> 
> ::: browser/modules/PluginContent.jsm
> @@ +25,5 @@
> > +}
> > +
> > +PluginContent.prototype = {
> > +  PREF_SESSION_PERSIST_MINUTES: "plugin.sessionPermissionNow.intervalInMinutes",
> > +  PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays",
> 
> These two are unused here.
> 

Removed.

> @@ +601,5 @@
> > +    }
> > +  },
> > +
> > +  reshowClickToPlayNotification: function () {
> > +    // Ignore events that aren't from the main document.
> 
> This doesn't match the following code.
> 

Not sure what you mean. What doesn't match? The style? If so, which parts, exactly? It's going over my head. :)

> @@ +633,5 @@
> > +  },
> > +
> > +  /**
> > +   * Manually activate the plugins that would have been automatically
> > +   * activated.
> 
> Out of the original context this is not quite clear - can you clarify a bit?
> 

Did my best to clear that up.

> @@ +917,5 @@
> > +    let isShowing = false;
> > +
> > +    if (plugin) {
> > +      // If there's no plugin (an <object> or <embed> element), this call is
> > +      // for a window-global plugin. I©n this case, there's no overlay to show.
> 
> Copy'n'paste massacre? ;)
> 

Fixed!

> @@ +939,5 @@
> > +        });
> > +        // Remove the notfication when the page is reloaded.
> > +        doc.defaultView.top.addEventListener("unload", event => {
> > +          this.hideNotificationBar("plugin-crashed");
> > +        }, false);
> 
> Doesn't it make more sense to handle this kind of cleanup on the chrome
> side, in case of content crashes etc.?
> 

Yes, perhaps - although I think that might be better as a follow-up. Does that work with you?

> @@ +943,5 @@
> > +        }, false);
> > +      }
> > +    }
> > +
> > +    // TODO: Make this this.setupPluginOverlay?
> 
> What's up with this?

Removed the TODO _ the setupPluginOverlay didn't go quietly into a function hanging off of PluginContent, since it expected much in its closure.

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make browser plugin tests work again. r=?

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8485180 [details] [diff] [review]
Make mozapps CTP tests work again. r=?

A small review, I swear! :D

Comment 63

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8485180 [details] [diff] [review]
Make mozapps CTP tests work again. r=?

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

*mumble mumble ... whitepace differences... reviewboard...*

Comment 64

[Georg Fritzsche [:gfritzsche]]

Side note: i will probably get to the reviews here again in the next two days.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #60)
> > @@ +601,5 @@
> > > +    }
> > > +  },
> > > +
> > > +  reshowClickToPlayNotification: function () {
> > > +    // Ignore events that aren't from the main document.
> > 
> > This doesn't match the following code.
> > 
> 
> Not sure what you mean. What doesn't match? The style? If so, which parts,
> exactly? It's going over my head. :)

There are two places that use the same comment - i think this one is just a left-over, as the following code doesn't try to ignore any events.

> > @@ +939,5 @@
> > > +        });
> > > +        // Remove the notfication when the page is reloaded.
> > > +        doc.defaultView.top.addEventListener("unload", event => {
> > > +          this.hideNotificationBar("plugin-crashed");
> > > +        }, false);
> > 
> > Doesn't it make more sense to handle this kind of cleanup on the chrome
> > side, in case of content crashes etc.?
> > 
> 
> Yes, perhaps - although I think that might be better as a follow-up. Does
> that work with you?

Sounds good.

Comment 65

[Mike Conley (:mconley) (:⚙️)]

Gentle review ping?

Comment 66

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8485178 [details] [diff] [review]
Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=?

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

Side-note: Nice to see things getting cleaned up here in the process!

::: browser/modules/PluginContent.jsm
@@ +598,5 @@
> +    }
> +  },
> +
> +  reshowClickToPlayNotification: function () {
> +    // Ignore events that aren't from the main document.

This seems to be copy/paste left-over.

@@ +630,5 @@
> +  },
> +
> +  /**
> +   * Activate the plugins that the user has specified that they
> +   * wanted activated.

Nitpicking here: Just "Activate the plugins that the user has specified" is enough?

Comment 67

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8485177 [details] [diff] [review]
Part 1: Update browser-plugins to use message passing for plugin events. r=?

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

Phew, i don't see any real issues here.

::: browser/base/content/browser-plugins.js
@@ +10,5 @@
>    PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays",
>  
> +  init: function () {
> +    const mm = window.messageManager;
> +    mm.addMessageListener("PluginContent:ShowClickToPlayNotification", this);

Looking at this pattern in this file again, can't we just put the message names in a const array and avoid explicitly specifying them twice in init() and uninit()?

@@ +77,5 @@
> +          case "openHelpPage":
> +          case "openPluginUpdatePage":
> +            this[msg.data.name].apply(this);
> +            break;
> +        }

Add a warning here for unexpected names?

Comment 68

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8481507 [details] [diff] [review]
Part 3: Misc. browser fixups to deal with the new browser-plugins.js

Whoops, I never set a reviewer on this. It's short, I promise. :)

Comment 69

[Mike Conley (:mconley) (:⚙️)]

Attachment: Fix an intermittent orange in browser_pluginnotification.js. r=?

Comment 70

[Mike Conley (:mconley) (:⚙️)]

I'll note that I _might_ be seeing a new random orange showing up from this patch in browser_CTP_data_urls.js on Linux debug:

https://tbpl.mozilla.org/?tree=Try&rev=32fff51c2bf9

:/

Comment 71

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8488056 [details] [diff] [review]
Fix an intermittent orange in browser_pluginnotification.js. r=?

Super tiny patch.

Comment 73

[Chris Peterson [:cpeterson]]

Moving old M2 P1 bugs to M3.

Comment 74

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8481507 [details] [diff] [review]
Part 3: Misc. browser fixups to deal with the new browser-plugins.js

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

::: browser/base/content/content.js
@@ +492,5 @@
>  
>  ContentLinkHandler.init(this);
>  
> +// TODO: Load this lazily so the JSM is run only if a relevant event/message fires.
> +let pluginContent = new PluginContent(global);

Please file a follow-up here.

Comment 75

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8485203 [details] [diff] [review]
Make browser plugin tests work again. r=?

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

oh these tests would be so much tidier if they were Tasks

::: browser/base/content/test/plugins/browser_CTP_crashreporting.js
@@ +97,5 @@
>  
>    let tab = gBrowser.loadOneTab("about:blank", { inBackground: false });
>    gTestBrowser = gBrowser.getBrowserForTab(tab);
> +  let mm = gTestBrowser.messageManager;
> +  mm.loadFrameScript("data:,(" + frameScript.toString() + ")();", true);

true should probably be false, to not let this framescript load for other browsers.. although I think that it doesn't make a difference if you're using the mm from this specific browser. but for good measure, switch it to false

Comment 76

[Mike Conley (:mconley) (:⚙️)]

Thank you so much for the reviews everybody! I've addressed all of the feedback, and pushed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/5747034f2bcc

I'm filing the follow-ups that were requested now, and will link to them shortly.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc permafail.
https://hg.mozilla.org/integration/fx-team/rev/cba8fb1e0e37

https://tbpl.mozilla.org/php/getParsedLog.php?id=48113491&tree=Fx-Team

Comment 78

[Mike Conley (:mconley) (:⚙️)]

It's actually a random orange it seems (and one I thought I'd fixed) - I just happened to have a particularly unlucky roll of the dice.

I'm able to reproduce the failure running the test with --run-until-failure. I'm starting investigations.

Comment 79

[Mike Conley (:mconley) (:⚙️)]

My current theory here is that the click-to-play notification is sometimes not updated to deal with new content before the test grabs the notification and clicks a button in it.

So I think we need to make sure each test makes sure the notification gets dismissed by browsing to a different URI before proceeding.

Comment 80

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5747034f2bcc

Comment 81

[Mike Conley (:mconley) (:⚙️)]

This was backed out (see comment 77)

Comment 82

[Mike Conley (:mconley) (:⚙️)]

Attachment: Plug another frequent orange in browser_pluginnotifications.js

Comment 83

[Ryan VanderMeulen [:RyanVM]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #81)
> This was backed out (see comment 77)

Sorry about that.

Comment 84

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backout was merged over in https://hg.mozilla.org/mozilla-central/rev/cba8fb1e0e37 tonight. Sorry for the confusion.

Comment 85

[Mike Conley (:mconley) (:⚙️)]

The test failure that I'm trying to fix seems to be centered on a race involving the click to play PopupNotifications - specifically, it looks like Test 24a's PopupNotification can sometimes set permission to "allow now" for the plugin on the system principal, instead of the proper principal which has the origin http://127.0.0.1:8000.

Narrowing in on the race now.

Comment 86

[Mike Conley (:mconley) (:⚙️)]

Attachment: Put a stake through the heart of browser_pluginnotifications.js orange

Comment 87

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8490787 [details] [diff] [review]
Put a stake through the heart of browser_pluginnotifications.js orange

So I think I figured out the orange.

Test 23 in browser_pluginnotification.js works by testing what happens when a plugin that has been activated changes types. We should deactivate the plugin in that case.

The last part of that test ensures that the plugin is not activated, and then we move on to Test 24a.

Here's the rub though - once PluginContent.jsm detects that the plugin has changed types and been deactivated, it sends a message up to the parent saying, "We should probably show a Click-to-play notification for this thing", and passes along the principal of the page it's on (which is a chrome URI for Test 23). The principal for a chrome URI is the System Principal.

Test 24a however is pointed at a local HTTP server, and has a different principal. By the time that test starts running, the message that the framescript sent to display the Click-to-play notification gets received and shown, even though we're no longer viewing the page it was meant for. That's why activating the plugin there sets the permission for the wrong principal - it sets it for the system principal instead of the local HTTP server principal.

This patch adds a check at the top of showClickToPlay notification to ensure that the principal that is passed from the framescript matches the principal for the browser content, and bails if they're not the same.

With this patch, I've run the tests with --run-until-failure several times and not hit the failure once.

Comment 88

[:Felipe Gomes (needinfo for replies!)]

There was some rebasing needed which I'm not sure if I did it correctly, so I preferred to send it to try first:
https://tbpl.mozilla.org/?tree=Try&rev=26e0c1f7f812

If it comes out green I'll land this for mconley as he is on TRIBE :)

Comment 89

[Mike Conley (:mconley) (:⚙️)]

That push went total orange due to a small rebase snafu - re-pushed to try:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=db6db827b59a

Comment 90

[Mike Conley (:mconley) (:⚙️)]

Not all the builds are in but I'm pretty convinced.

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

Comment 91

[Mike Conley (:mconley) (:⚙️)]

So, lovely, things are coming in green, and it looks like my patch fixes the frequent orange for click-to-play on non-e10s! That's the good news.

The bad news is because I spent so much time focusing on getting the test to go green, I didn't check to see if click-to-play in e10s (which this patch is all about) actually still works. My fix actually breaks click-to-play for e10s mode because in that mode, we attempt to do a comparison of a nsIPrincipal with an nsIPrincipal CPOW, which is a no go.

However, because all of this foundational work is both large, reviewed, doesn't break non-e10s click-to-play and a dependency for some PFS clean-up work, I'm opting to keep the patch landed, but work on a follow-up patch to fix the nsIPrincipal stuff.

So stay tuned for that.

Comment 92

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Mike, I'm just curious: do you know how far we are from having click-to-play tests working in e10s? Even having just a few running would be pretty cool.

Comment 93

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bill McCloskey (:billm) from comment #92)
> Mike, I'm just curious: do you know how far we are from having click-to-play
> tests working in e10s? Even having just a few running would be pretty cool.

Having glanced at quite a few e10s tests, I think the main barrier is that the tests directly manipulate plugin elements (like activating them) by using contentDocument. We could go for a short-term win by using contentDocumentAsCPOW and then updating each test (I'd love to get these all updated to use add_task, to avoid the callback hell).

I can test the plausibility of that theory tomorrow.

Comment 94

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #93)
> Having glanced at quite a few e10s tests, I think the main barrier is that
> the tests directly manipulate plugin elements (like activating them) by
> using contentDocument. We could go for a short-term win by using
> contentDocumentAsCPOW and then updating each test (I'd love to get these all
> updated to use add_task, to avoid the callback hell).

browser-chrome tests are run in the context of an add-on. Therefore, all the add-on shims are in force. So contentDocument will just work inside test code. The same is true for the other usual ways of accessing content.

Comment 95

[Ryan VanderMeulen [:RyanVM]]

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

Comment 96

[Georg Fritzsche [:gfritzsche]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #91)
> The bad news is because I spent so much time focusing on getting the test to
> go green, I didn't check to see if click-to-play in e10s (which this patch
> is all about) actually still works. My fix actually breaks click-to-play for
> e10s mode because in that mode, we attempt to do a comparison of a
> nsIPrincipal with an nsIPrincipal CPOW, which is a no go.

Hm, so this means we're also missing some test-coverage if this patch went green?

Comment 97

[Mike Conley (:mconley) (:⚙️)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #96)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #91)
> > The bad news is because I spent so much time focusing on getting the test to
> > go green, I didn't check to see if click-to-play in e10s (which this patch
> > is all about) actually still works. My fix actually breaks click-to-play for
> > e10s mode because in that mode, we attempt to do a comparison of a
> > nsIPrincipal with an nsIPrincipal CPOW, which is a no go.
> 
> Hm, so this means we're also missing some test-coverage if this patch went
> green?

Indeed - almost none of the mochitest-browser tests are being run with e10s enabled. We're working on it (see bug 984139).

Comment 99

[Mike Conley (:mconley) (:⚙️)]

With bug 1069567 fixed, this CTP in e10s windows should be working now.

Comment 100

[Camelia Badau [:cbadau], Desktop QA]

Verified on latest Nightly 35.0a1 (buildID: 20140921030208): 
 - on Windows 7 64bit and Mac OSX 10.9.5 - it works fine
 - on Ubuntu 32bit: the click-to-play overlay is correctly displayed, but after that, even if I select to allow a plugin, the video/game doesn't play - I filled bug 1070981 for this.

Comment 101

[Mike Conley (:mconley) (:⚙️)]

Attachment: bug-899347.zip

Bugnotes: http://people.mozilla.org/~mconley2/bugnotes/bug-899347.html