Make click-to-play work in e10s

VERIFIED FIXED in Firefox 35

Status

()

defect
P1
normal
VERIFIED FIXED
6 years ago
Last year

People

(Reporter: keeler, Assigned: mconley)

Tracking

(Depends on 3 bugs, Blocks 1 bug)

unspecified
Firefox 35
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(e10sm3+)

Details

Attachments

(9 attachments, 19 obsolete attachments)

6.30 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
48.56 KB, patch
gfritzsche
: feedback+
Details | Diff | Splinter Review
56.03 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
37.64 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
7.42 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
37.36 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
2.17 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
1.53 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
621.02 KB, application/zip
Details
No description provided.
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
Duplicate of this bug: 1011924
The fix was reverted and now it isn't working again.
So e10s-m2 will ship with this not fixed?
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.)
Status: NEW → ASSIGNED
Assignee: nobody → mbrubeck
Posted patch WIP (obsolete) — Splinter Review
This is patch is about 1/3 complete and not very functional yet, but it's getting there.
Posted patch WIP 2 (obsolete) — Splinter Review
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.
Attachment #8443136 - Attachment is obsolete: true
Posted patch WIP 3 (obsolete) — Splinter Review
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.
Attachment #8443575 - Attachment is obsolete: true
Posted patch WIP 4 (obsolete) — Splinter Review
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.
Attachment #8443662 - Attachment is obsolete: true
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
Posted patch WIP 5 (obsolete) — Splinter Review
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.)
Attachment #8443747 - Attachment is obsolete: true
Posted patch WIP 6 (obsolete) — Splinter Review
Almost done. I think the only thing left to touch is crash reporting.
Attachment #8445495 - Attachment is obsolete: true
Holla Matt

here i just reported it https://bugzilla.mozilla.org/show_bug.cgi?id=1031606
Posted patch WIP 7 (obsolete) — Splinter Review
A fully functioning patch!  Next steps: Rebase on m-c; fix mochitest failures.
Attachment #8447507 - Attachment is obsolete: true
looking forward to check it! Flashblock was really misbehaving recently
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #8448345 - Attachment is obsolete: true
Attachment #8448888 - Flags: review?(benjamin)
Attachment #8448888 - Flags: feedback?(wmccloskey)
Comment on attachment 8448888 [details] [diff] [review]
patch

I'm going to delegate this to gfritzsche.
Attachment #8448888 - Flags: review?(benjamin) → review?(georg.fritzsche)
I will probably not get to this today. I will see that i get to looking over it this week though.
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 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.
Attachment #8448888 - Flags: feedback?(wmccloskey) → feedback+
(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!
Depends on: 1032592
(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.
Priority: -- → P1
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.
Matt, do you intend to break this into multiple patches or rebase it soon?
Or should i plan on making a first pass already?
Duplicate of this bug: 1047671
(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!
Ok, i'll try to get to a first pass here next week - sorry for the long delay.
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?
Attachment #8448888 - Flags: feedback+
Attachment #8448888 - Flags: review?(georg.fritzsche)
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
Flags: needinfo?(mbrubeck)
Tentatively nabbing until I hear back from mbrubeck...
Assignee: mbrubeck → mconley
This has bitrotted a bit, especially due to the work with GMP from bug 1009760. I'm cleaning this up right now...
(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!
Flags: needinfo?(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.
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).
Attachment #8448888 - Attachment is obsolete: true
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.
Attachment #8481323 - Attachment is obsolete: true
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 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.
Attachment #8481499 - Flags: review?(jaws)
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.
Attachment #8481500 - Flags: review?(jaws)
Attachment #8481508 - Attachment is obsolete: true
Attachment #8483042 - Attachment is obsolete: true
Attachment #8483044 - Attachment is obsolete: true
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?
Attachment #8481499 - Flags: review?(jaws) → review?(georg.fritzsche)
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).
Attachment #8481500 - Flags: review?(jaws) → review?(georg.fritzsche)
Attachment #8483048 - Attachment is obsolete: true
Flags: firefox-backlog+
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?
Attachment #8481518 - Flags: feedback+
Flags: firefox-backlog+
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?
Attachment #8481500 - Flags: review?(georg.fritzsche)
Attachment #8481499 - Attachment is obsolete: true
Attachment #8481499 - Flags: review?(georg.fritzsche)
Attachment #8481500 - Attachment is obsolete: true
Attachment #8483662 - Attachment is obsolete: true
Attachment #8483664 - Attachment is obsolete: true
Attachment #8485177 - Flags: review?(georg.fritzsche)
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.
Attachment #8485178 - Flags: review?(georg.fritzsche)
Attachment #8485179 - Attachment is obsolete: true
Attachment #8485203 - Flags: review?(georg.fritzsche)
Comment on attachment 8485180 [details] [diff] [review]
Make mozapps CTP tests work again. r=?

A small review, I swear! :D
Attachment #8485180 - Flags: review?(bmcbride)
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...*
Attachment #8485180 - Flags: review?(bmcbride) → review+
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.
Flags: firefox-backlog+
Gentle review ping?
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?
Attachment #8485178 - Flags: review?(georg.fritzsche) → review+
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
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?
Attachment #8485177 - Flags: review?(georg.fritzsche) → review+
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. :)
Attachment #8481507 - Flags: review?(georg.fritzsche)
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 on attachment 8488056 [details] [diff] [review]
Fix an intermittent orange in browser_pluginnotification.js. r=?

Super tiny patch.
Attachment #8488056 - Flags: review?(georg.fritzsche)
Duplicate of this bug: 1066208
Blocks: 1066401
Moving old M2 P1 bugs to M3.
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.
Attachment #8481507 - Flags: review?(georg.fritzsche) → review+
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
Attachment #8485203 - Flags: review?(georg.fritzsche) → review+
Attachment #8488056 - Flags: review?(georg.fritzsche) → review+
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.
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.
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.
https://hg.mozilla.org/mozilla-central/rev/5747034f2bcc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
This was backed out (see comment 77)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8489731 - Flags: review?(wmccloskey)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #81)
> This was backed out (see comment 77)

Sorry about that.
Target Milestone: Firefox 35 → ---
Backout was merged over in https://hg.mozilla.org/mozilla-central/rev/cba8fb1e0e37 tonight. Sorry for the confusion.
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.
Attachment #8489731 - Attachment is obsolete: true
Attachment #8489731 - Flags: review?(wmccloskey)
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.
Attachment #8490787 - Flags: review?(georg.fritzsche)
Attachment #8490787 - Flags: review?(georg.fritzsche) → review+
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 :)
That push went total orange due to a small rebase snafu - re-pushed to try:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=db6db827b59a
Not all the builds are in but I'm pretty convinced.

remote:   https://hg.mozilla.org/integration/fx-team/rev/c07c6f96d613
Whiteboard: [fixed-in-fx-team]
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.
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][keep-open]
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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/c07c6f96d613
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team][keep-open] → [keep-open]
Depends on: 1069075
Keywords: leave-open
Whiteboard: [keep-open]
(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?
(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).
Duplicate of this bug: 1066464
With bug 1069567 fixed, this CTP in e10s windows should be working now.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Iteration: --- → 35.2
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
QA Contact: bogdan.maris → camelia.badau
No longer depends on: 1070981
You need to log in before you can comment on or make changes to this bug.