Closed Bug 1261107 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- affected
firefox50 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

(Blocks 1 open bug)

Details

(Whiteboard: aes+)

Attachments

(4 files)

Lots of this was already implemented as test code in bug 1258844, so it just needs to be Gecko-ized.
Depends on: 1263224
Attached patch WIPSplinter Review
Whiteboard: aes-win
Whiteboard: aes-win → aes+
Depends on: 1287002
Review commit: https://reviewboard.mozilla.org/r/65048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65048/
Attachment #8772177 - Flags: review?(jmathies)
Attachment #8772178 - Flags: review?(jmathies)
Attachment #8772179 - Flags: review?(wmccloskey)
Attachment #8772179 - Flags: review?(jmathies)
(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 on attachment 8772177 [details]
Bug 1261107: Add IsCurrentThreadMTA() to ipc/mscom/Utils;

https://reviewboard.mozilla.org/r/65048/#review62262
Attachment #8772177 - Flags: review?(jmathies) → review+
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
Attachment #8772178 - Flags: review?(jmathies) → review+
Attachment #8772179 - Flags: review?(wmccloskey) → review+
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 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 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?
Attachment #8772179 - Flags: review?(jmathies) → review+
(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 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 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 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/
Pushed by aklotz@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/e108877f47de
https://hg.mozilla.org/mozilla-central/rev/e87f3cbb950f
https://hg.mozilla.org/mozilla-central/rev/23d63b3b2c75
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.