Closed Bug 1009148 Opened 5 years ago Closed 5 years ago

[e10s] Plugin not rendering on Mac after mouse input

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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

https://www.youtube.com/watch?v=4mTbPfI1BtA
Actually the rendering appears to stop after the plugin has been clicked. Tentatively flagging as depending on bug 586656.
Depends on: 586656
Summary: [e10s] Plugin not rendering on Mac → [e10s] Plugin not rendering on Mac after mouse input
Attached file Hack to get input event working (obsolete) —
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.
Attached patch patch (obsolete) — Splinter Review
This fixes rendering and input event problems with e10s plugins.
Assignee: nobody → bgirard
Attachment #8421292 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8423359 - Flags: review?(smichaud)
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;
Attachment #8423359 - Flags: review?(smichaud) → review-
(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?
> 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.
(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.
> 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.
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.
> 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 :-)
Attached patch patchSplinter Review
Attachment #8423359 - Attachment is obsolete: true
Attachment #8424110 - Flags: review?(smichaud)
Attachment #8424110 - Flags: review?(smichaud) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/505f38b0649b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.