Closed Bug 1265975 Opened 4 years ago Closed 4 years ago

Remove compositor widget access on Windows

Categories

(Core :: Widget: Win32, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 8 obsolete files)

10.98 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.90 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.15 KB, patch
jimm
: review+
Details | Diff | Splinter Review
24.32 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.73 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.58 KB, patch
jimm
: review+
Details | Diff | Splinter Review
There are a few things the compositor needs to know about the widget, like its HWND, transparency, size - it looks like all of these can be separated out. Keeping them in sync might be a bit difficult, we'll see how that pans out. There's also a lock that's shared between WM_SETTEXT and the compositor that needs to be moved.
Attached patch part 1, wrapper class WIP (obsolete) — Splinter Review
Attachment #8743147 - Attachment is obsolete: true
Attached patch part 1, WinCompositorWidgetProxy (obsolete) — Splinter Review
This implements CompositorWidgetProxy as a wrapper around nsWindow.
Attachment #8743148 - Attachment is obsolete: true
This is the object that will sit on the compositor thread, in OOP mode - or shared directly with the compositor when in-process. In OOP mode, the actual CompositorWidgetProxy on the widget side will have to remote relevant callbacks.
Attachment #8743562 - Attachment is obsolete: true
Attachment #8744784 - Flags: review?(jmathies)
The compositor shouldn't access nsWindow::mBounds directly, so instead this uses GetClientRect.
Attachment #8743563 - Attachment is obsolete: true
Attachment #8744785 - Flags: review?(jmathies)
Move the WM_SETTEXT lock to the proxy. In OOP mode, at least EnterPresentLock will probably have to be a synchronous message.
Attachment #8743564 - Attachment is obsolete: true
Attachment #8744786 - Flags: review?(jmathies)
Attached patch part 4, move remote drawing code (obsolete) — Splinter Review
This patch moves the transparent surface and Start/EndRemoteDrawing code to WinCompositorWidgetProxy. It should have the same functionality, though I cleaned up the code a little (for example, SetupTranslucentMemoryBitmap got split into two methods).

I'm not sure how to test this - when are windows transparent?
Attachment #8744797 - Flags: review?(jmathies)
Comment on attachment 8744784 [details] [diff] [review]
part 1, WinCompositorWidgetProxy

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

::: widget/windows/WinCompositorWidgetProxy.cpp
@@ +10,5 @@
> +namespace widget {
> +
> +WinCompositorWidgetProxy::WinCompositorWidgetProxy(nsWindow* aWindow)
> + : mWindow(aWindow)
> +{

lets add a debug assertion here that aWindow is valid.

::: widget/windows/WinCompositorWidgetProxy.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace widget {
> + 
> +class WinCompositorWidgetProxy : public CompositorWidgetProxyBase

nit - bit o whitepsace here

also, please add a comment header here with a brief description of what this class is for.
Attachment #8744784 - Flags: review?(jmathies) → review+
Comment on attachment 8744785 [details] [diff] [review]
part 2, implement GetClientSize

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

::: widget/windows/WinCompositorWidgetProxy.cpp
@@ +10,5 @@
>  namespace widget {
>  
>  WinCompositorWidgetProxy::WinCompositorWidgetProxy(nsWindow* aWindow)
> + : mWindow(aWindow),
> +   mWnd(reinterpret_cast<HWND>(aWindow->GetNativeData(NS_NATIVE_WINDOW)))

another assertion here too on mWnd.
Attachment #8744785 - Flags: review?(jmathies) → review+
Comment on attachment 8744786 [details] [diff] [review]
part 3, move WM_SETTEXT lock

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

::: widget/windows/nsWindow.cpp
@@ +4967,5 @@
>          // To do this we take mPresentLock in nsWindow::PreRender and
>          // if that lock is taken we wait before doing WM_SETTEXT
> +        //
> +        // Note: Store the proxy locally, in case it goes away while updating
> +        // the window.

this is interesting since nsWindow creates this. Why would it go away?
Attachment #8744786 - Flags: review?(jmathies) → review+
Rebased for the latest changes to bug 1264545.
Attachment #8744797 - Attachment is obsolete: true
Attachment #8744797 - Flags: review?(jmathies)
Attachment #8746402 - Flags: review?(jmathies)
Attached patch part 5, remove compositor calls (obsolete) — Splinter Review
After this there should be no more calls from Windows compositors into nsIWidget. Aside from CompositorOGL, which we don't support on Windows.
Attachment #8746403 - Flags: review?(jmathies)
Attached patch part 5, remove compositor calls (obsolete) — Splinter Review
Same patch, but with the MOZ_ASSERT_UNREACHABLE in RealWidget() removed. Turns out we're not ready for that yet.
Attachment #8746403 - Attachment is obsolete: true
Attachment #8746403 - Flags: review?(jmathies)
Attachment #8746868 - Flags: review?(jmathies)
Err, correct patch/
Attachment #8746868 - Attachment is obsolete: true
Attachment #8746868 - Flags: review?(jmathies)
Attachment #8746877 - Flags: review?(jmathies)
CompositorBridgeParent has RealWidget() calls that send the nsIWidget* back to the UI thread for hiding plugins. This patch adds a "GetWidgetKey" function to CompositorWidgetProxy so we don't need to have the nsIWidget pointer around.

The key is still the widget pointer casted to uintptr_t. From looking at the code, this shouldn't have any security implications for out-of-process compositing - maybe at worst a harmless ABA problem, but it seems unlikely. If we're worried we can make the key more advanced though.
Attachment #8746878 - Flags: review?(jmathies)
Attachment #8746402 - Flags: review?(jmathies) → review+
Attachment #8746877 - Flags: review?(jmathies) → review+
Comment on attachment 8746878 [details] [diff] [review]
part 6, fix plugin hiding

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

::: widget/CompositorWidgetProxy.h
@@ +165,5 @@
>    virtual void CleanupRemoteDrawing();
>  
>    /**
> +   * Return a key that can represent the widget object round-trip across the
> +   * CompositorBridge channel. This only needs to be implemented on GTK and

This isn't really a generic key, it need to be the window widget that owns this compositor. The casting just makes it easy to move around ipdl. We do a direct compare of this with the parent widget in nsIWidget::UpdateRegisteredPluginWindowVisibility.
Attachment #8746878 - Flags: review?(jmathies) → review+
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.