542656 - [OOPP] nsObjectFrame::PaintPlugin doesn't render properly for windowed plugins

Status: RESOLVED INCOMPLETE

(Core :: IPC, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It uses the PrintWindow API function which sends synchronous messages to the child process. If we want to support this then we need to use some IPC sharedsurface magic to get those bits from the child.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bug 542337 fixed the deadlock here, but I don't know if we should still do the IPC sharedsurface thing. We're probably only drawing a blank box into the preview, and we may want to draw the real plugin's contents. Morphing.

Comment 3

[Jim Mathies [:jimm]]

Attachment: wip

Untested wip, until we track down the cause of bug 313462.

Comment 4

[Jim Mathies [:jimm]]

Attachment: wip

We don't have to go over ipc for this, we can just grab the dc from the window and copy it.

Unfortunately the repainting problem for previews is a non-oopp specific problem. I've filed bug 557823 on this.

Comment 5

[Jim Mathies [:jimm]]

Attachment: fix

updated patch that works with the latest in bug 557823.

Comment 6

[Jim Mathies [:jimm]]

Attachment: oopp plugin print window v.1

Comment 7

[Jim Mathies [:jimm]]

Attachment: oopp plugin print window v.1

changes: release the dc, updated the method in plugin instance parent to better reflect what this is, and cleaned up the comments.

Roc, there's no need to review this if you're not happy with the patch in bug 557823.

Comment 8

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

Does this approach of BitBlitting directly from the plugin window actually work when the plugin window is a) clipped or b) covered by other content?

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #8)
> Does this approach of BitBlitting directly from the plugin window actually work
> when the plugin window is a) clipped or b) covered by other content?

Re A, Yes, we do a GetWindowRect is nsObjectFrame, and hand that to whatever we use to do rendering. So we always render the whole window. Some parts might be obscured due to scrolling, but those areas won't be displayed when the page is layed out and rendered by drawWindow.

One thing I tied was to get a window object for the HTML object element and render just the plugin. I didn't have any luck with that so I couldn't call drawWindow directly on the plugin element. For example:

var Cc = Components.classes;
var Ci = Components.interfaces;

var wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
var win = wm.getMostRecentWindow("navigator:browser");

var canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");

win = win.content.document.getElementById("flash");

canvas.width = win.innerWidth;
canvas.height = win.innerHeight;

canvas.getContext("2d").drawWindow(win, 0, 0, win.innerWidth, win.innerHeight, "rgb(255,255,255)");

var image = canvas.toDataURL();

drawWindow fails here due to win. Is there a trick I'm missing or is this impossible? In which case clipped areas shouldn't be an issue as we always render the whole content area.
 
As far as B goes, I can obscure the window with other windows on my desktop and everything still works. (For win7) I'll do some testing on XP to be sure.

Comment 10

[Jim Mathies [:jimm]]

Actually sometimes you get distortion because the painting is out of sync between the two processes. I really wanted to avoid going over IPC for this but maybe I'll have to.

Comment 11

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

Yeah, I would check XP because Vista/Win7 use the DWM so probably do have a backing buffer for the toplevel HWND.

I wonder if this is going to break when we add the ability to layer chrome elements over content (bug 130078).

Actually if you're snapshotting the toplevel content window HWND, how is this going to work with bug 130078, where there isn't one?

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #11)
> I wonder if this is going to break when we add the ability to layer chrome
> elements over content (bug 130078).

If the original PrntWindow work you did in nsObjectFrame continues to work with 130078, then what I'm cooking up now will work.

Comment 13

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

The PrintWindow code uses the HWND for the windowed plugin, which will still exist with bug 130078.

I guess this probably will too, except for the problem of chrome over content. So disregard the last paragraph of comment #11.

Comment 14

[Jim Mathies [:jimm]]

Attachment: oopp plugin print window v.2

Ok, here's a new rev. I need to do some release testing on this tomorrow before I seek review, but preliminary testing indicates it's working great.

Comment 15

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

I'm still pretty worried about overlapping windows on Windows XP, and chrome-over-content with bug 130078 fixed on any Windows version.

Comment 16

[Jim Mathies [:jimm]]

(In reply to comment #15)
> I'm still pretty worried about overlapping windows on Windows XP, and
> chrome-over-content with bug 130078 fixed on any Windows version.

Well, if those issues exist, they exist independent of this code and OOPP. Which patches would I apply to test? (Part 1 & Part 2?) Is there a test case posted someplace I could play with?

Comment 17

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

(In reply to comment #16)
> Well, if those issues exist, they exist independent of this code and OOPP.

No, because PrintWindow actually calls WM_PAINT on the plugin so it will be able to paint content that isn't actually visible on the screen. I expect that BitBlit from an HWND that's partially covered will not be able to get content that is clipped out or covered by another window.

> Which patches would I apply to test? (Part 1 & Part 2?)

Yes

> Is there a test case posted someplace I could play with?

I don't have one specifically for chrome over content.

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> (In reply to comment #16)
> > Well, if those issues exist, they exist independent of this code and OOPP.
> 
> No, because PrintWindow actually calls WM_PAINT on the plugin so it will be
> able to paint content that isn't actually visible on the screen. I expect that
> BitBlit from an HWND that's partially covered will not be able to get content
> that is clipped out or covered by another window.

Look at my follow-up patch! :) I'm now forwarding the PrintWindow call over ipc to the child. The existing code in nsObjectFrame actually works sans this patch, but calling PrintWindow on a window that contains child windows owned by another process's thread exhibits really poor performance. Our ipc code does a far better job of getting the call over, grabing the window via PrintWindow, and sending it back over ipc via a shared dib. 

I'll post an updated patch, I've updated the naming of methods, message constants, etc. to better define what's going on.  

> > Which patches would I apply to test? (Part 1 & Part 2?)
> 
> Yes
> 
> > Is there a test case posted someplace I could play with?
> 
> I don't have one specifically for chrome over content.

I''ll take this for a spin so I can work on taskbar preview issues with these changes. But I don't think this effects the plugin window capture work we're doing here for oopp plugins.

Comment 19

[Jim Mathies [:jimm]]

Attachment: oopp plugin print window v.2

Here's the latest rev.

Comment 20

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

Comment on attachment 439470 [details] [diff] [review]
oopp plugin print window v.2

+#ifdef OS_WIN

Where did OS_WIN come from? We've always used XP_WIN

It seems like we have an unnecessary copy or two here. The BitBlit in SnapshotWindow shouldn't really be needed. We could pass the Thebes context directly down, wrap a gfxWindowsSurface around the shared memory DIB and then copy directly from it to the output context.

Why do we have to fire a custom event here? Can't we just make some kind of method call into the plugin code?

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> (From update of attachment 439470 [details] [diff] [review])
> +#ifdef OS_WIN
> 
> Where did OS_WIN come from? We've always used XP_WIN

It's defined in the google chromium code. We've been using it in the dom/plugin code, it's the convention there. Honestly not sure why we did that.
  
> It seems like we have an unnecessary copy or two here. The BitBlit in
> SnapshotWindow shouldn't really be needed. We could pass the Thebes context
> directly down, wrap a gfxWindowsSurface around the shared memory DIB and then
> copy directly from it to the output context.

Hmm, I'll take a look. The context would pass through non-oopp plugins in the custom event. Is there any security risk there? (HDC's are pretty opaque, but the ctx isn't.)

> Why do we have to fire a custom event here? Can't we just make some kind of
> method call into the plugin code?

We have to work within the confines of the plugin api, nsObjectFrame can't communicate with PluginInstanceParent directly. PluginInstanceParent wraps/acts like a plugin instance.

Comment 22

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

(In reply to comment #21)
> It's defined in the google chromium code. We've been using it in the dom/plugin
> code, it's the convention there. Honestly not sure why we did that.

Yet we're using MOZ_ and XP_ stuff in the same file as well. Maybe minor, but suboptimal.

> Hmm, I'll take a look. The context would pass through non-oopp plugins in the
> custom event. Is there any security risk there? (HDC's are pretty opaque, but
> the ctx isn't.)

I don't see any security risk here.

> > Why do we have to fire a custom event here? Can't we just make some kind of
> > method call into the plugin code?
> 
> We have to work within the confines of the plugin api, nsObjectFrame can't
> communicate with PluginInstanceParent directly. PluginInstanceParent wraps/acts
> like a plugin instance.

This is PluginInstanceParent-specific code here anyway. Could we define a way to get a reference to the PluginInstanceParent, if the plugin is one, otherwise returning null, so we can then just make method calls on it?

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #22)
> (In reply to comment #21)
> > It's defined in the google chromium code. We've been using it in the dom/plugin
> > code, it's the convention there. Honestly not sure why we did that.
> 
> 
> This is PluginInstanceParent-specific code here anyway. Could we define a way
> to get a reference to the PluginInstanceParent, if the plugin is one, otherwise
> returning null, so we can then just make method calls on it?

Ah, good idea. I think we put an instance pointer in the window props I can grab. That would be better too, no chance of a false positive from the event. I'll take a look.

Comment 24

[Jim Mathies [:jimm]]

Attachment: patch v.3

Addressing comments. This however introduces a new problem. Without the use of PrintWindow, the lazy race I commented about in bug 557823 doesn't occur for oopp plugins, and I don't see many ways around it. So while this is the proper way to get the thumbnail and avoids the performance problems of cross process PrintWindow calls, it re-introduces bug 557823 for oopp plugins that don't use wm_paint to trigger painting. (video sites are the most commonly effected.)

Comment 26

[Benjamin Smedberg]

bent, what is this bug about, and is it still valuable? Is it about printing?

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This was about pulgins showing up as black boxes in aero peek previews. It originally deadlocked, but then I think that got fixed some other way.

I'm not sure if our printing code uses the same API that the aero peek stuff used.

Comment 28

[Benjamin Smedberg]

ok, I'm not going to track it then