684618 - Deny requests for DOM full-screen when windowed plugins are present

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We need to disable windowed plugins when we enter DOM full-screen mode (see bug 545812 for DOM full-screen mode). We exit DOM full-screen mode when a restricted key is pressed. However windowed plugins exist outside our event handling chain, so when in DOM full-screen mode they will still receive key events. This can then be used to phish logins etc.

We can't enable full-screen until this bug is fixed.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Change of plan. We'll deny requests for full-screen mode if any content document in the subtree being made full-screen contains a windowed plugin. We also need to exit DOM full-screen mode when a windowed plugin is created in a content document which is in DOM full-screen mode.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'll base this on top of bug 90268, to reduce conflicts and make my life easier...

Comment 3

[Benjamin Smedberg]

Could we just use keyboard hooking to catch the relevant keystrokes?

Comment 4

[Brian R. Bondy [:bbondy]]

Yes I think we could use a hook to detect these keys and then force exit DOM full-screen mode.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can on Windows, maybe. Can we do it for X11? Can we do it reliably for any child widgets the plugin widget may have? Can we make sure we get all relevant key events? Will it work for IME users?

We thought about trying to block key events to windowed plugins but it seemed like it might be a bit fragile.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think also getting it to work for out-of-process plugins was going to be a little bit painful.

Comment 7

[Karl Tomlinson (:karlt)]

On X11 at least, exiting DOM full-screen mode when a plugin takes focus may be simpler than filtering plugin key events, and perhaps this could be easier than detecting the presence of windowed plugins.  Doing so could lead to a surprise exit from full-screen when clicking on a plugin, though.

I think it would be possible to filter key events for X11 XEmbed plugins, or it may be possible to deny a plugin focus request.  Solutions here would rely a bit on the internal mechanisms of GtkSocket.

Xt plugin focus is complex (and doesn't work well).  We may be able to do a key event filter for Xt plugins, but that could be even more complex.  They are not prevalent enough to justify doing anything beyond a complete block of focus or key events (whichever is easier).

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: deny requests for full-screen when windowed plugins present

Deny requests for full-screen in documents containing windowed plugins. Exit full-screen when widget created for windowed plugin.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #8)
> Created attachment 559930 [details] [diff] [review]
> Patch: deny requests for full-screen when windowed plugins present

I should also mention, this patch is based on top of the two patches in bug 90268.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559930 [details] [diff] [review]
Patch: deny requests for full-screen when windowed plugins present

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

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +3423,5 @@
> +// Returns PR_TRUE if the doctree rooted at aDoc contains any plugins which
> +// we don't control event dispatch for. i.e. do any plugins in this doc tree
> +// receive key events outside of our control?
> +static PRBool
> +HasPluginsOutsideEventDispatch(nsIDocument* aDoc)

Move this to nsContentUtils I think. It's a bit unfortunate to have this code in nsGenericHTMLElement.

"OutsideEventDispatch" seems a bit unclear to me. Call it HasPluginWithUncontrolledEventDispatch, maybe?

::: content/html/content/test/file_fullscreen-plugins.html
@@ +83,5 @@
> +
> +var fullScreenChangeCount = 0;
> +
> +function macFullScreenChange(event) {
> +  switch (fullScreenChangeCount) {

I think we need to test that adding a windowed plugin does not exit full-screen mode on Mac.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +2928,5 @@
>    mInstance->IsWindowless(&windowless);
> +  nsIDocument *doc = mContent ? mContent->GetOwnerDoc() : nsnull;
> +  if (!windowless && doc && doc->IsFullScreenDoc()) {
> +    NS_DispatchToCurrentThread(
> +      NS_NewRunnableMethod(doc, &nsIDocument::CancelFullScreen));

Does this work correctly on Mac? Without checking the code I'm not sure that IsWindowless always returns true on Mac. Hence my request for the test above.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Comment on attachment 559930 [details] [diff] [review]
> Patch: deny requests for full-screen when windowed plugins present
> 
>[...]
> ::: dom/plugins/base/nsPluginInstanceOwner.cpp
> @@ +2928,5 @@
> >    mInstance->IsWindowless(&windowless);
> > +  nsIDocument *doc = mContent ? mContent->GetOwnerDoc() : nsnull;
> > +  if (!windowless && doc && doc->IsFullScreenDoc()) {
> > +    NS_DispatchToCurrentThread(
> > +      NS_NewRunnableMethod(doc, &nsIDocument::CancelFullScreen));
> 
> Does this work correctly on Mac? Without checking the code I'm not sure that
> IsWindowless always returns true on Mac. Hence my request for the test above.

Oops, IsWindowless() appears to always report false on Mac. I'll just ifdef out this check on Mac, and add a test to ensure adding a windowed plugin doesn't exit full-screen mode on Mac.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

Updated with review comments.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 560100 [details] [diff] [review]
Patch v2

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

::: content/base/src/nsContentUtils.cpp
@@ +5751,5 @@
> +static void
> +CheckForWindowedPlugins(nsIContent* aContent, void* aResult)
> +{
> +  if (!aContent->IsInDoc()) {
> +    return;

I don't think you should do this check. What if (post-bug-90268) a page creates a plugin element, doesn't put it in the document, goes full-screen, then adds it to the document? I think you end up with a windowed plugin in the full-screen document able to receive key events. Better add a test for that!

@@ +5793,5 @@
> +nsContentUtils::HasPluginWithUncontrolledEventDispatch(nsIDocument* aDoc)
> +{
> +#ifdef XP_MACOSX
> +	// We control dispatch to all mac plugins.
> +	return PR_FALSE;

Fix indent.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 560100 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 560100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsContentUtils.cpp
> @@ +5751,5 @@
> > +static void
> > +CheckForWindowedPlugins(nsIContent* aContent, void* aResult)
> > +{
> > +  if (!aContent->IsInDoc()) {
> > +    return;
> 
> I don't think you should do this check. What if (post-bug-90268) a page
> creates a plugin element, doesn't put it in the document, goes full-screen,
> then adds it to the document? I think you end up with a windowed plugin in
> the full-screen document able to receive key events. Better add a test for
> that!

CreateWidget is called every time we add a plugin to a document, so the check in CreateWidget will cause us to exit full-screen in the above case.

I already test this case; in the non-mac case I load a windowed plugin in the document, remove it from document, go full-screen, then re-add it and verify we exited full-screen. 

I'm not testing that adding a windowed plugin which has never been added to a document before and which was created before going full-screen causes us to exit full-screen mode, I can add a test case for that.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #14)
> CreateWidget is called every time we add a plugin to a document,

I don't think that's true, post-bug90268.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

* Add a check in nsHTMLSharedObjectElement::BindToTree() rather removing the IsInDoc() check suggested in comment 13, as we can't rely on plugin containers being GC'd before the full-screen request comes in as discussed.
* Also added extra test as I described in comment 14.

Comment 17

[Jesse Ruderman]

How often will this restriction kick in? (Is it only for plugins in the same tab that's going full-screen? Which popular plugins are 'windowed' nowadays?)

Comment 18

[Benjamin Smedberg]

It is only the same tab. Most of the popular plugins can be windowed, including Flash (unless it has wmode="transparent" or wmode="opaque") and silverlight and Acrobat.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4 - not based on bug 90268

It looks like this patch pretty much works as-is when rebased without bug 90268. I had to change the test to ensure it waits for the windowed plugin in the test's subdocument to load before starting, but other than that, this patch is pretty much the same.

Re-requesting review just in case the approach I use here isn't supposed to work without 90268.

Looks greenish on Try: https://tbpl.mozilla.org/?tree=Try&rev=55a7472f6e62

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e375d0cf797

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2e375d0cf797