1261107 - Add ability to marshal a COM object and transfer its serialized proxy across IPDL

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Lots of this was already implemented as test code in bug 1258844, so it just needs to be Gecko-ized.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: WIP

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1261107: Add IsCurrentThreadMTA() to ipc/mscom/Utils;

Review commit: https://reviewboard.mozilla.org/r/65048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65048/

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1261107: Add EnsureMTA class to ipc/mscom/Utils. This class synchronously executes a function on a background thread that resides in Microsoft COM's multithreaded apartment;

Review commit: https://reviewboard.mozilla.org/r/65050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65050/

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1261107: Adds code to marshal a Microsoft COM object and transfer its serialized proxy across IPDL;

Review commit: https://reviewboard.mozilla.org/r/65052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65052/

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #4)
> Created attachment 8772179 [details]
> Bug 1261107: Adds code to marshal a Microsoft COM object and transfer its
> serialized proxy across IPDL;
> 
> Review commit: https://reviewboard.mozilla.org/r/65052/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/65052/

Bill, I added you specifically for the IPDL (de)serialization bits.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8772177 [details]
Bug 1261107: Add IsCurrentThreadMTA() to ipc/mscom/Utils;

https://reviewboard.mozilla.org/r/65048/#review62262

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8772178 [details]
Bug 1261107: Add EnsureMTA class to ipc/mscom/Utils. This class synchronously executes a function on a background thread that resides in Microsoft COM's multithreaded apartment;

https://reviewboard.mozilla.org/r/65050/#review62266

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8772179 [details]
Bug 1261107: Adds code to marshal a Microsoft COM object and transfer its serialized proxy across IPDL;

https://reviewboard.mozilla.org/r/65052/#review62354

::: ipc/mscom/COMPtrHolder.h:58
(Diff revision 1)
> +  COMPtrHolder(COMPtrHolder&& aOther)
> +    : mPtr(Move(aOther.mPtr))
> +  {
> +  }
> +
> +  // Terrible hack because IPDL structs don't use move semantics

Could you say more about this?

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Bill McCloskey (:billm) from comment #8)
> Comment on attachment 8772179 [details]
> Bug 1261107: Adds code to marshal a Microsoft COM object and transfer its
> serialized proxy across IPDL;
> 
> https://reviewboard.mozilla.org/r/65052/#review62354
> 
> ::: ipc/mscom/COMPtrHolder.h:58
> (Diff revision 1)
> > +  COMPtrHolder(COMPtrHolder&& aOther)
> > +    : mPtr(Move(aOther.mPtr))
> > +  {
> > +  }
> > +
> > +  // Terrible hack because IPDL structs don't use move semantics
> 
> Could you say more about this?

In a later patch the COMPtrHolder is eventually added as a member of a struct that is declared in IPDL. The generated C++ code for that IPDL struct assumes that everything is copyable. I don't think that those copy constructors and operator= are actually used by any generated code, but they are made available.

I *think* that what we would need to avoid this hack is to be able to explicitly declare the copy/move semantics of the struct in IPDL.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8772179 [details]
Bug 1261107: Adds code to marshal a Microsoft COM object and transfer its serialized proxy across IPDL;

https://reviewboard.mozilla.org/r/65052/#review62670

generally this is really impressive work. a couple questions about implementation details below.

::: ipc/mscom/ProxyStream.cpp:31
(Diff revision 1)
> +}
> +
> +// GetBuffer() fails with this variant, but that's okay because we're just
> +// reconstructing the stream from a buffer anyway.
> +ProxyStream::ProxyStream(const BYTE* aInitBuf, const int aInitBufSize)
> +  : mStream(InitStream(aInitBuf, static_cast<const UINT>(aInitBufSize)))

Should we check or assert on a null mStream here? It looks like InitStream can fail in a couple different ways.

::: ipc/mscom/ProxyStream.cpp:51
(Diff revision 1)
> +  {
> +    IUnknown* rawUnmarshaledProxy = nullptr;
> +    // OK to forget mStream when calling into this function because the stream
> +    // gets released even if the unmarshaling part fails.
> +    DebugOnly<HRESULT> hr =
> +      ::CoGetInterfaceAndReleaseStream(mStream.forget().take(), IID_IUnknown,

I can see why we would forget this here based on the docs, but why do you take() it again?

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #10)
> Comment on attachment 8772179 [details]
> Bug 1261107: Adds code to marshal a Microsoft COM object and transfer its
> serialized proxy across IPDL;
> 
> https://reviewboard.mozilla.org/r/65052/#review62670
> 
> generally this is really impressive work. a couple questions about
> implementation details below.
> 
> ::: ipc/mscom/ProxyStream.cpp:31
> (Diff revision 1)
> > +}
> > +
> > +// GetBuffer() fails with this variant, but that's okay because we're just
> > +// reconstructing the stream from a buffer anyway.
> > +ProxyStream::ProxyStream(const BYTE* aInitBuf, const int aInitBufSize)
> > +  : mStream(InitStream(aInitBuf, static_cast<const UINT>(aInitBufSize)))
> 
> Should we check or assert on a null mStream here? It looks like InitStream
> can fail in a couple different ways.

I can add a check for that, but it should go *after* the check for a 0 aInitBufSize, since InitStream may return nullptr in that case even though it is a legit corner case.

> 
> ::: ipc/mscom/ProxyStream.cpp:51
> (Diff revision 1)
> > +  {
> > +    IUnknown* rawUnmarshaledProxy = nullptr;
> > +    // OK to forget mStream when calling into this function because the stream
> > +    // gets released even if the unmarshaling part fails.
> > +    DebugOnly<HRESULT> hr =
> > +      ::CoGetInterfaceAndReleaseStream(mStream.forget().take(), IID_IUnknown,
> 
> I can see why we would forget this here based on the docs, but why do you
> take() it again?

That's just to extract the raw pointer from the already_AddRefed returned by forget()

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772177 [details]
Bug 1261107: Add IsCurrentThreadMTA() to ipc/mscom/Utils;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65048/diff/1-2/

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772178 [details]
Bug 1261107: Add EnsureMTA class to ipc/mscom/Utils. This class synchronously executes a function on a background thread that resides in Microsoft COM's multithreaded apartment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65050/diff/1-2/

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772179 [details]
Bug 1261107: Adds code to marshal a Microsoft COM object and transfer its serialized proxy across IPDL;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65052/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e108877f47de
Add IsCurrentThreadMTA() to ipc/mscom/Utils; r=jimm
https://hg.mozilla.org/integration/autoland/rev/e87f3cbb950f
Add EnsureMTA class to ipc/mscom/Utils. This class synchronously executes a function on a background thread that resides in Microsoft COM's multithreaded apartment; r=jimm
https://hg.mozilla.org/integration/autoland/rev/23d63b3b2c75
Adds code to marshal a Microsoft COM object and transfer its serialized proxy across IPDL; r=billm,jimm

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e108877f47de
https://hg.mozilla.org/mozilla-central/rev/e87f3cbb950f
https://hg.mozilla.org/mozilla-central/rev/23d63b3b2c75