Closed Bug 1264545 Opened 4 years ago Closed 4 years ago

Restrict Compositor access to nsIWidget

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 7 obsolete files)

12.72 KB, patch
nical
: review+
Details | Diff | Splinter Review
11.60 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.96 KB, patch
jimm
: review+
Details | Diff | Splinter Review
16.13 KB, patch
jimm
: review+
Details | Diff | Splinter Review
13.14 KB, patch
jimm
: review+
Details | Diff | Splinter Review
35.60 KB, patch
jimm
: review+
Details | Diff | Splinter Review
81.17 KB, patch
Details | Diff | Splinter Review
In preparation for out-of-process compositing work, we need to restrict compositor access to nsIWidget. The current plan is to introduce an intermediary class, CompositorWidgetProxy, containing the methods the compositor uses.

We would begin by moving nsIWidget methods like StartRemoteDrawing, PreRender, etc, into this class, and having nsIWidget inherit from CompositorWidgetProxy. Then we can change the compositor to hold a CompositorWidgetProxy instead.

Next, we would remove nsIWidget's inheritance of CompositorWidgetProxy, and build a generic in-process wrapper around the nsIWidget. From there we could begin building platform-specific wrappers in separate bugs to eliminate actual nsIWidget use.

Two notes:

Some parts of the compositor, for various reasons, need the actual nsIWidget. Mostly this happens in CompositorOGL. Calls to this getter will have to be eliminated for each compositor to work out-of-process.

To make a CompositorWidgetProxy fully OOP-compatible, we may have to invert how some things are communicated. For example, on Windows, the compositor enters a shared lock via the widget. It will make more sense in the OOP world for the widget to enter a lock in the compositor. Similarly, things that are requested off the widget now (like whether or not APZ is enabled) will have to be communicated to the proxy.
Every compositor has its own copy of mWidget. There's an :XXX: to hoist this out, so now seems like a good time to do it.
Attachment #8741251 - Flags: review?(nical.bugzilla)
This is just the interface of compositor-accessed methods on nsIWidget. Initially, nsIWidget inherits from this. (This inheritance will be removed in part 5.)
Attachment #8741253 - Flags: review?(jmathies)
This changes Compositor::GetWidget to return the proxy pointer instead.
Attachment #8741254 - Flags: review?(jmathies)
This changes mWidget to be a CompositorWidgetProxy instead of an nsIWidget.
Attachment #8741255 - Flags: review?(jmathies)
Attachment #8741251 - Flags: review?(nical.bugzilla) → review+
Some later patches uncovered minor problems, so I'm attaching new versions of these patches to address those.
Attachment #8741253 - Attachment is obsolete: true
Attachment #8741253 - Flags: review?(jmathies)
Attachment #8742207 - Flags: review?(jmathies)
Attachment #8741254 - Attachment is obsolete: true
Attachment #8741254 - Flags: review?(jmathies)
Attachment #8742208 - Flags: review?(jmathies)
Attachment #8741255 - Attachment is obsolete: true
Attachment #8741255 - Flags: review?(jmathies)
Attachment #8742210 - Flags: review?(jmathies)
(assuming the other patches look okay to Jim)
Attachment #8742211 - Flags: review?(bugmail.mozilla)
Attached patch part 6, break inheritance WIP (obsolete) — Splinter Review
This moves CompositorWidgetProxy inheritance from nsIWidget into each individual platform widget. Only Windows is done so far.

After this patch, the goal will be to move each widget's CompositorWidgetProxy methods into a separate object, so it can be allocated in a separate process.
Comment on attachment 8742211 [details] [diff] [review]
part 5, make CompositorBridgeParent use a proxy

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

There's an override in widget/android/nsWindow.cpp that needs modifying also. Same in gonk if you care about B2G.
Attachment #8742211 - Flags: review?(bugmail.mozilla) → review+
Attachment #8742207 - Flags: review?(jmathies) → review+
Comment on attachment 8742208 [details] [diff] [review]
part 3, make Compositor::GetWidget return proxy

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

I'm not clear on the relationship, but I'm concerned this work might impact the value returned by CompositorBridgeParent's GetWidget()? That value plays into controlling plugin window visibility and needs to point to the actual nsIWidget. It doesn't look like you're altering that code here, just wanted to bring it up.
Attachment #8742208 - Flags: review?(jmathies) → review+
Attachment #8742210 - Flags: review?(jmathies) → review+
Comment on attachment 8742211 [details] [diff] [review]
part 5, make CompositorBridgeParent use a proxy

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

::: gfx/layers/ipc/CompositorBridgeParent.h
@@ +488,5 @@
>     * Returns true if the calling thread is the compositor thread.
>     */
>    static bool IsInCompositorThread();
>  
> +  widget::CompositorWidgetProxy* GetWidget() { return mWidget; }

Oh, and here it is. This is going to break plugin window lookups.

For example:

SendHideAllPlugins takes a nsIWidget:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#2453
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeChild.cpp#379
http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#2081
http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#2102
Attachment #8742211 - Flags: feedback-
I renamed mWidget to mWidgetProxy to catch this. We'll have to change later this to get rid of the RealWidget() call, but doing so looks pretty straightforward.
Attachment #8742211 - Attachment is obsolete: true
Attachment #8743082 - Flags: review?(jmathies)
builds on android now
Attachment #8743082 - Attachment is obsolete: true
Attachment #8743082 - Flags: review?(jmathies)
Attachment #8743099 - Flags: review?(jmathies)
Attached patch part 6, break inheritance (obsolete) — Splinter Review
This moves inheritance of CompositorWidgetProxy from nsIWidget to each individual platform widget. For a widget to be OOP-ready, it will have to (at least) remove this new inheritance, but this can be done on a per-widget basis now.

Due to multiple inheritance rules, GetClientSize had to be re-implemented in each widget, which is kind of annoying. It should (in theory) be temporary as each widget is made OOP-compatible, but otherwise if you have a better, non-conflicting name I can do that instead.
Attachment #8742212 - Attachment is obsolete: true
Attachment #8743130 - Flags: review?(jmathies)
Comment on attachment 8743130 [details] [diff] [review]
part 6, break inheritance

kats for TestCompositor/MockWidget changes.
Attachment #8743130 - Flags: review?(bugmail.mozilla)
Comment on attachment 8743130 [details] [diff] [review]
part 6, break inheritance

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

- I'm assuming the changes to nsMeterFrame.h are for unified build issues, because otherwise they seem unrelated.
- For the GetClientSize thing, you should be able to do |using nsIWidget::GetClientSize;| instead of actually overriding the function and delegating, which is a little cleaner. See https://gist.github.com/anonymous/75997e6e5879db77e16df3d945392b9f for a simplified example
- The testing code changes look fine, and I quickly glanced through the rest but didn't do a thorough review. Still, everything looked reasonable.

::: widget/nsBaseWidget.h
@@ +484,5 @@
>    virtual void WindowUsesOMTC() {}
>    virtual void RegisterTouchWindow() {}
>  
>    nsIDocument* GetDocument() const;
> +  

nit: trailing whitespace
Attachment #8743130 - Flags: review?(bugmail.mozilla) → review+
Attachment #8743099 - Flags: review?(jmathies) → review+
Attachment #8743130 - Flags: review?(jmathies) → review+
This approach turned out not to work so great. Managing the lifetime of the proxy is much more complicated when it can either be an independent object or an nsIWidget. Also during shutdown, the compositor can call back into nsIWidget as the nsBaseWidget destructor is running, which means inheriting the proxy at a more derived level is broken.

This patch instead introduces CompositorWidgetProxyWrapper. Widgets are responsible for instantiating this, and it simply calls back into nsBaseWidget to implement each method.

Widgets can override individual methods or use non-wrapper class to support out-of-process compositing.
Attachment #8743130 - Attachment is obsolete: true
Attachment #8745828 - Flags: review?(jmathies)
Comment on attachment 8745828 [details] [diff] [review]
part 6, break inheritance v2

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

Would you mind posting a rollup?

::: widget/CompositorWidgetProxy.cpp
@@ +155,5 @@
> +CompositorWidgetProxyWrapper::CleanupWindowEffects()
> +{
> +  if (!mWidget) {
> +    return;
> +  }

You could make this a macro and save some space with the trade off that debugging a null widget requires an edit.

::: widget/cocoa/nsCocoaWindow.h
@@ +386,5 @@
>  
>      NS_IMETHOD         ReparentNativeWidget(nsIWidget* aNewParent) override;
> +
> +    CompositorWidgetProxy* NewCompositorWidgetProxy() override {
> +      return nullptr;

why is this returning null?
Attachment #8745828 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #19)
> Comment on attachment 8745828 [details] [diff] [review]
> part 6, break inheritance v2
> 
> ::: widget/cocoa/nsCocoaWindow.h
> @@ +386,5 @@
> >  
> >      NS_IMETHOD         ReparentNativeWidget(nsIWidget* aNewParent) override;
> > +
> > +    CompositorWidgetProxy* NewCompositorWidgetProxy() override {
> > +      return nullptr;
> 
> why is this returning null?

It looked like nsCocoaWindow doesn't create compositors? If that's wrong I can change it. It did pass on try though.
Attached patch rollupSplinter Review
Here's the rollup patch.
You need to log in before you can comment on or make changes to this bug.