782451 - Work - OleSetClipboard always fails with winrt backend

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsClipboard.cpp#236

The call fails with a "CoInit was never called" error, however com has been initialized. I confirmed this by adding a RoInit on the metro main thread which return S_FALSE, indicating COM was already set up. (Interesting to note that the main thread was initialized with RO_INIT_SINGLETHREADED.)

So I'm not sure what's wrong here. Winrt has a wrapper around these calls which we can use - 

http://msdn.microsoft.com/en-us/library/windows/apps/br205867.aspx

But I'd rather get our existing code working rather than reformulating it to work with the winrt classes.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I'll take a look

Comment 2

[Brian R. Bondy [:bbondy]]

Hey Tim, this should be failing because we're no longer calling OleInitialize in WinRT code.  This call only works when Ole is initialized. It's not related to CoInitialize/RoInitialize.

Comment 3

[Brian R. Bondy [:bbondy]]

OleInitialize calls CoInitialize internally but also does some other stuff that is needed for the clipboard.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Thanks Brian!

I had noticed that `OleInitialize` is probably the initialization function that we wanted.  I'm testing locally whether calling `OleInitialize` before attempting the `OleSetClipboard` works, and if it does I'll post a patch shortly.

Comment 5

[Brian R. Bondy [:bbondy]]

Sounds good, I'd do the call somewhere higher up, maybe in MetroWidget::MetroWidget's !gInstanceCount condition.

Then in the destructor under !gInstanceCount call both OleFlushClipboard and OleUninitialize

Comment 6

[Brian R. Bondy [:bbondy]]

Check out how nsWindow.cpp does it with the error check in case the initialize fails (using sIsOleInitialized)

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch

This patch creates an `OleInitializeWrapper` to mimic `RoInitializeWrapper`.  It creates an `OleInitializeWrapper` member of `MetroWidget` that initializes OLE in its constructor and, when the final `MetroWidget` is destroyed, handles OLE uninitialization.

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 658545 [details] [diff] [review]
Patch

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

Feel free to land on elm with those changes, thanks!

::: widget/windows/winrt/MetroWidget.cpp
@@ +70,5 @@
>    gInstanceCount--;
>  
>    // Global shutdown
>    if (!gInstanceCount) {
> +    mOleInitializeWrapper.flush();

Just put the flush call in the destructor instead

::: widget/windows/winrt/MetroWidget.h
@@ +142,5 @@
> +    HRESULT const hr;
> +
> +    operator HRESULT() const {
> +      return hr;
> +    }

I don't think this is needed.

@@ +156,5 @@
> +    {
> +    }
> +
> +    ~OleInitializeWrapper() {
> +      ::OleUninitialize();

Also wrap this inside an if (SUCCEEDED(hr)) {

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: patch

Implemented review comments.  I'll test locally and push to elm.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/projects/elm/rev/4e39611ec963