Here's how I think this can work - each plugin instance has an "outer native window" owned by the UI process U, and an "inner native window" owned by the plugin subprocess P - when a content process C reflows, and the frame of windowed instance I of P is created/resized etc * C makes a synchronous request to U to create/change the outer native window of I * C makes a synchronous update to P on behalf of I's inner window * the new geometry of I is forward along with the rest of the layers transaction between C and U A question here is whether the outer-window update in U can be deferred until the next paint in U (obviously creating a new window can't be deferred). If we can defer, I think we can get flickerless and jitterless painting across multiple processes. If we can't defer, oh well. Another issue is deadlocking. The problem is this scenario plugin --sync--> content --sync--> UI --SendMessage()--> plugin where "sync" is a synchronous IPC message. It's not horrible because the current WindowsSpinLoop code ought to take care of unbreaking the cycle, it just means we can't deep-six that code until there are no more sync content-->UI messages.

Chatted with cjones: I think that all of the positioning for both windowed and windowless plugins can be handled through layer tree updates, so that content C never has to directly tell the plugin instance I of position/size updates. Then the chrome process can move the native widget as it applies the new layer tree.

Is this still valid, esp. in the context of OMTC? Does the latter impact plugins at all?

John, is this already resolved or duped by the ongoing e10s work?

Yeah 923746 is a dupe of this. Duping that one since there's useful discussion here.

Moving old M2 P1 bugs to M3.

yoink!

- fixes build bustage related to SendSetFocus, which wasn't defined.

Generally this has flash positioned and painting, and you can interact with it. a few open issues I'm still investigating - 1a) PluginInstanceParent subclass [1] - this no longer works because the HWND we're trying to subclass is from another process. I don't think we want a subclass here at all, the content process should act as a simple middle man forwarding messages. Avoiding native hooks in the content process also avoids a lot of deadlock issues. There isn't a lot happening in the subclass procedure itself so I'm going to shunt this code, hopefully without breaking PluginInstanceParent. At some point we should be able to remove this code. 1b) PluginInstanceParent access in widget on Windows [2] - yay for hacks. Currently I'm seeing some problems with paint invalidation on youtube videos, and I'm also seeing a lot console spew related to my log statement here that this WM_PAINT on the top level parent is getting repeatedly dumped. PluginInstanceParent is now in the content process and this runs in chrome. 2) widget visibility - since the chrome parent isn't involved with content layout, it doesn't get hidden automatically when we tab switch. As a result plugin windows tend to accumulate from all tabs on the main window and remain visible.. which makes surfing the web with a lot of tab rather interesting. :D 3) nsPluginNativeWindowXXX [3] - the logic here still needs to be ported to this new architecture. For Windows, I'm going to look at removing as much of this as possible. For example we no longer need the flash throttling nsPluginNativeWindowWin provides, and I seriously doubt we need that subclass. Also not convinced we need the windowing hooks, I'd prefer that anything like this take place in PluginInstanceChild only. If we do need those in chrome, it shouldn't be too hard to hook them up in PluginWidgetParent. Linux absolutely needs this [4] for getting its rendering set up, but I've been asked to concentrate on win first, so we'll see how far we get. Also I'm pretty sure I can just ignore nsPluginNativeWindowQt. [1] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceParent.cpp#1628 [2] http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindowGfx.cpp#187 [3] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/ [4] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginNativeWindowGtk.cpp

Josh, curious how compatible you think these changes are with the osx plugin work?

There shouldn't be much of a problem, the mac stuff is focused on the widget and nsPluginInstanceOwner code. This commit shows the entirety of the work so far: https://github.com/bdaehlie/gecko-dev/commit/ace837bbbb5fd560920c2a382683f41a8d02d7bb

merged to mc tip plus the latest from bug 641685.

- visibility problems addresses when switching tabs - this also has the temp crash fix for bug e10s-plugin-ipc.

I'll be filing a follow up on this.

Hate to say it benjamin but it looks like I'll have to flag you on all of these.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47d4b23c6276

fyi, I have an updated patch set merged to what landed in the parent bug on mc building which has a few updates. Will post in about thirty minutes, got caught in a clobber.

In the last rev of this I was experimenting with making some calls async, like Invalidate. That was causing some rendering problems so I've reverted that change.

- the right patch this time.

- remove white space

I'll be rolling this in with part 2 at the next opportunity. When TabChild is torn down, there are few calls that are made by the plugin code as the widget is being destroyed on the content side. One of those calls is Move, which is made from nsPluginFrame::SetInstanceOwner(inst) where 'inst' is null. Over in the Move handler in PluginWidgetParent, we try to grab the parent widget from the tab, which fails. That was early returning false for the RecvMove handler, which I guess ipdl considers terminal so it aborts the content process, crashing all tabs.

- rolling part 2.5 into part 2 - fixed a linux build bug I introduced in the last rev in some logging macros

This update fixes a missing include header on linux when built against mc tip. I've built this on win and mac, pushed to try for a build on linux.

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d0f8d8b09b7 android bustage above was a missed brace, follow up with a fix - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f5bf61c5dea

Comment on attachment 8514251 [details] [diff] [review] part 1 - ipdl changes Why is PPluginWidget in the PBrowser hierarchy? Why does the plugin have a nsIWidget at all? Is that required by some code within Mozilla? I thought that the positioning/clipping of the plugin window was going to be managed by the compositor, to avoid as much disconnect as possible. I assumed that meant that methods like Show/Invalidate/Resize/Move/SetWindowClipRegion would be handled as part of compositing and not in an IPDL connection. GetNativeData is not a well-named API, if it's really just about NS_NATIVE_PLUGIN_PORT. The IPDL method name should be GetPluginWindget or somesuch.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #40) > Comment on attachment 8514251 [details] [diff] [review] > part 1 - ipdl changes > > Why is PPluginWidget in the PBrowser hierarchy? It could work in either I think. I like having it on the tab so that we have easy access to tab properties like offset from the main window. Accessing the right tab from the top level protocol might be tricky. I don't see any harm in having it on PBrowser, do you? > I thought that the positioning/clipping of the plugin window was going to be > managed by the compositor, to avoid as much disconnect as possible. I plan to look at doing this, but I didn't have the time to put it together for this bug. I think some feedback from layout folks might be useful here. > GetNativeData is not a well-named API, if it's really just about > NS_NATIVE_PLUGIN_PORT. The IPDL method name should be GetPluginWindget or > somesuch. Every call in this ipdl matches an nsIWidget method in name. I'd like to keep it this wayas long as we continue to use nsIWidget as the bridge. If you're familiar with the nsIWidget interface these calls should be very recognizable, which is a good quality to have imo.

Comment on attachment 8514251 [details] [diff] [review] part 1 - ipdl changes These are a couple days out of date, will refresh and repost. Also, I'll schedule a follow up from this weeks meeting so we can go over what we've found related to hang issues.

- better docs

Comment on attachment 8519194 [details] [diff] [review] part 5 - use remote widgets in content - blassey helped test this code on various platforms.

Comment on attachment 8519202 [details] [diff] [review] part 7 - Gtk socket widget support for chrome side As far as I can tell, we always call CreateXEmbedWindow with oop [1]. All other paths here are irrelevant. PluginWidgetParent emulates this behavior. [1] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginNativeWindowGtk.cpp#127

- fixes a build error on android

Comment on attachment 8519192 [details] [diff] [review] part 4 - base widget support for remote controlled widgets Review of attachment 8519192 [details] [diff] [review]: ----------------------------------------------------------------- 8 lines of context please. The commit message doesn't look right. Did you consider not having a full-fledged proxy nsIWidget on the content side, and just having nsPluginFrame send messages to the chrome process directly? Having a full-fledged nsIWidget raises questions about how much of the nsIWidget interface we're actually going to support for these proxy widgets. I assume we only want to support remote-controlled widgets for eWindowType_plugin. Might it make sense to introduce a new eWindowType_pluginRemoteController instead of a boolean mRemote which could conceivably be set orthogonally to the window type? ::: widget/nsIWidget.h @@ +1267,4 @@ > nsWindowType WindowType() { return mWindowType; } > > /** > + * Returns true if this widget is controlled remotely by a content process. Please explain in more detail what this means. @@ +2154,4 @@ > // When Destroy() is called, the sub class should set this true. > bool mOnDestroyCalled; > nsWindowType mWindowType; > + bool mRemote; Put this next to mOnDestroyCalled. ::: widget/nsWidgetInitData.h @@ +130,5 @@ > // such windows. > bool mRequireOffMainThreadCompositing; > + // Indicates this widget is controlled remotely by a content process. Used for > + // windowed plugins on win and linux only. > + bool mRemote; This is a bit confusing because we have two kinds of remote widgets, both called "mRemote": -- A widget which is a proxy to remotely control some chrome-process widget -- That chrome-process widget I think we should tighten up the terminology with separate terms for the two sides, e.g. "remote-controller widget" and "remote-controlled widget".

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59) > Comment on attachment 8519192 [details] [diff] [review] > part 4 - base widget support for remote controlled widgets > > Review of attachment 8519192 [details] [diff] [review]: > ----------------------------------------------------------------- > > 8 lines of context please. > > The commit message doesn't look right. > > Did you consider not having a full-fledged proxy nsIWidget on the content > side, and just having nsPluginFrame send messages to the chrome process > directly? Having a full-fledged nsIWidget raises questions about how much of > the nsIWidget interface we're actually going to support for these proxy > widgets. I liked the idea of using nsIWidget with a proxy hidden behind it, but I suppose we could dconnect things up directly. I'm not sure though, this would require updates to both nsPluginFrame and nsPluginInstanceOwner, which I think will be a bit confusing with dual support for both e10s and non-e10s. I'm not seeing the advantage here? > > I assume we only want to support remote-controlled widgets for > eWindowType_plugin. Might it make sense to introduce a new > eWindowType_pluginRemoteController instead of a boolean mRemote which could > conceivably be set orthogonally to the window type? Sounds good, will update.

- combining the two main widget patches into one, so they can be reviewed together.

Comment on attachment 8519204 [details] [diff] [review] disable pip focus related code Review of attachment 8519204 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/ipc/PluginInstanceParent.cpp @@ +1854,4 @@ > // focus. We forward the event down to widget so the dom/focus manager can > // be updated. > #if defined(OS_WIN) > + // XXX This needs to go to PuppetWidget. bug ??? Is there a bug # for this?

(In reply to Aaron Klotz [:aklotz] from comment #64) > Comment on attachment 8519204 [details] [diff] [review] > part 8 - disable pip focus related code > > Review of attachment 8519204 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/plugins/ipc/PluginInstanceParent.cpp > @@ +1854,4 @@ > > // focus. We forward the event down to widget so the dom/focus manager can > > // be updated. > > #if defined(OS_WIN) > > + // XXX This needs to go to PuppetWidget. bug ??? > > Is there a bug # for this? Yes, bug 1095761.

(In reply to Jim Mathies [:jimm] from comment #60) > I liked the idea of using nsIWidget with a proxy hidden behind it, but I > suppose we could dconnect things up directly. I'm not sure though, this > would require updates to both nsPluginFrame and nsPluginInstanceOwner, which > I think will be a bit confusing with dual support for both e10s and > non-e10s. I'm not seeing the advantage here? nsIWidget is quite a complex API and we're not going to support all of it for remote-controller widgets (I assume). So that adds pitfalls for people working on this code in the future. But I'm OK with doing this.

Comment on attachment 8520038 [details] [diff] [review] part 2 - implement PPluginWidget plus widget updates Review of attachment 8520038 [details] [diff] [review]: ----------------------------------------------------------------- 8 lines of context, please. Also we can probably split this patch into "widget changes" and "everything else", at least, which I think would be helpful. ::: widget/nsWidgetInitData.h @@ +21,5 @@ > + // desktop (has no border)) > + eWindowType_invisible, // windows that are invisible or offscreen > + eWindowType_plugin, // plugin window > + eWindowType_plugin_parent, // chrome side native plugin window (e10s) > + eWindowType_plugin_child, // content side PluginWidgetProxy plugin window (e10s) I don't think parent/child are good terms here. We also have parent/child relationships for widgets, which are totally different.

- except two calls to the new IsPlugin method on nsIWidget in nsViewManager and nsLayoutUtils, stripped out all non-widget code and moved that to other patches. - renamed plugin types to 'eWindowType_plugin_ipc_chrome' and 'eWindowType_plugin_ipc_content' roc, if those names are good for you please tell me what names you would like to use and I'll update.

- 8 lines of context - moved unrelated changes to the next patch stand alone this patch won't make much sense - generally I'm just short circuiting code that isn't needed in the content process, as well as adding some comments.

I just noticed two of the window type checks in here were incorrect. In ConfigureChildren we want to prevent the parent browser window from calling into the clipping apis, so the check here should be for eWindowType_plugin_ipc_chrome.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66) > (In reply to Jim Mathies [:jimm] from comment #60) > > I liked the idea of using nsIWidget with a proxy hidden behind it, but I > > suppose we could dconnect things up directly. I'm not sure though, this > > would require updates to both nsPluginFrame and nsPluginInstanceOwner, which > > I think will be a bit confusing with dual support for both e10s and > > non-e10s. I'm not seeing the advantage here? > > nsIWidget is quite a complex API and we're not going to support all of it > for remote-controller widgets (I assume). So that adds pitfalls for people > working on this code in the future. But I'm OK with doing this. Well, so there's two uses of nsIWidget here, I think we need to differentiate. On the layout side having a true xpcom nsIWidget is what this code expects, so the PluginWidgetProxy here (which is really just PuppetWidget) is no different from what layout is currently working with. This is a good use of nsIWidget since we won't have to make any changes to plugin or layout code. For the connection, I used a protocol modeled after nsIWidget, but it's not meant to be a perfect mirror of that, nor does it need to be. https://bug669200.bugzilla.mozilla.org/attachment.cgi?id=8519414 Once we get show, move, resize, and setclip going over the compositor, all we're left with is a light weight ipdl api for creating and destroying widgets on the parent side. Devs can change this api any way they see fit.

Comment on attachment 8520254 [details] [diff] [review] part 2 - widget updates Review of attachment 8520254 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsWidgetInitData.h @@ +21,5 @@ > + // desktop (has no border)) > + eWindowType_invisible, // windows that are invisible or offscreen > + eWindowType_plugin, // plugin window > + eWindowType_plugin_ipc_chrome, // chrome side native widget for plugins (e10s) > + eWindowType_plugin_ipc_content, // content side puppet widget for plugins (e10s) This looks better, thanks! ::: widget/windows/nsWindow.cpp @@ +547,5 @@ > } > // Plugins are created in the disabled state so that they can't > // steal focus away from our main window. This is especially > // important if the plugin has loaded in a background tab. > + if(aInitData->mWindowType == eWindowType_plugin || Space before ( @@ +6509,5 @@ > // If a plugin is not visible, especially if it is in a background tab, > // it should not be able to steal keyboard focus. This code checks whether > // the region that the plugin is being clipped to is NULLREGION. If it is, > // the plugin window gets disabled. > + if(IsPlugin()) { Space before (

Comment on attachment 8520241 [details] [diff] [review] part 6 - nsPluginNativeWindow(Gtk/Win) updates Review of attachment 8520241 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginNativeWindowGtk.cpp @@ +105,5 @@ > + // In this case, most of the initialization code here has already happened > + // in the chrome process. The window we have in content is the XID of the > + // socket widget we need to hand to plugins. > + SetWindow((XID)window); > + } else if (type == NPWindowTypeWindow) { Fix indent

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #75) > Comment on attachment 8520241 [details] [diff] [review] > part 6 - nsPluginNativeWindow(Gtk/Win) updates > > Review of attachment 8520241 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/plugins/base/nsPluginNativeWindowGtk.cpp > @@ +105,5 @@ > > + // In this case, most of the initialization code here has already happened > > + // in the chrome process. The window we have in content is the XID of the > > + // socket widget we need to hand to plugins. > > + SetWindow((XID)window); > > + } else if (type == NPWindowTypeWindow) { > > Fix indent sorry about the tabs, removed locally.

Comment on attachment 8519414 [details] [diff] [review] part 1 - ipdl Review of attachment 8519414 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/PPluginWidget.ipdl @@ +25,5 @@ > + * connection (PluginWidgetChild) are separated. PluginWidgetChild will > + * be torn down first by the tab, followed by the deref'ing of the nsIWidget > + * via layout. > + */ > +sync protocol PPluginWidget { After the discussion about hangs yesterday, if we want to cut down on sync content -> chrome calls here, I think it would be safe to make most of this interface async. The only calls that need to be sync are Create and GetNativePluginPort.

Comment on attachment 8520242 [details] [diff] [review] part 7 - Gtk socket widget support for chrome side I'm not qualified to review this code. Hopefully roc can do it.

Comment on attachment 8520242 [details] [diff] [review] part 7 - Gtk socket widget support for chrome side Review of attachment 8520242 [details] [diff] [review]: ----------------------------------------------------------------- Can you add [defaults] qdiff = -p -U 8 diff = -p -U 8 to ~/.hgrc? Bit of a rubber stamp review... ::: dom/plugins/ipc/PluginWidgetParent.cpp @@ +79,4 @@ > > // When plugins run in chrome, nsPluginNativeWindow(Plat) implements platform > // specific functionality that wraps plugin widgets. With e10s we currently > +// bypass this code on window, and reuse a bit of it on linux. Content still "on Windows" ::: dom/plugins/ipc/PluginWidgetParent.h @@ +42,5 @@ > mozilla::dom::TabParent* mTab; > // The chrome side native widget. > nsCOMPtr<nsIWidget> mWidget; > +#if defined(XP_LINUX) > + nsPluginNativeWindow* mWrapper; Can we make this UniquePtr<nsPluginNativeWindow> and lose the manual delete calls?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #80) > Comment on attachment 8520242 [details] [diff] [review] > part 7 - Gtk socket widget support for chrome side > > Review of attachment 8520242 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you add > [defaults] > qdiff = -p -U 8 > diff = -p -U 8 > to ~/.hgrc? > oddly that's already in there, its been there for ages. I'm not sure why hg is having such a hard time generating normal patches, but it is. :/

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #80) > Comment on attachment 8520242 [details] [diff] [review] > ::: dom/plugins/ipc/PluginWidgetParent.h > @@ +42,5 @@ > > mozilla::dom::TabParent* mTab; > > // The chrome side native widget. > > nsCOMPtr<nsIWidget> mWidget; > > +#if defined(XP_LINUX) > > + nsPluginNativeWindow* mWrapper; > > Can we make this UniquePtr<nsPluginNativeWindow> and lose the manual delete > calls? sure. I think I can make this a nsPluginNativeWindowGtk and save myself some of the casting too.

Comment on attachment 8519414 [details] [diff] [review] part 1 - ipdl Review of attachment 8519414 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/PBrowser.ipdl @@ +92,5 @@ > + * Creates a new remoted nsIWidget connection for windowed plugins > + * in e10s mode. This is always initiated from the child in response > + * to windowed plugin creation. > + */ > + sync PPluginWidget(); Any reason this needs to be sync? ::: dom/ipc/PPluginWidget.ipdl @@ +25,5 @@ > + * connection (PluginWidgetChild) are separated. PluginWidgetChild will > + * be torn down first by the tab, followed by the deref'ing of the nsIWidget > + * via layout. > + */ > +sync protocol PPluginWidget { Why does Create need to be sync?

Comment on attachment 8520225 [details] [diff] [review] part 3 - implement PPluginWidget Review of attachment 8520225 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +3112,5 @@ > > +mozilla::plugins::PPluginWidgetChild* > +TabChild::AllocPPluginWidgetChild() > +{ > + return static_cast<mozilla::plugins::PPluginWidgetChild*>(new mozilla::plugins::PluginWidgetChild()); Don't need this cast. ::: dom/plugins/ipc/PluginWidgetParent.cpp @@ +23,5 @@ > +#endif > + > +// This macro returns true to prevent an abort in the child process when > +// ipc message delivery fails. > +#define INSURE_WIDGET { \ I think ENSURE is the more common spelling, so let's use that. And I think it would look nicer to write it as ENSURE_CHANNEL(), with parens. Also, the code should be enclosed in a do { ... } while (0) block. @@ +26,5 @@ > +// ipc message delivery fails. > +#define INSURE_WIDGET { \ > + if (!mWidget) { \ > + NS_WARNING("called on an invalid remote widget."); \ > + PWLOG("%s%s",__func__," call made on on dead channel\n"); \ Given that NS_WARNING can print a full stack trace, this code doesn't really seem that useful. Maybe take it out? @@ +31,5 @@ > + return true; \ > + } \ > +} > + > +PluginWidgetParent::PluginWidgetParent(mozilla::dom::TabParent* aTab) : There isn't a great reason to have this parameter. IPDL adds a Manager() method to every generated class. In this case it will return a PBrowser, which you can then static_cast to TabParent (maybe in a utility method).

Comment on attachment 8519191 [details] [diff] [review] part 4 - implement PluginWidgetProxy Review of attachment 8519191 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, but I'd like to see a version with the __delete__ issue fixed. ::: dom/plugins/ipc/PluginWidgetParent.cpp @@ +80,4 @@ > > nsWidgetInitData initData; > initData.mWindowType = eWindowType_plugin; > + initData.mRemote = true; This seems to be left over from a previous patch. ::: widget/PluginWidgetProxy.cpp @@ +12,5 @@ > + > +/* static */ > +already_AddRefed<nsIWidget> > +nsIWidget::CreatePluginProxyWidget(TabChild* aTabChild, > + mozilla::plugins::PluginWidgetChild* aChannel) Calling this a channel seems a little odd. Maybe |aActor|? @@ +28,5 @@ > +#ifndef __func__ > +#define __func__ __FUNCTION__ > +#endif > + > +#define INSURE_CHANNEL { \ Same comments about INSURE as before. @@ +51,5 @@ > + PWLOG("PluginWidgetProxy::~PluginWidgetProxy()\n"); > +} > + > +NS_IMETHODIMP > +PluginWidgetProxy::Create(nsIWidget *aParent, I think Gecko style calls for * and & to go with the type. @@ +76,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +PluginWidgetProxy::SetParent(nsIWidget *aNewParent) Same here. @@ +107,5 @@ > + mChannel->SendShow(false); > + } > + > + if (mChannel) { > + mChannel->SendDestroy(); I'm worried that no one ever sends the __delete__ message to the actor. Consequently it will be leaked until the tab is closed. ::: widget/PluginWidgetProxy.h @@ +20,5 @@ > +class PluginWidgetChild; > +} > +namespace widget { > + > +class PluginWidgetProxy MOZ_FINAL : public PuppetWidget I think it might be possible for PluginWidgetProxy to directly derive from PPluginWidgetChild. Then you wouldn't need to keep around a separate actor object. The ActorDestroy method would just set a flag denoting the fact that it should no longer send any more messages. IPDL would own one reference to the object and the widget code would own other references. I don't think it's necessary to do this, though. Just wanted to point it out. @@ +67,5 @@ > + void ChannelDestroyed() { mChannel = nullptr; } > + > +private: > + // Our connection with the chrome widget, created on PBrowser. > + mozilla::plugins::PluginWidgetChild* mChannel; Same here: mActor.

(In reply to Bill McCloskey (:billm) from comment #83) > Comment on attachment 8519414 [details] [diff] [review] > part 1 - ipdl > > Review of attachment 8519414 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/PBrowser.ipdl > @@ +92,5 @@ > > + * Creates a new remoted nsIWidget connection for windowed plugins > > + * in e10s mode. This is always initiated from the child in response > > + * to windowed plugin creation. > > + */ > > + sync PPluginWidget(); > > Any reason this needs to be sync? > > ::: dom/ipc/PPluginWidget.ipdl > @@ +25,5 @@ > > + * connection (PluginWidgetChild) are separated. PluginWidgetChild will > > + * be torn down first by the tab, followed by the deref'ing of the nsIWidget > > + * via layout. > > + */ > > +sync protocol PPluginWidget { > > Why does Create need to be sync? Plugin code makes sync calls to apis like GetNativePluginPort immediately after the create call, so I don't think this can be async. For PPluginWidget, seems like that needs to be sync too, we call the constructor sync, then Create right after it, also sync.

(In reply to Bill McCloskey (:billm) from comment #85) > Comment on attachment 8519191 [details] [diff] [review] > part 4 - implement PluginWidgetProxy > > @@ +107,5 @@ > > + mChannel->SendShow(false); > > + } > > + > > + if (mChannel) { > > + mChannel->SendDestroy(); > > I'm worried that no one ever sends the __delete__ message to the actor. > Consequently it will be leaked until the tab is closed. It does, but we may wait until the tab is torn down. I've cleaned this up by adding a call to Send__delete__ in PluginWidgetProxy::Destroy().

(In reply to Bill McCloskey (:billm) from comment #85) > Comment on attachment 8519191 [details] [diff] [review] > part 4 - implement PluginWidgetProxy > > Review of attachment 8519191 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks fine, but I'd like to see a version with the __delete__ issue > fixed. > > ::: dom/plugins/ipc/PluginWidgetParent.cpp > @@ +80,4 @@ > > > > nsWidgetInitData initData; > > initData.mWindowType = eWindowType_plugin; > > + initData.mRemote = true; > > This seems to be left over from a previous patch. > > ::: widget/PluginWidgetProxy.cpp > @@ +12,5 @@ > > + > > +/* static */ > > +already_AddRefed<nsIWidget> > > +nsIWidget::CreatePluginProxyWidget(TabChild* aTabChild, > > + mozilla::plugins::PluginWidgetChild* aChannel) > > Calling this a channel seems a little odd. Maybe |aActor|? > > @@ +28,5 @@ > > +#ifndef __func__ > > +#define __func__ __FUNCTION__ > > +#endif > > + > > +#define INSURE_CHANNEL { \ > > Same comments about INSURE as before. > > @@ +51,5 @@ > > + PWLOG("PluginWidgetProxy::~PluginWidgetProxy()\n"); > > +} > > + > > +NS_IMETHODIMP > > +PluginWidgetProxy::Create(nsIWidget *aParent, > > I think Gecko style calls for * and & to go with the type. > > @@ +76,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +PluginWidgetProxy::SetParent(nsIWidget *aNewParent) > > Same here. > > @@ +107,5 @@ > > + mChannel->SendShow(false); > > + } > > + > > + if (mChannel) { > > + mChannel->SendDestroy(); > > I'm worried that no one ever sends the __delete__ message to the actor. > Consequently it will be leaked until the tab is closed. > > ::: widget/PluginWidgetProxy.h > @@ +20,5 @@ > > +class PluginWidgetChild; > > +} > > +namespace widget { > > + > > +class PluginWidgetProxy MOZ_FINAL : public PuppetWidget > > I think it might be possible for PluginWidgetProxy to directly derive from > PPluginWidgetChild. Then you wouldn't need to keep around a separate actor > object. The ActorDestroy method would just set a flag denoting the fact that > it should no longer send any more messages. IPDL would own one reference to > the object and the widget code would own other references. > > I don't think it's necessary to do this, though. Just wanted to point it out. Mixing nsISupport and ipdl seems fraught with lifetime issues since layout expects the proxy to live longer than PluginWidgetChild.. which has delete called on it in the tab child code when the protocol chain is torn down. I can file a follow up on trying to blend these I guess, but I'd rather not muck around with that in this bug.

(In reply to Jim Mathies [:jimm] from comment #86) > Plugin code makes sync calls to apis like GetNativePluginPort immediately > after the create call, so I don't think this can be async. For > PPluginWidget, seems like that needs to be sync too, we call the constructor > sync, then Create right after it, also sync. IPDL ensures that messages are always processed in the order they were sent. So even if the constructor and Create are async, they're guaranteed to finish before GetNativePluginPort runs. Making them async just means we only do one roundtrip (for GetNativePluginPort) rather than three.

> Mixing nsISupport and ipdl seems fraught with lifetime issues since layout expects the proxy > to live longer than PluginWidgetChild.. which has delete called on it in the tab child code > when the protocol chain is torn down. It's fine to postpone this, but I just wanted to point out that IPDL doesn't delete PluginWidgetChild. It just calls the DeallocPPluginWidgetChild method. That method can choose to decref instead of delete (as it currently does). Here's an example: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/nsIContentParent.cpp#145

Comment on attachment 8521441 [details] [diff] [review] part 4 - implement PluginWidgetProxy Review of attachment 8521441 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: widget/PluginWidgetProxy.cpp @@ +105,5 @@ > + if (mActor) { > + mActor->SendDestroy(); > + } > + > + if (mActor) { Please move all of these into a single if statement.

(In reply to Bill McCloskey (:billm) from comment #96) > (In reply to Jim Mathies [:jimm] from comment #86) > > Plugin code makes sync calls to apis like GetNativePluginPort immediately > > after the create call, so I don't think this can be async. For > > PPluginWidget, seems like that needs to be sync too, we call the constructor > > sync, then Create right after it, also sync. > > IPDL ensures that messages are always processed in the order they were sent. > So even if the constructor and Create are async, they're guaranteed to > finish before GetNativePluginPort runs. Making them async just means we only > do one roundtrip (for GetNativePluginPort) rather than three. Ok sounds good. I've updated the ipdl, everything is async except GetNativePluginPort.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/10f4a7ba42c8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc233b91a99 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcaebd09531c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad862fdf75cd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/115f074c9b0d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/706254cbf092 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/99bad02916c6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f72f08089393 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c63b3b1de80

https://hg.mozilla.org/mozilla-central/rev/10f4a7ba42c8 https://hg.mozilla.org/mozilla-central/rev/dcc233b91a99 https://hg.mozilla.org/mozilla-central/rev/bcaebd09531c https://hg.mozilla.org/mozilla-central/rev/ad862fdf75cd https://hg.mozilla.org/mozilla-central/rev/115f074c9b0d https://hg.mozilla.org/mozilla-central/rev/706254cbf092 https://hg.mozilla.org/mozilla-central/rev/99bad02916c6 https://hg.mozilla.org/mozilla-central/rev/f72f08089393 https://hg.mozilla.org/mozilla-central/rev/6c63b3b1de80 https://hg.mozilla.org/mozilla-central/rev/64f1fb1e2f38

Jim: now that this plugin fix has landed, should bug 923745 be backed out as per comment 5 and the whiteboard comment?

(In reply to Chris Peterson (:cpeterson) from comment #103) > Jim: now that this plugin fix has landed, should bug 923745 be backed out as > per comment 5 and the whiteboard comment? This work was removed through various plugin landings, I think we're ok here I can't find any of this in the tree.

(In reply to zhoubcfan from comment #106) > Created attachment 8554969 [details] [diff] [review] > fix wrong include guard Thanks for catching this. Would you like to file a new bug and post the patch there? This bug is already fixed, so we can't land it from here.