1009148 - [e10s] Plugin not rendering on Mac after mouse input

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Flash plugin on Youtube don't appear to be rendering.

https://www.youtube.com/watch?v=4mTbPfI1BtA

Comment 1

[Benoit Girard (:BenWa)]

Actually the rendering appears to stop after the plugin has been clicked. Tentatively flagging as depending on bug 586656.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Hack to get input event working

Comment 3

[Benoit Girard (:BenWa)]

Ok with the above patch the rest of the problem is coming from GetPluginClipRect & FixUpPluginWindow which expect a root window to properly set the NPWindow clipRect.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

This fixes rendering and input event problems with e10s plugins.

Comment 5

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=4d823b2ac36d

Comment 6

[Steven Michaud [:smichaud] (Retired)]

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

   else if (inPaintState == ePluginPaintEnable)
   {
-    mPluginWindow->clipRect.bottom = mPluginWindow->clipRect.top + widgetClip.height;
-    mPluginWindow->clipRect.right  = mPluginWindow->clipRect.left + widgetClip.width; 
+    mPluginWindow->clipRect.bottom = mPluginWindow->clipRect.top + mPluginWindow->height;
+    mPluginWindow->clipRect.right  = mPluginWindow->clipRect.left + mPluginWindow->width;
   }

This is incorrect for non-content-process plugins.

   nsRect windowRect;
-  NS_NPAPI_CocoaWindowFrame(cocoaTopLevelWindow, windowRect);
+  if (cocoaTopLevelWindow) {
+    NS_NPAPI_CocoaWindowFrame(cocoaTopLevelWindow, windowRect);
+  }

And with windowRect set to its default of all zeroes, mPluginWindow->x and y will presumably not get set correctly below for content-process plugins, here:

  mPluginWindow->x = geckoScreenCoords.x/intScaleFactor - windowRect.x;
  mPluginWindow->y = geckoScreenCoords.y/intScaleFactor - windowRect.y;

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Steven Michaud from comment #6)
> Comment on attachment 8423359 [details] [diff] [review]
> patch
> 
>    else if (inPaintState == ePluginPaintEnable)
>    {
> -    mPluginWindow->clipRect.bottom = mPluginWindow->clipRect.top +
> widgetClip.height;
> -    mPluginWindow->clipRect.right  = mPluginWindow->clipRect.left +
> widgetClip.width; 
> +    mPluginWindow->clipRect.bottom = mPluginWindow->clipRect.top +
> mPluginWindow->height;
> +    mPluginWindow->clipRect.right  = mPluginWindow->clipRect.left +
> mPluginWindow->width;
>    }

AFAIK this was needed for QuickDraw which we no longer support. We now draw entirely with windowless plugin so passing a clipped window is not useful other then the special empty window when the plugin should stop rendering.

> 
> This is incorrect for non-content-process plugins.
> 
>    nsRect windowRect;
> -  NS_NPAPI_CocoaWindowFrame(cocoaTopLevelWindow, windowRect);
> +  if (cocoaTopLevelWindow) {
> +    NS_NPAPI_CocoaWindowFrame(cocoaTopLevelWindow, windowRect);
> +  }
> 
> And with windowRect set to its default of all zeroes, mPluginWindow->x and y
> will presumably not get set correctly below for content-process plugins,
> here:
> 
>   mPluginWindow->x = geckoScreenCoords.x/intScaleFactor - windowRect.x;
>   mPluginWindow->y = geckoScreenCoords.y/intScaleFactor - windowRect.y;

So AFAIK I couldn't detect any problems with this with and without content scale. Do you know how I can test this code?

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> AFAIK this was needed for QuickDraw which we no longer support. We
> now draw entirely with windowless plugin so passing a clipped window
> is not useful other then the special empty window when the plugin
> should stop rendering.

You're right that we no longer use NPWindow.clipRect ourselves.  But
we still send NPWindow to the plugin every time we call
NPP_SetWindow() in the plugin, so we need to keep NPWindow.clipRect
accurate, since the plugin may rely on it.

> So AFAIK I couldn't detect any problems with this with and without
> content scale. Do you know how I can test this code?

I don't know how to test this.  And in fact it my not be a problem
with content-process plugins.  But we should keep our eyes open to the
possibility that it will be a problem.

In any case this is a much less serious problem than the first one.

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Steven Michaud from comment #8)
> > AFAIK this was needed for QuickDraw which we no longer support. We
> > now draw entirely with windowless plugin so passing a clipped window
> > is not useful other then the special empty window when the plugin
> > should stop rendering.
> 
> You're right that we no longer use NPWindow.clipRect ourselves.  But
> we still send NPWindow to the plugin every time we call
> NPP_SetWindow() in the plugin, so we need to keep NPWindow.clipRect
> accurate, since the plugin may rely on it.

I'm not sure what the plugin would use it for. Or how it could hurt us by passing in everything.

> 
> > So AFAIK I couldn't detect any problems with this with and without
> > content scale. Do you know how I can test this code?
> 
> I don't know how to test this.  And in fact it my not be a problem
> with content-process plugins.  But we should keep our eyes open to the
> possibility that it will be a problem.
> 
> In any case this is a much less serious problem than the first one.

Alright well I'll if (e10s) just so we don't risk regressing the non-e10s code.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> I'm not sure what the plugin would use it for. Or how it could hurt us by passing in everything.

The problem with plugins is that they're black boxes -- we don't really know what they do, or what they rely on.  Furthermore NPWindow.clipRect has been part of the NPAPI for a long time, and for a long time we've gone to a lot of trouble to keep it accurate.  Unless we have a *very* good reason not to, we need to keep doing that.

And if we did decide to (say) drop support for NPWindow.clipRect, we'd need to announce this publicly well in advance -- so that plugin vendors would have a chance to learn about it, and a chance to complain if it was going to cause them trouble.

Comment 11

[Benoit Girard (:BenWa)]

I'm not advocating for dropping it. I'm saying that we're always going to provide a fullsize rendering context and we're always telling the plugin to draw the full size. Even if the plugin is part of the way occluded by the edge of the scroll part. We're still providing a NPAPI compliant implementation by asking it to paint fully.

And this is actually useful since we only support Async rendering with e10s. This means that even if the plugin is partly occluded we still want to paint the full plugin so that the user can scroll that portion of the plugin into view without having to block on the async painting.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> I'm saying that we're always going to provide a fullsize rendering
> context and we're always telling the plugin to draw the full size.

OK, this makes sense.  Now I understand a lot better.

But you should special-case your change to how NSWindow.clipRect is managed, so that it only effects content-process plugins.

And you *really* need to explain this in a comment :-)

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 14

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #5)
> https://tbpl.mozilla.org/?tree=Try&rev=4d823b2ac36d

This test run is still close enough to the current patch. Decided not to rerun on try.

Comment 15

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/505f38b0649b

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/505f38b0649b

Comment 17

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using a old Nightly build (2014-05-12), verified that flash videos work as expected on Firefox 32 beta 8 e10s and latest Nightly.