Closed Bug 1303060 Opened 8 years ago Closed 7 years ago

Implement COM handler to reduce RPC round-trips

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: aes+)

Attachments

(9 files, 8 obsolete files)

59 bytes, text/x-review-board-request
jimm
: review+
Details
59 bytes, text/x-review-board-request
jimm
: review+
Details
880 bytes, patch
Details | Diff | Splinter Review
20.44 KB, patch
jimm
: review+
Details | Diff | Splinter Review
33.13 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
2.49 KB, patch
mshal
: review+
Details | Diff | Splinter Review
39.88 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
2.04 KB, patch
mshal
: review+
Details | Diff | Splinter Review
7.08 KB, patch
Details | Diff | Splinter Review
COM handlers allow us to implement an object that wraps a COM proxy and allows us to handle calls to the interface before passing them on to the proxy. By doing so, we may use the handler to query some cached values that were sent in a payload along with the proxy.

For ATs that inject into our chrome process, the handler will be loaded into our chrome process as well. For clients that use out-of-process COM hooks, the handler would be loaded into their address space.

See also:
[1] https://www.microsoft.com/msj/0599/com/com0599.aspx
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms683786.aspx
[3] Wicked Code, August 2000 MSDN Magazine. This is not available on the MSDN Magazine website but I have an archived copy on CD-ROM.
Blocks: 1258839
No longer blocks: 1296571
I have a naive implementation of this that does not invalidate. Initial tests show a 60% reduction in the amount of time required to enumerate all visible accessibles in a GMail tab. \o/
(In reply to Aaron Klotz [:aklotz] from comment #1)
> I have a naive implementation of this that does not invalidate. Initial
> tests show a 60% reduction in the amount of time required to enumerate all
> visible accessibles in a GMail tab. \o/

Very promising! I don't want to muddy things here but I can't help but wonder if we can utilize the same cache for Trevor's linux/mac solution?
(In reply to David Bolter [:davidb] from comment #2)
> Very promising! I don't want to muddy things here but I can't help but
> wonder if we can utilize the same cache for Trevor's linux/mac solution?

Unfortunately this is fairly platform and toolkit specific. It might certainly be worth evaluating the feasibility of such an approach using the facilities available on the other platforms.
Whiteboard: aes+
Keywords: perf
Depends on: 1338596
Depends on: 1338600
Depends on: 1338609
Depends on: 1339942
Depends on: 1339945
Depends on: 1339951
This relies on implementing the IHandlerPayload interface from previous patches in this bug, as well as the patch from bug 1338609.

This is the code where we gather the data for the payload. Since the request comes in on a background thread, which we then forward that call to the main thread for payload retrieval.
Attachment #8840046 - Flags: review?(tbsaunde+mozbugs)
Attached patch First cut of the handler dll (obsolete) — Splinter Review
This is the main implementation of the DLL.

Most of it is the AccessibleHandler class, which is a boilerplate implementation of IAccessible2_3. It accesses cached fields when available, otherwise it forwards the request to the COM proxy. The AccessibleHandlerControl class maintains a generation count. When that count is out of sync with the generation of the handler object's cache, the handler object refreshes itself.
Attachment #8840050 - Flags: review?(tbsaunde+mozbugs)
This patch just adds invalidation code. It fires an async COM request to each handler dll, requesting that the dll increment its cache generation count.
Attachment #8840052 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8839620 [details]
Bug 1303060: ipc/mscom changes for handler and payload support;

https://reviewboard.mozilla.org/r/114192/#review116060
Attachment #8839620 - Flags: review?(jmathies) → review+
Comment on attachment 8839621 [details]
Bug 1303060: Additions to ipc/mscom for out-of-process components;

https://reviewboard.mozilla.org/r/114194/#review116066
Attachment #8839621 - Flags: review?(jmathies) → review+
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/604605ca738c
ipc/mscom changes for handler and payload support; r=jimm
https://hg.mozilla.org/integration/autoland/rev/729a1b8c7e47
Additions to ipc/mscom for out-of-process components; r=jimm
Comment on attachment 8840052 [details] [diff] [review]
Add invalidation events to a11y code

># HG changeset patch
># User Aaron Klotz <aklotz@mozilla.com>
># Date 1487203868 25200
>#      Wed Feb 15 17:11:08 2017 -0700
># Node ID b5cfa3a2d9e50275ee3a10cc49c956177261c5ee
># Parent  df1c577237460b92f762988c206b438c76d64f46
>Bug 1303060: Fire invalidation events to the com handler; r?tbsaunde
>
>MozReview-Commit-ID: DDWP0ziAPhc
>
>diff --git a/accessible/ipc/win/DocAccessibleChild.cpp b/accessible/ipc/win/DocAccessibleChild.cpp
>--- a/accessible/ipc/win/DocAccessibleChild.cpp
>+++ b/accessible/ipc/win/DocAccessibleChild.cpp
>@@ -3,16 +3,17 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "DocAccessibleChild.h"
> 
> #include "Accessible-inl.h"
> #include "mozilla/a11y/PlatformChild.h"
>+#include "mozilla/a11y/AccessibleWrap.h"
> #include "mozilla/ClearOnShutdown.h"
> #include "RootAccessible.h"
> 
> namespace mozilla {
> namespace a11y {
> 
> static StaticAutoPtr<PlatformChild> sPlatformChild;
> 
>@@ -106,65 +107,70 @@ DocAccessibleChild::PushDeferredEvent(Un
>     topLevelIPCDoc->mDeferredEvents.AppendElement(Move(aEvent));
>   }
> }
> 
> bool
> DocAccessibleChild::SendEvent(const uint64_t& aID, const uint32_t& aType)
> {
>   if (IsConstructedInParentProcess()) {
>+    AccessibleWrap::InvalidateHandlers();

I think this and some of the other calls invalidate more than they really need to, but I guess that's fine for now.

> DocAccessibleChild::SendRoleChangedEvent(const uint32_t& aRole)
> {
>   if (IsConstructedInParentProcess()) {
>+    AccessibleWrap::InvalidateHandlers();

especially this one, only the one objects role has changed, and for !windows we already cache this, but whatever.

>+void
>+AccessibleWrap::InvalidateHandlers()
>+{
>+  static const HRESULT kErrorServerDied =
>+    HRESULT_FROM_WIN32(RPC_S_SERVER_UNAVAILABLE);
>+  MOZ_ASSERT(XRE_IsContentProcess() && NS_IsMainThread());

might be good to put them on different lines so its easy to know which triggered.

>+  AutoTArray<DWORD, 1> defunctHandlerPids;
>+
>+  for (auto&& controller : *sHandlerControllers) {

I think a normal lval ref would be fine here.

That said this whole loop seems over complicated, can't you just loop over the array backwards and remove elements that are dead?

>+    MOZ_ASSERT(controller.mPid && controller.mCtrl);

separate lines?

>+    ASYNC_INVOKER_FOR(IHandlerControl) invoker(controller.mCtrl);

tbh just doing AsyncInvoker<AsyncIFoo> seems simpler to me, but no big deal.
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5c379a1a3cb
ipc/mscom changes for handler and payload support; r=jimm
https://hg.mozilla.org/integration/autoland/rev/e08dabc92ecd
Additions to ipc/mscom for out-of-process components; r=jimm
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/d3241c1454e4
Fix problematic annotation on mscom::InParamWalker; r=staticbustage-fix
This revision has us send the IHandlerControl interface up into chrome so that we may dispatch handler events from there. This is important when we dispatch text change events, as we need them to be synchronized with respect to other events being dispatched by the chrome process.
Attachment #8840046 - Attachment is obsolete: true
Attachment #8840046 - Flags: review?(tbsaunde+mozbugs)
Attachment #8843006 - Flags: review?(tbsaunde+mozbugs)
This addresses your previous comments on this patch, and also reflects the changes in the other patch, as this code now executes on the chrome process main thread.

We probably want to be smarter about the InvalidateHandlers() call but we can do that in a follow-up; this code will be disabled by default when it lands.
Attachment #8840052 - Attachment is obsolete: true
Attachment #8840052 - Flags: review?(tbsaunde+mozbugs)
Attachment #8843008 - Flags: review?(tbsaunde+mozbugs)
I've added a bunch of doc info to the commit message in the patch.
Attachment #8843006 - Attachment is obsolete: true
Attachment #8843006 - Flags: review?(tbsaunde+mozbugs)
Attachment #8845111 - Flags: review?(tbsaunde+mozbugs)
See the commit message for more info
Attachment #8840050 - Attachment is obsolete: true
Attachment #8840050 - Flags: review?(tbsaunde+mozbugs)
Attachment #8845137 - Flags: review?(tbsaunde+mozbugs)
This part adds a function that traverses the list of registered handlers and calls IHandlerControl::Invalidate on each one. It does so asynchronously when possible.

Right now I have it set up to invalidate each time there is a NotifyWinEvent -- this is probably not what we want, but we can improve that later; the handler is currently prefed off by default.
Attachment #8843008 - Attachment is obsolete: true
Attachment #8843008 - Flags: review?(tbsaunde+mozbugs)
Attachment #8845142 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8845111 [details] [diff] [review]
a11y changes to be able to serve a payload (r3)

># HG changeset patch
># User Aaron Klotz <aklotz@mozilla.com>
># Date 1488928796 25200
>#      Tue Mar 07 16:19:56 2017 -0700
># Node ID 181fae3b65a19d2afa687eb77fc49c4948d4601d
># Parent  96a3f8573f0306bc783abc86713c86dd8f4fde8a
>Bug 1303060: Changes to a11y to enable the serving of a COM handler; r?tbsaunde
>
>MozReview-Commit-ID: JPDqBptS7V2
>
>A general outline of the COM handler (a.k.a. the "smart proxy"):
>
>COM handlers are pieces of code that are loaded by the COM runtime along with
>a proxy and are layered above that proxy. This enables the COM handler to
>interpose itself between the caller and the proxy, thus providing the
>opportunity for the handler to manipulate an interface's method calls before
>those calls reach the proxy.
>
>Handlers are regular COM components that live in DLLs and are declared in the
>Windows registry. In order to allow for the specifying of a handler (and an
>optional payload to be sent with the proxy), the mscom library allows its
>clients to specify an implementation of the IHandlerPayload interface.

I'm not sure why this needs to be a COMish looking interface, but ok.

>IHandlerPayload consists of 5 functions:

This and the related HandlerPayload name seem pretty confusing,  I'm not exactly sure what to call this thing, HandlerManager or payloadManager maybe? but it certainly isn't either the handler itself or a payload.

>* GetHandlerPayloadSize and WriteHandlerPayload are for obtaining the payload
>  data. These calls are made on a background thread, so we must forward

calls are made on is kind of confusing wording, I guess you mean they are called there and need to do most of their work on the main thread?

>* Clone, which is needed to ensure that all descendent proxies are also imbued
>  with the same handler code.

so there is one handlerPayload object per pair of accessible and proxies to it?

>The main focus of this patch is as follows:

seems like it could have been broken up even more, but I'll live.

>+
>+IHandlerControlHolder
>+CreateHolderFromHandlerControl(mscom::ProxyUniquePtr<IHandlerControl> aHandlerControl)

I guess this actually points at a proxy to a IHandlerControl?  I do so love COM obscuring types of things.

>+{
>+  MOZ_ASSERT(aHandlerControl && NS_IsMainThread());

this only gets called in content processes right? would be good to assert things about processes as well as threads.

>+HandlerPayload::AddRef()
>+{
>+  return ++mRefCnt;
>+}
>+
>+ULONG
>+HandlerPayload::Release()
>+{
>+  ULONG result = --mRefCnt;
>+  if (!result) {
>+    delete this;
>+  }
>+  return result;
>+}

can you use the IUnknownImpl.h macros here?

>+void
>+HandlerPayload::GetAndSerializePayload(mscom::InterceptorTargetPtr<IUnknown> aTarget)
>+{

would be good to assert what thread its on, an MTA one I think?

>+  if (mSerializer) {

can you explain the need for this?

how is it syncronized?  I guess this function expects to be called with mMutex held, can you pass in a reference to the AutoLock to require / document that?

>+  // But we set mGeckoBackChannel on the current thread which resides in the
>+  // MTA. This is important to ensure that COM always invokes
>+  // IGeckoBackChannel methods in an MTA background thread.

I guess its also important we do the serialization itself on a background thread? presumably just setting a variable that's a raw pointer isn't itself the important part?

>+
>+  RefPtr<IGeckoBackChannel> payloadRef(this);
>+  // AddRef/Release pair for this reference is handled by payloadRef

why do we need the ref? presumably our caller has a ref to us, and StructToStream presumably takes one for the proxy?

>+HandlerPayload::GetHandlerPayloadSize(REFIID aIid,
>+                                      mscom::InterceptorTargetPtr<IUnknown> aTarget,
>+                                      DWORD* aOutPayloadSize)

seems to me kind of weird how getting the length and the data are different, but I guess there is a good reason in the mscom stuff for that.

>+{

assertion this is on a MTA thread?

>+  if (!aOutPayloadSize) {

assert? just wrap the arg in NonNull<> and not have the check?

>+  static_assert(&NEWEST_IA2_IID == &IID_IAccessible2_3,

I worry people will just not update NEWEST_IA2_IID, but better than nothing I guess.

>+HandlerPayload::BuildIA2Data(IA2Data* aOutIA2Data)
>+{
>+  IA2Data& data = *aOutIA2Data;

how is that useful? I'd rather not pretend this isn't indirecting through a pointer, though I guess its not a big deal.

>+
>+  // This loop is just to give us something to break out of
>+  do {

it walks quacks and looks like a goto, so just use goto ;) of course RAII would be nicer though.

>+    hr = mTarget->accLocation(&data.mLeft, &data.mTop, &data.mWidth,
>+                              &data.mHeight, kChildIdSelf);

I don't really like caching this, I don't think we have even a conservatively correct way to invalidate, but I guess we fire enough events its "likely" to usually be fine.

>+    hr = mTarget->get_accState(kChildIdSelf, &varVal);

same here, we might be able to correctly cache some states, but I'm pretty unsure about all of them.

>+    hr = mTarget->get_accKeyboardShortcut(kChildIdSelf, &data.mKeyboardShortcut);

same, I have no idea what all effects that one.

>+    hr = mTarget->get_accName(kChildIdSelf, &data.mName);

same :/ a lot of things can effect that.

>+    hr = mTarget->get_accDescription(kChildIdSelf, &data.mDescription);

same

>+    hr = mTarget->get_accValue(kChildIdSelf, &data.mValue);

same

>+    hr = mTarget->get_states(&data.mIA2States);

same

>+    hr = mTarget->get_attributes(&data.mAttributes);

same

>+    HWND hwnd;
>+    hr = mTarget->get_windowHandle(&hwnd);

not sure your current set of times to invalidate covers this, but it should certainly be cacheable.

>+    hr = mTarget->get_locale(&data.mIA2Locale);

I'm not sure how easy it would be to invalidate this.

>+    // NB: get_uniqueID should be the final property retrieved in this method,
>+    // as its presence is used to determine whether the rest of this data
>+    // retrieval was succcessful.

successful

>+HandlerPayload::WriteHandlerPayload(IStream* aStream, REFIID aIid,
>+                                    mscom::InterceptorTargetPtr<IUnknown> aTarget)
>+{

assert about threads?

>+  if (!aStream) {

assert or make it NonNull?

>+  }
>+  // Otherwise we juse return the identity.

just?

>+HandlerPayload::Clone(IHandlerPayload** aOutNewPayload)
>+{

guess it doesn't matter much now, but probably good to assert about threading anyway.

>+HRESULT
>+HandlerPayload::Refresh(IA2Data* aOutData)
>+{

assert about thread?

>+
>+private:
>+  virtual ~HandlerPayload() {}

the class is final, so I don't see a need for it to be virtual.

>+  Mutex                             mMutex;

it would be good to document what this protects, everything in the class I guess.

>+  mscom::STAUniquePtr<NEWEST_IA2_INTERFACE> mTarget;

would it make sense to clear this in BuildIA2Data() when done with it? does it even need to be a member?

>+#define NEWEST_IA2_IDL IDLFOR(NEWEST_IA2_BASENAME)
>+
>+#define __GENIFACE(base) I##base
>+#define INTERFACEFOR(base) __GENIFACE(base)
>+#define NEWEST_IA2_INTERFACE INTERFACEFOR(NEWEST_IA2_BASENAME)

it would be good to use this in accessible/windows/{ia2,msaa} as well to make it harder to not update.

>+interface IGeckoBackChannel : IUnknown
>+{
>+  [propput] HRESULT HandlerControl([in] long aPid, [in] IHandlerControl* aCtrl);
>+  HRESULT Refresh([out] IA2Data* aOutData);

so this is just refreshing the data for one particular accessible? and we have one of these objects per accessible?

>+    // This list of interfaces must be identical to all of the IIDs that are
>+    // supported by AccessibleWrap::QueryInterface(). Note that we set

I assume AccessibleWrap::QI or an override of it?  I think your missing a few, at least ISimpleDOMText and ISimpleDOMDocument of the top of my head.  Oh also IAccessibleTable{,2}

>+    // IAccessible as the default because ATs using IDispatch need to receive an
>+    // automation-compliant interface as the default.
>+    [default] interface IAccessible;
>+    interface NEWEST_IA2_INTERFACE;

don't you need to include its parents?

>diff --git a/accessible/ipc/win/handler/Makefile.in b/accessible/ipc/win/handler/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/accessible/ipc/win/handler/Makefile.in

please get this reviewed by a build system person.

>+# Since we are definining our own COM entry points (DllRegisterServer et al),

defining

>+AccessibleWrap::SetHandlerControl(DWORD aPid, RefPtr<IHandlerControl>&& aCtrl)

you always take ownership, so just make the type RefPtr<>?

>+  struct HandlerControllerData final
>+  {
>+    HandlerControllerData(DWORD aPid, RefPtr<IHandlerControl>&& aCtrl)
>+      : mPid(aPid)
>+      , mCtrl(Move(aCtrl))
>+    {
>+      mIsProxy = mozilla::mscom::IsProxy(mCtrl);

its not proxied if the consumer dll is loaded into our main process right?

>+  static DWORD sParentPid;

is this used for something? also really? windows doesn't make that cheap to get?
> >+  mscom::STAUniquePtr<NEWEST_IA2_INTERFACE> mTarget;
> 
> would it make sense to clear this in BuildIA2Data() when done with it? does
> it even need to be a member?

I guess you need it in Refresh() but that raises another question, how are  HandlerPayload objects created for things other than the top level document?
Comment on attachment 8845137 [details] [diff] [review]
First cut of the handler dll (r2)

>+uint32_t AccessibleHandler::sDistinctHandlerInstances;

this doesn't seem to be used for anything?

>+AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT& aResult)

HRESULT* please

isn't aOuter the proxy we are wrapping? saying its the outer thing when its actually the inner one seems confusing.

>+  if (SUCCEEDED(hr)) {
>+    // mIA2PassThru is a weak reference
>+    mIA2PassThru->Release();

what do we gain from it being weak? we already hold one reference why is it better to drop the one we just got now rather than later?

>+AccessibleHandler::MaybeUpdateCachedData()
>+{
>+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());

couldn't we just create it in DLLMain() and point a StaticRefPtr at it? then we could just assume its nonnull and drop the RefPtrs.

>+AccessibleHandler::ResolveIDispatch(void** aOutInterface)
>+{

do we ever pass in a pointer to something other than mDispatch?

>+  if (!mDispatchUnk) {

what is the difference between mDispatch and mDispatchUnk?

>+AccessibleHandler::QueryHandlerInterface(IUnknown* aProxyUnknown, REFIID aIid,
>+                                         void** aOutInterface)
>+{

what is this supposed to do if the proxy would implement the interface but we don't want to wrap it? looks like we return E_NOINTERFACE but unclear to me if that's correct.

>+  RefPtr<IHandlerControl> ctl;
>+  HRESULT hr = gControlFactory.CreateInstance(nullptr, IID_IHandlerControl,
>+                                              getter_AddRefs(ctl));

seems like we could write this in a way that makes it clearer the dll will only ever have one AccessiblehandlerControl object.

>+  // If we're sending the payload to somebody else, we'd better make sure that
>+  // it is up to date. If the cache update fails then we'll return a 0 payload
>+  // size so that we don't transfer obsolete data.
>+  if (FAILED(MaybeUpdateCachedData())) {
>+    *aOutPayloadSize = 0;

I thought there was a helper for empty structs? are you not using that OOP?

>+HRESULT
>+AccessibleHandler::QueryInterface(REFIID riid, void** ppv)
>+{
>+  return Handler::QueryInterface(riid, ppv);

how does that work? I don't see mscom::Handler defining an actual QueryInterface() method.

>+#define BEGIN_CACHE_ACCESS \
>+  { \
>+    HRESULT hr; \
>+    if (FAILED(hr = MaybeUpdateCachedData())) { \
>+      return hr; \
>+    } \
>+  }
>+
>+#define GET_FIELD(member, assignTo) \
>+  { \
>+    assignTo = mCachedData.mData.member; \
>+  }

I'm infavor of autogenerating as much of the following stuff as we can, but this macro doesn't seem to really make things any shorter than just writing the assignment.

>+
>+#define GET_BSTR(member, assignTo) \
>+  { \
>+    assignTo = ::SysAllocStringLen(mCachedData.mData.member, \
>+                                   ::SysStringLen(mCachedData.mData.member)); \
>+  }

similar seems like a function that handles the length thing would be nicer than this.

>+  return mIA2PassThru->get_accChild(varChild, ppdispChild);

this isn't used much? shouldn't be too bad to cache the set of children.

>+    return mIA2PassThru->get_accName(varChild, pszName);
>+  }
>+  BEGIN_CACHE_ACCESS;

not really a fan of caching this because of invalidation, but I'm not sure this is the place to fight about what we cache.
similar comments for some other things below, but I don't really want to look carefully at all that boiler plate.

>+AccessibleHandler::put_accName(VARIANT varChild, BSTR szName)
>+{
>+  // This matches AccessibleWrap

I don't really care about this one, but copying some rules about what these methods do is not great.

>+static const GUID kUnsupportedServices[] = {
>+  // Unknown, queried by Windows
>+  {0x33f139ee, 0xe509, 0x47f7, {0xbf, 0x39, 0x83, 0x76, 0x44, 0xf7, 0x45, 0x76}},
>+  IID_IAccessibleEx,

so, there is a pref to support that one, so not all that great to force disable it here.

>+  AccessibleHandler(IUnknown* aOuter, HRESULT& aResult);
>+  ~AccessibleHandler();

does this need to be virtual? or where is it deleted?

>+  RefPtr<IUnknown>                  mDispatchUnk;
>+  IDispatch*                        mDispatch;         // weak
>+  NEWEST_IA2_INTERFACE*             mIA2PassThru;      // weak

wouldn't one RefPtr to the ia2 interface work as well? assuming they point at the same thing, or is that not the case?

>+  IServiceProvider*                 mServProvPassThru; // weak

can you document why they need to be weak / why they will stay valid?

>+mscom::SingletonFactory<AccessibleHandlerControl> gControlFactory;

why do we need to implement IClassFactory? afaict we never expose this object to anyone else.  So it seems like a StaitcRefPtr would be enough.  However I'm not really clear on how the AccessibleHandlerControl is cleaned up / if gecko gets notified of that.

>--- a/accessible/ipc/win/handler/moz.build
>+++ b/accessible/ipc/win/handler/moz.build

please get someone who knows build system things to look at this part.
Attachment #8845137 - Flags: review?(tbsaunde+mozbugs)
Attachment #8845111 - Flags: review?(tbsaunde+mozbugs)
Attachment #8845142 - Flags: review?(tbsaunde+mozbugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #23)
> Comment on attachment 8845111 [details] [diff] [review]
> a11y changes to be able to serve a payload (r3)
> 
> 
> I'm not sure why this needs to be a COMish looking interface, but ok.
> 

This is because COM invokes methods on an implementation of IMarshal that is provided in the mscom library. This implementation then invokes the simplified IHandlerPayload to request payload information. So it is that way essentially to minimize impedance mismatch between IHandlerPayload and IMarshal.

> >IHandlerPayload consists of 5 functions:
> 
> This and the related HandlerPayload name seem pretty confusing,  I'm not
> exactly sure what to call this thing, HandlerManager or payloadManager
> maybe? but it certainly isn't either the handler itself or a payload.
> 

I have renamed it to IHandlerProvider.

> >* GetHandlerPayloadSize and WriteHandlerPayload are for obtaining the payload
> >  data. These calls are made on a background thread, so we must forward
> 
> calls are made on is kind of confusing wording, I guess you mean they are
> called there and need to do most of their work on the main thread?

Yes.

> 
> >* Clone, which is needed to ensure that all descendent proxies are also imbued
> >  with the same handler code.
> 
> so there is one handlerPayload object per pair of accessible and proxies to
> it?
> 

Yes.

> 
> >+
> >+IHandlerControlHolder
> >+CreateHolderFromHandlerControl(mscom::ProxyUniquePtr<IHandlerControl> aHandlerControl)
> 
> I guess this actually points at a proxy to a IHandlerControl?  I do so love
> COM obscuring types of things.

Yes.
> 
> >+{
> >+  MOZ_ASSERT(aHandlerControl && NS_IsMainThread());
> 
> this only gets called in content processes right? would be good to assert
> things about processes as well as threads.

Sure, done.

> 
> >+HandlerPayload::AddRef()
> >+{
> >+  return ++mRefCnt;
> >+}
> >+
> >+ULONG
> >+HandlerPayload::Release()
> >+{
> >+  ULONG result = --mRefCnt;
> >+  if (!result) {
> >+    delete this;
> >+  }
> >+  return result;
> >+}
> 
> can you use the IUnknownImpl.h macros here?

Isn't thread safe. :-(

> 
> >+void
> >+HandlerPayload::GetAndSerializePayload(mscom::InterceptorTargetPtr<IUnknown> aTarget)
> >+{
> 
> would be good to assert what thread its on, an MTA one I think?

Done

> 
> >+  if (mSerializer) {
> 
> can you explain the need for this?
Added comments above the code.

> 
> how is it syncronized?  I guess this function expects to be called with
> mMutex held, can you pass in a reference to the AutoLock to require /
> document that?

Done

> 
> >+  // But we set mGeckoBackChannel on the current thread which resides in the
> >+  // MTA. This is important to ensure that COM always invokes
> >+  // IGeckoBackChannel methods in an MTA background thread.
> 
> I guess its also important we do the serialization itself on a background
> thread? presumably just setting a variable that's a raw pointer isn't itself
> the important part?

Yeah, COM assigns apartments based on which apartment the proxy was marshaled in. So if we want IGeckoBackChannel's RPCs to enter our process on an MTA thread, we need IGeckoBackChannel to be presented to COM on an MTA thread.

> 
> >+
> >+  RefPtr<IGeckoBackChannel> payloadRef(this);
> >+  // AddRef/Release pair for this reference is handled by payloadRef
> 
> why do we need the ref? presumably our caller has a ref to us, and
> StructToStream presumably takes one for the proxy?

Presumably, but since the _Encode and _Decode functions are opaque to us, I felt that it was safest to make that reference a strong reference to ensure that the refcount is stabilized lest those functions try something funny.
> 
> >+HandlerPayload::GetHandlerPayloadSize(REFIID aIid,
> >+                                      mscom::InterceptorTargetPtr<IUnknown> aTarget,
> >+                                      DWORD* aOutPayloadSize)
> 
> seems to me kind of weird how getting the length and the data are different,
> but I guess there is a good reason in the mscom stuff for that.

As discussed on IRC, I have simplified this interface.

> 
> >+{
> 
> assertion this is on a MTA thread?

Done.

> 
> >+  if (!aOutPayloadSize) {
> 
> assert? just wrap the arg in NonNull<> and not have the check?

added assertion

> 
> >+  static_assert(&NEWEST_IA2_IID == &IID_IAccessible2_3,
> 
> I worry people will just not update NEWEST_IA2_IID, but better than nothing
> I guess.
> 
> >+HandlerPayload::BuildIA2Data(IA2Data* aOutIA2Data)
> >+{
> >+  IA2Data& data = *aOutIA2Data;
> 
> how is that useful? I'd rather not pretend this isn't indirecting through a
> pointer, though I guess its not a big deal.

I removed that

> 
> >+
> >+  // This loop is just to give us something to break out of
> >+  do {
> 
> it walks quacks and looks like a goto, so just use goto ;) of course RAII
> would be nicer though.

Yeah for now I've removed all of that (since for the moment we're now only caching uniqueID). I'll switch it to RAII when things get more complex later.

> >+    // NB: get_uniqueID should be the final property retrieved in this method,
> >+    // as its presence is used to determine whether the rest of this data
> >+    // retrieval was succcessful.
> 
> successful

fixed

> 
> >+HandlerPayload::WriteHandlerPayload(IStream* aStream, REFIID aIid,
> >+                                    mscom::InterceptorTargetPtr<IUnknown> aTarget)
> >+{
> 
> assert about threads?

added assertion

> 
> >+  if (!aStream) {
> 
> assert or make it NonNull?
> 
added assertion

> >+  }
> >+  // Otherwise we juse return the identity.
> 
> just?
> 

fixed


> >+HRESULT
> >+HandlerPayload::Refresh(IA2Data* aOutData)
> >+{
> 
> assert about thread?

done

> 
> >+
> >+private:
> >+  virtual ~HandlerPayload() {}
> 
> the class is final, so I don't see a need for it to be virtual.

fixed
> 
> >+  Mutex                             mMutex;
> 
> it would be good to document what this protects, everything in the class I
> guess.

commented

> 
> >+  mscom::STAUniquePtr<NEWEST_IA2_INTERFACE> mTarget;
> 
> would it make sense to clear this in BuildIA2Data() when done with it? does
> it even need to be a member?

made local

> 
> >+#define NEWEST_IA2_IDL IDLFOR(NEWEST_IA2_BASENAME)
> >+
> >+#define __GENIFACE(base) I##base
> >+#define INTERFACEFOR(base) __GENIFACE(base)
> >+#define NEWEST_IA2_INTERFACE INTERFACEFOR(NEWEST_IA2_BASENAME)
> 
> it would be good to use this in accessible/windows/{ia2,msaa} as well to
> make it harder to not update.

Probably a good idea, but gonna punt on this for now.

> 
> >+interface IGeckoBackChannel : IUnknown
> >+{
> >+  [propput] HRESULT HandlerControl([in] long aPid, [in] IHandlerControl* aCtrl);
> >+  HRESULT Refresh([out] IA2Data* aOutData);
> 
> so this is just refreshing the data for one particular accessible? and we
> have one of these objects per accessible?

Yeah, one per proxy. mozilla::a11y::HandlerProvider implements IGeckoBackChannel as well as IHandlerProvider.

> 
> >+    // This list of interfaces must be identical to all of the IIDs that are
> >+    // supported by AccessibleWrap::QueryInterface(). Note that we set
> 
> I assume AccessibleWrap::QI or an override of it?  I think your missing a
> few, at least ISimpleDOMText and ISimpleDOMDocument of the top of my head. 
> Oh also IAccessibleTable{,2}
> 
> >+    // IAccessible as the default because ATs using IDispatch need to receive an
> >+    // automation-compliant interface as the default.
> >+    [default] interface IAccessible;
> >+    interface NEWEST_IA2_INTERFACE;
> 
> don't you need to include its parents?

Parents are implicitly included. IAccessible is a special case because it needs to be marked [default]. The real purpose of this is so that interpreted language FFIs using IDispatch can query our typelib for supported interfaces. For example, NVDA's python stuff uses typelib info to figure out how to get from IDispatch to IAccessible. We probably don't need all of them here, we just need whatever ones help them to resolve IAccessible.

We can probably reduce this down to IAccessible, IEnumVARIANT, IProvideClassInfo, and IServiceProvider.

> 
> >diff --git a/accessible/ipc/win/handler/Makefile.in b/accessible/ipc/win/handler/Makefile.in
> >new file mode 100644
> >--- /dev/null
> >+++ b/accessible/ipc/win/handler/Makefile.in
> 
> please get this reviewed by a build system person.

Sure, I've separated out the build system changes to separate patches.

> 
> >+# Since we are definining our own COM entry points (DllRegisterServer et al),
> 
> defining

Fixed

> 
> >+AccessibleWrap::SetHandlerControl(DWORD aPid, RefPtr<IHandlerControl>&& aCtrl)
> 
> you always take ownership, so just make the type RefPtr<>?

Fixed

> 
> >+  struct HandlerControllerData final
> >+  {
> >+    HandlerControllerData(DWORD aPid, RefPtr<IHandlerControl>&& aCtrl)
> >+      : mPid(aPid)
> >+      , mCtrl(Move(aCtrl))
> >+    {
> >+      mIsProxy = mozilla::mscom::IsProxy(mCtrl);
> 
> its not proxied if the consumer dll is loaded into our main process right?

Correct. But in general we cannot assume that the consumer is in our main process, hence the check.

> 
> >+  static DWORD sParentPid;
> 
> is this used for something? also really? windows doesn't make that cheap to
> get?

It's cheap to get. I've removed this.

(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> Comment on attachment 8845137 [details] [diff] [review]
> First cut of the handler dll (r2)
> 
> >+uint32_t AccessibleHandler::sDistinctHandlerInstances;
> 
> this doesn't seem to be used for anything?

It's gone for the moment, though it may need to come back.

> 
> >+AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT& aResult)
> 
> HRESULT* please

Fixed

> 
> isn't aOuter the proxy we are wrapping? saying its the outer thing when its
> actually the inner one seems confusing.

It's definitely the outer. Handlers are kind of weird because they wrap a proxy, but they are also wrapped themselves by this COM "identity object." So aOuter is actually the identity object that everybody is delegating their IUnknown to.

> 
> >+  if (SUCCEEDED(hr)) {
> >+    // mIA2PassThru is a weak reference
> >+    mIA2PassThru->Release();
> 
> what do we gain from it being weak? we already hold one reference why is it
> better to drop the one we just got now rather than later?

I've added documentation in AccessibleHandler.h on this. Basically because the proxies are aggregated, only the original IUnknown is a strong reference to the proxy. Any interfaces queried off of that proxy use delegated unknowns whose references, if held strongly by us, would create a cycle.

> 
> >+AccessibleHandler::MaybeUpdateCachedData()
> >+{
> >+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> 
> couldn't we just create it in DLLMain() and point a StaticRefPtr at it? then
> we could just assume its nonnull and drop the RefPtrs.

Doing non-trivial stuff in DLLMain is generally verboten save for a few kernel32 APIs, as it is called while its thread holds the process loader lock.

> 
> >+AccessibleHandler::ResolveIDispatch(void** aOutInterface)
> >+{
> 
> do we ever pass in a pointer to something other than mDispatch?

I cleaned this up.

> 
> >+  if (!mDispatchUnk) {
> 
> what is the difference between mDispatch and mDispatchUnk?

mDispatchUnk is the "real" IUnknown of the aggregated IDispatch object. mDispatch is the IDispatch that is actually callable, but has a delegated IUnknown.

> 
> >+AccessibleHandler::QueryHandlerInterface(IUnknown* aProxyUnknown, REFIID aIid,
> >+                                         void** aOutInterface)
> >+{
> 
> what is this supposed to do if the proxy would implement the interface but
> we don't want to wrap it? looks like we return E_NOINTERFACE but unclear to
> me if that's correct.

Yep, this is called by ipc/mscom/Handler.cpp. If we return E_NOINTERFACE here, the handler code tries the proxy directly.


> 
> >+  RefPtr<IHandlerControl> ctl;
> >+  HRESULT hr = gControlFactory.CreateInstance(nullptr, IID_IHandlerControl,
> >+                                              getter_AddRefs(ctl));
> 
> seems like we could write this in a way that makes it clearer the dll will
> only ever have one AccessiblehandlerControl object.

I'd like to, but we don't control the lifetime of this DLL, COM does via the DllCanUnloadNow entry point. Any live objects need to be plugged into the code that manages that so that DllCanUnloadNow returns the correct result when queried.

> 
> >+  // If we're sending the payload to somebody else, we'd better make sure that
> >+  // it is up to date. If the cache update fails then we'll return a 0 payload
> >+  // size so that we don't transfer obsolete data.
> >+  if (FAILED(MaybeUpdateCachedData())) {
> >+    *aOutPayloadSize = 0;
> 
> I thought there was a helper for empty structs? are you not using that OOP?

We can. Fixed.

> 
> >+HRESULT
> >+AccessibleHandler::QueryInterface(REFIID riid, void** ppv)
> >+{
> >+  return Handler::QueryInterface(riid, ppv);
> 
> how does that work? I don't see mscom::Handler defining an actual
> QueryInterface() method.

It's because mscom::Handler is aggregated by the COM identity object. The IUnknown methods are implemented within the DECLARE_AGGREGATABLE macro.

> 
> >+#define BEGIN_CACHE_ACCESS \
> >+  { \
> >+    HRESULT hr; \
> >+    if (FAILED(hr = MaybeUpdateCachedData())) { \
> >+      return hr; \
> >+    } \
> >+  }
> >+
> >+#define GET_FIELD(member, assignTo) \
> >+  { \
> >+    assignTo = mCachedData.mData.member; \
> >+  }
> 
> I'm infavor of autogenerating as much of the following stuff as we can, but
> this macro doesn't seem to really make things any shorter than just writing
> the assignment.

Sure, I removed GET_FIELD.

> 
> >+
> >+#define GET_BSTR(member, assignTo) \
> >+  { \
> >+    assignTo = ::SysAllocStringLen(mCachedData.mData.member, \
> >+                                   ::SysStringLen(mCachedData.mData.member)); \
> >+  }
> 
> similar seems like a function that handles the length thing would be nicer
> than this.

Changed.

> 
> >+static const GUID kUnsupportedServices[] = {
> >+  // Unknown, queried by Windows
> >+  {0x33f139ee, 0xe509, 0x47f7, {0xbf, 0x39, 0x83, 0x76, 0x44, 0xf7, 0x45, 0x76}},
> >+  IID_IAccessibleEx,
> 
> so, there is a pref to support that one, so not all that great to force
> disable it here.

Removed.

> 
> >+  AccessibleHandler(IUnknown* aOuter, HRESULT& aResult);
> >+  ~AccessibleHandler();
> 
> does this need to be virtual? or where is it deleted?

It is virtual by virtue of inheriting from mscom::Handler, but I have changed this to be explicit.

> 
> >+  RefPtr<IUnknown>                  mDispatchUnk;
> >+  IDispatch*                        mDispatch;         // weak
> >+  NEWEST_IA2_INTERFACE*             mIA2PassThru;      // weak
> 
> wouldn't one RefPtr to the ia2 interface work as well? assuming they point
> at the same thing, or is that not the case?
> 
> >+  IServiceProvider*                 mServProvPassThru; // weak
> 
> can you document why they need to be weak / why they will stay valid?

I've added a big comment above the weak pointer declarations to try to explain this.

> 
> >+mscom::SingletonFactory<AccessibleHandlerControl> gControlFactory;
> 
> why do we need to implement IClassFactory? afaict we never expose this
> object to anyone else.  So it seems like a StaitcRefPtr would be enough. 
> However I'm not really clear on how the AccessibleHandlerControl is cleaned
> up / if gecko gets notified of that.

Again, to ensure that the lifetime of the object is plugged into DllCanUnloadNow().

> 
> >--- a/accessible/ipc/win/handler/moz.build
> >+++ b/accessible/ipc/win/handler/moz.build
> 
> please get someone who knows build system things to look at this part.

Lifted to a separate patch.
Attachment #8845111 - Attachment is obsolete: true
Attachment #8852241 - Flags: review?(tbsaunde+mozbugs)
Attachment #8845137 - Attachment is obsolete: true
Attachment #8852245 - Flags: review?(tbsaunde+mozbugs)
Attachment #8845142 - Attachment is obsolete: true
Attachment #8852247 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8852244 [details] [diff] [review]
a11y changes to be able to serve a payload (build system)

I don't have the build system paged back into my brain yet. So punting this off to mshal.
Attachment #8852244 - Flags: review?(gps) → review?(mshal)
Comment on attachment 8852246 [details] [diff] [review]
First cut of the handler dll (build system)

This looks innocuous enough that I could probably review it. But since I already gave mshal one of the reviews, I feel it is better to have N-1 reviewers in total.
Attachment #8852246 - Flags: review?(gps) → review?(mshal)
(In reply to Aaron Klotz [:aklotz] from comment #26)
> (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > Comment on attachment 8845111 [details] [diff] [review]
> > a11y changes to be able to serve a payload (r3)
> > 
> > 
> > I'm not sure why this needs to be a COMish looking interface, but ok.
> > 
> 
> This is because COM invokes methods on an implementation of IMarshal that is
> provided in the mscom library. This implementation then invokes the
> simplified IHandlerPayload to request payload information. So it is that way
> essentially to minimize impedance mismatch between IHandlerPayload and
> IMarshal.

ok, I guess that is fair to a point.

> > 
> > >+HandlerPayload::AddRef()
> > >+{
> > >+  return ++mRefCnt;
> > >+}
> > >+
> > >+ULONG
> > >+HandlerPayload::Release()
> > >+{
> > >+  ULONG result = --mRefCnt;
> > >+  if (!result) {
> > >+    delete this;
> > >+  }
> > >+  return result;
> > >+}
> > 
> > can you use the IUnknownImpl.h macros here?
> 
> Isn't thread safe. :-(

we can certainly add more, and we have plenty of thread safe refcounting now.

> > 
> > >+
> > >+  RefPtr<IGeckoBackChannel> payloadRef(this);
> > >+  // AddRef/Release pair for this reference is handled by payloadRef
> > 
> > why do we need the ref? presumably our caller has a ref to us, and
> > StructToStream presumably takes one for the proxy?
> 
> Presumably, but since the _Encode and _Decode functions are opaque to us, I
> felt that it was safest to make that reference a strong reference to ensure
> that the refcount is stabilized lest those functions try something funny.

ok, fair enough.

> > >+
> > >+  // This loop is just to give us something to break out of
> > >+  do {
> > 
> > it walks quacks and looks like a goto, so just use goto ;) of course RAII
> > would be nicer though.
> 
> Yeah for now I've removed all of that (since for the moment we're now only
> caching uniqueID). I'll switch it to RAII when things get more complex later.

fair enough.

> > 
> > >+#define NEWEST_IA2_IDL IDLFOR(NEWEST_IA2_BASENAME)
> > >+
> > >+#define __GENIFACE(base) I##base
> > >+#define INTERFACEFOR(base) __GENIFACE(base)
> > >+#define NEWEST_IA2_INTERFACE INTERFACEFOR(NEWEST_IA2_BASENAME)
> > 
> > it would be good to use this in accessible/windows/{ia2,msaa} as well to
> > make it harder to not update.
> 
> Probably a good idea, but gonna punt on this for now.

yeah, its an independent change so best to keep it separate, just want to add it to a todo list somewhere.

> > >+interface IGeckoBackChannel : IUnknown
> > >+{
> > >+  [propput] HRESULT HandlerControl([in] long aPid, [in] IHandlerControl* aCtrl);
> > >+  HRESULT Refresh([out] IA2Data* aOutData);
> > 
> > so this is just refreshing the data for one particular accessible? and we
> > have one of these objects per accessible?
> 
> Yeah, one per proxy. mozilla::a11y::HandlerProvider implements
> IGeckoBackChannel as well as IHandlerProvider.

ok, this setup keeps confusing me with all its classes, but I'm not yet sure how to improve that.

> > >+    // This list of interfaces must be identical to all of the IIDs that are
> > >+    // supported by AccessibleWrap::QueryInterface(). Note that we set
> > 
> > I assume AccessibleWrap::QI or an override of it?  I think your missing a
> > few, at least ISimpleDOMText and ISimpleDOMDocument of the top of my head. 
> > Oh also IAccessibleTable{,2}
> > 
> > >+    // IAccessible as the default because ATs using IDispatch need to receive an
> > >+    // automation-compliant interface as the default.
> > >+    [default] interface IAccessible;
> > >+    interface NEWEST_IA2_INTERFACE;
> > 
> > don't you need to include its parents?
> 
> Parents are implicitly included. IAccessible is a special case because it
> needs to be marked [default]. The real purpose of this is so that
> interpreted language FFIs using IDispatch can query our typelib for
> supported interfaces. For example, NVDA's python stuff uses typelib info to
> figure out how to get from IDispatch to IAccessible. We probably don't need
> all of them here, we just need whatever ones help them to resolve
> IAccessible.
> 
> We can probably reduce this down to IAccessible, IEnumVARIANT,
> IProvideClassInfo, and IServiceProvider.

What about the IAccessibleTable question?

> > >+  struct HandlerControllerData final
> > >+  {
> > >+    HandlerControllerData(DWORD aPid, RefPtr<IHandlerControl>&& aCtrl)
> > >+      : mPid(aPid)
> > >+      , mCtrl(Move(aCtrl))
> > >+    {
> > >+      mIsProxy = mozilla::mscom::IsProxy(mCtrl);
> > 
> > its not proxied if the consumer dll is loaded into our main process right?
> 
> Correct. But in general we cannot assume that the consumer is in our main
> process, hence the check.

I think my question more was what does that variable mean.

> > >+  static DWORD sParentPid;
> > 
> > is this used for something? also really? windows doesn't make that cheap to
> > get?
> 
> It's cheap to get. I've removed this.

ok, I'm never sure with windows.

> > >+uint32_t AccessibleHandler::sDistinctHandlerInstances;
> > 
> > this doesn't seem to be used for anything?
> 
> It's gone for the moment, though it may need to come back.

ok, then atleast we'll have it with some idea what its for.

> > 
> > isn't aOuter the proxy we are wrapping? saying its the outer thing when its
> > actually the inner one seems confusing.
> 
> It's definitely the outer. Handlers are kind of weird because they wrap a
> proxy, but they are also wrapped themselves by this COM "identity object."
> So aOuter is actually the identity object that everybody is delegating their
> IUnknown to.

oh, lovely

> > >+  if (SUCCEEDED(hr)) {
> > >+    // mIA2PassThru is a weak reference
> > >+    mIA2PassThru->Release();
> > 
> > what do we gain from it being weak? we already hold one reference why is it
> > better to drop the one we just got now rather than later?
> 
> I've added documentation in AccessibleHandler.h on this. Basically because
> the proxies are aggregated, only the original IUnknown is a strong reference
> to the proxy. Any interfaces queried off of that proxy use delegated
> unknowns whose references, if held strongly by us, would create a cycle.

hrm ugh, I don't think I want to think about why that's a cycle.

> > >+AccessibleHandler::MaybeUpdateCachedData()
> > >+{
> > >+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> > 
> > couldn't we just create it in DLLMain() and point a StaticRefPtr at it? then
> > we could just assume its nonnull and drop the RefPtrs.
> 
> Doing non-trivial stuff in DLLMain is generally verboten save for a few
> kernel32 APIs, as it is called while its thread holds the process loader
> lock.

I... see

> > 
> > >+  if (!mDispatchUnk) {
> > 
> > what is the difference between mDispatch and mDispatchUnk?
> 
> mDispatchUnk is the "real" IUnknown of the aggregated IDispatch object.
> mDispatch is the IDispatch that is actually callable, but has a delegated
> IUnknown.

so more agrigation stuff, I think I'll just leave it at that.

> > >+AccessibleHandler::QueryHandlerInterface(IUnknown* aProxyUnknown, REFIID aIid,
> > >+                                         void** aOutInterface)
> > >+{
> > 
> > what is this supposed to do if the proxy would implement the interface but
> > we don't want to wrap it? looks like we return E_NOINTERFACE but unclear to
> > me if that's correct.
> 
> Yep, this is called by ipc/mscom/Handler.cpp. If we return E_NOINTERFACE
> here, the handler code tries the proxy directly.

ok

> > >+  RefPtr<IHandlerControl> ctl;
> > >+  HRESULT hr = gControlFactory.CreateInstance(nullptr, IID_IHandlerControl,
> > >+                                              getter_AddRefs(ctl));
> > 
> > seems like we could write this in a way that makes it clearer the dll will
> > only ever have one AccessiblehandlerControl object.
> 
> I'd like to, but we don't control the lifetime of this DLL, COM does via the
> DllCanUnloadNow entry point. Any live objects need to be plugged into the
> code that manages that so that DllCanUnloadNow returns the correct result
> when queried.

if there's no easier way to match up the lifetimes ok I guess.

> > 
> > >+mscom::SingletonFactory<AccessibleHandlerControl> gControlFactory;
> > 
> > why do we need to implement IClassFactory? afaict we never expose this
> > object to anyone else.  So it seems like a StaitcRefPtr would be enough. 
> > However I'm not really clear on how the AccessibleHandlerControl is cleaned
> > up / if gecko gets notified of that.
> 
> Again, to ensure that the lifetime of the object is plugged into
> DllCanUnloadNow().

I'm not sure how that works, but ok.
Comment on attachment 8852241 [details] [diff] [review]
a11y changes to be able to serve a payload (r4)

>IHandlerPayload consists of 5 functions:

that should be updated right?

>1. Provide an implementation of the IHandlerPayload interface;

same

>+HandlerProvider::GetHandler(CLSID* aHandlerClsid)
>+{
>+  if (!aHandlerClsid) {
>+    return E_INVALIDARG;
>+  }

we control the callers right? can we just require this to be nonnull and mark it as such?

>+  if (!IsTargetInterfaceCacheable()) {

It seems like it would be good both for sanity and perf to not create these objects for uncachable interfaces in the first place.

>+
>+  RefPtr<IGeckoBackChannel> payloadRef(this);
>+  // AddRef/Release pair for this reference is handled by payloadRef

payloadRef is some sort of smart ptr that deals with proxying stuff and comes from windows?

>+HandlerProvider::GetHandlerPayloadSize(DWORD* aOutPayloadSize)
>+{
>+  if (!aOutPayloadSize) {

require nonnull? or is this easier because of what com can do?

>+  RefPtr<NEWEST_IA2_INTERFACE>
>+    target(static_cast<NEWEST_IA2_INTERFACE*>(mTargetUnk.get()));

it would be good to have a pointer to a raw AccessibleWrap*, that should help perf some I would think.

>+
>+  IA2Data& data = *aOutIA2Data;
I thought you got rid of that?

>+HandlerProvider::WriteHandlerPayload(IStream* aStream)
>+{
>+  if (!aStream) {
>+    return E_INVALIDARG;

require nonnull? or is it easier to check here?

>+HandlerProvider::NewInstance(REFIID aIid,
>+                             mscom::InterceptorTargetPtr<IUnknown> aTarget,
>+                             mscom::IHandlerProvider** aOutNewPayload)

any reason to make this a method instead of a static method?

>+{
>+  RefPtr<IHandlerProvider> newPayload(new HandlerProvider(aIid, Move(aTarget)));
>+  newPayload.forget(aOutNewPayload);
>+  return S_OK;

just return a Already_AddRefed?

>+HandlerProvider::Refresh(IA2Data* aOutData)
>+{

assert is MTA?

>+
>+namespace mscom {
>+
>+class AgileReference;

what is this about?

>+  Atomic<uint32_t>                  mRefCnt;
>+  Mutex                             mMutex; // Protects mSerializer
>+  REFIID                            mTargetUnkIid;

we only check if that gets marshaled as newest ia2 interface to see if its cached right?  Seems like we could either use a bool or ideally not create objects for uncachable things and not need it.

>+  mscom::InterceptorTargetPtr<IUnknown> mTargetUnk;

seems like we should note this and the iid are constant and main thread only.

>+  UniquePtr<mscom::StructToStream>  mSerializer;

would it make sense to use a Maybe<StructToStream> instead?

>+library AccessibleHandlerTypeLib
>+{
>+  [uuid(HANDLER_CLSID)]
>+  coclass AccessibleHandler
>+  {
>+    // This list of interfaces should generally resolve to any interface that
>+    // could be used to resolve accessibles given an IDispatch. Note that we
>+    // set IAccessible as the default because ATs using IDispatch need to
>+    // receive an automation-compliant interface as the default.

please remember to work on the comment here.

>+  static StaticAutoPtr<nsTArray<HandlerControllerData>> sHandlerControllers;
>+  static DWORD sParentPid;

you should remove that right?

all of this seems simple or like it can get fixed later so r=me
Attachment #8852241 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8852244 [details] [diff] [review]
a11y changes to be able to serve a payload (build system)

Guess we still need to fix bug 1256013 :/
Attachment #8852244 - Flags: review?(mshal) → review+
Attachment #8852246 - Flags: review?(mshal) → review+
Comment on attachment 8852245 [details] [diff] [review]
First cut of the handler dll (r3)

>  midl. It should essentially be the inverse of the serialization code in the
>  IHandlerPayload implementation inside Gecko.

update?

>+AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT* aResult)
>+  : mscom::Handler(aOuter, aResult)
>+  , mDispatch(nullptr)
>+  , mIA2PassThru(nullptr)
>+  , mServProvPassThru(nullptr)
>+  , mCachedData()
>+  , mCacheGen(0)
>+{
>+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());

this is just so it gets created the first time a handler is created right?

>+  MOZ_ASSERT(ctl);
>+  if (!ctl) {
>+    return;

shouldn't you set aHR here?

>+AccessibleHandler::~AccessibleHandler()
>+{
>+  if (mCachedData.mGeckoBackChannel) {

when is it null?

>+AccessibleHandler::MaybeUpdateCachedData()
>+{
>+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());

there's no reason we need a ref here right? the dll can't somehow be unloaded with this on the stack can it?  and it should have been created when this object's ctor was called.

>+  if (!mDispatchUnk) {
>+    RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());

same

>+    if (!ctl) {

is this just being defensive?

>+AccessibleHandler::put_accName(VARIANT varChild, BSTR szName)
>+{
>+  // This matches AccessibleWrap
>+  return E_NOTIMPL;

I doubt this one will change, but this dupplication is really going to suck as we add more.

>+AccessibleHandler::get_uniqueID(long* uniqueID)
>+{
>+  if (!uniqueID) {
>+    return E_INVALIDARG;
>+  }
>+  if (!HasPayload()) {

when does that happen? I forget if you explained somewhere sorry.

>+    HRESULT hr = ResolveIA2();
>+    if (FAILED(hr)) {
>+      return hr;
>+    }
>+    return mIA2PassThru->get_uniqueID(uniqueID);
>+  }
>+  BEGIN_CACHE_ACCESS;

is that necessary? it should never be necessary to invalidate or update the unique id since it should be constant for the proxied object right?

>+AccessibleHandler::GetClassInfo(ITypeInfo** aOutTypeInfo)
>+{
>+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());

we don't need a ref here either for similar reasons right? though I doubt perf really matters.

>+  /**
>+   * Handlers aggregate their proxies. This means that their proxies delegate
>+   * their IUnknown implementation to us.
>+   *
>+   * mDispatchUnk and the result of Handler::GetProxy() are both strong
>+   * references to the aggregated objects. OTOH, any interfaces that are QI'd
>+   * from those aggregated objects have delegated unknowns.
>+   *
>+   * AddRef'ing an interface with a delegated unknown ends up incrementing the
>+   * refcount of the *aggregator*. Since we are the aggregator of mDispatchUnk
>+   * and of the wrapped proxy, holding a strong reference to any interfaces
>+   * QI'd off of those objects would create a reference cycle.
>+   *
>+   * We may hold onto pointers to those references, but when we query them we
>+   * must immediately Release() them to prevent these cycles.

there not really cycles as much as refs to ourself right? or are they to that identity object thing?

maybe worth explicitly calling out this is safe because the raw pointers point to the agrigated things we already have a ref to?
Attachment #8852245 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8852240 - Flags: review?(jmathies) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #35)
> (In reply to Aaron Klotz [:aklotz] from comment #26)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > > >+  struct HandlerControllerData final
> > > >+  {
> > > >+    HandlerControllerData(DWORD aPid, RefPtr<IHandlerControl>&& aCtrl)
> > > >+      : mPid(aPid)
> > > >+      , mCtrl(Move(aCtrl))
> > > >+    {
> > > >+      mIsProxy = mozilla::mscom::IsProxy(mCtrl);
> > > 
> > > its not proxied if the consumer dll is loaded into our main process right?
> > 
> > Correct. But in general we cannot assume that the consumer is in our main
> > process, hence the check.
> 
> I think my question more was what does that variable mean.

"Is aCtrl referring to an in-proc object or an out-of-proc object?" If it's in-proc (ie, not proxied), we just make the virtual function call. If it's out of proc, we do some magic to make the IPC call asynchronous.

(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> Comment on attachment 8852241 [details] [diff] [review]
> a11y changes to be able to serve a payload (r4)
> 
> >IHandlerPayload consists of 5 functions:
> 
> that should be updated right?

fixed

> 
> >1. Provide an implementation of the IHandlerPayload interface;
> 
> same

fixed

> 
> >+HandlerProvider::GetHandler(CLSID* aHandlerClsid)
> >+{
> >+  if (!aHandlerClsid) {
> >+    return E_INVALIDARG;
> >+  }
> 
> we control the callers right? can we just require this to be nonnull and
> mark it as such?

Yeah, I can do that.

> 
> >+  if (!IsTargetInterfaceCacheable()) {
> 
> It seems like it would be good both for sanity and perf to not create these
> objects for uncachable interfaces in the first place.

In general we don't. This is more for the case where the object implements both cacheable and non-cacheable interfaces, we've already proxied a cacheable interface off this object, and now we're proxying a non-cacheable interface.

> 
> >+
> >+  RefPtr<IGeckoBackChannel> payloadRef(this);
> >+  // AddRef/Release pair for this reference is handled by payloadRef
> 
> payloadRef is some sort of smart ptr that deals with proxying stuff and
> comes from windows?

It's just a mozilla RefPtr.

> 
> >+HandlerProvider::GetHandlerPayloadSize(DWORD* aOutPayloadSize)
> >+{
> >+  if (!aOutPayloadSize) {
> 
> require nonnull? or is this easier because of what com can do?

Fixed.

> >+
> >+  IA2Data& data = *aOutIA2Data;
> I thought you got rid of that?

Should have. Looks like that got lost somewhere. Fixed.

> 
> >+HandlerProvider::WriteHandlerPayload(IStream* aStream)
> >+{
> >+  if (!aStream) {
> >+    return E_INVALIDARG;
> 
> require nonnull? or is it easier to check here?

Fixed.

> 
> >+HandlerProvider::NewInstance(REFIID aIid,
> >+                             mscom::InterceptorTargetPtr<IUnknown> aTarget,
> >+                             mscom::IHandlerProvider** aOutNewPayload)
> 
> any reason to make this a method instead of a static method?

mscom is calling this on a generic IHandlerProvider without any awareness of the concrete class.

> >+HandlerProvider::Refresh(IA2Data* aOutData)
> >+{
> 
> assert is MTA?
> 

Fixed

> >+
> >+namespace mscom {
> >+
> >+class AgileReference;
> 
> what is this about?

Ah, vestigial from a previous revision. Removed.

> 
> >+  Atomic<uint32_t>                  mRefCnt;
> >+  Mutex                             mMutex; // Protects mSerializer
> >+  REFIID                            mTargetUnkIid;
> 
> we only check if that gets marshaled as newest ia2 interface to see if its
> cached right?  Seems like we could either use a bool or ideally not create
> objects for uncachable things and not need it.

The problem is that if the initial interface is not cacheable, we still need to know what to do if the client queries an interface that is.

> 
> >+  mscom::InterceptorTargetPtr<IUnknown> mTargetUnk;
> 
> seems like we should note this and the iid are constant and main thread only.
> 

Done

> >+  UniquePtr<mscom::StructToStream>  mSerializer;
> 
> would it make sense to use a Maybe<StructToStream> instead?

I'll think about this.

> 
> >+library AccessibleHandlerTypeLib
> >+{
> >+  [uuid(HANDLER_CLSID)]
> >+  coclass AccessibleHandler
> >+  {
> >+    // This list of interfaces should generally resolve to any interface that
> >+    // could be used to resolve accessibles given an IDispatch. Note that we
> >+    // set IAccessible as the default because ATs using IDispatch need to
> >+    // receive an automation-compliant interface as the default.
> 
> please remember to work on the comment here.

Done.

> 
> >+  static StaticAutoPtr<nsTArray<HandlerControllerData>> sHandlerControllers;
> >+  static DWORD sParentPid;
> 
> you should remove that right?

Yep, gone.


(In reply to Trevor Saunders (:tbsaunde) from comment #38)
> Comment on attachment 8852245 [details] [diff] [review]
> First cut of the handler dll (r3)
> 
> >  midl. It should essentially be the inverse of the serialization code in the
> >  IHandlerPayload implementation inside Gecko.
> 
> update?

Fixed

> 
> >+AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT* aResult)
> >+  : mscom::Handler(aOuter, aResult)
> >+  , mDispatch(nullptr)
> >+  , mIA2PassThru(nullptr)
> >+  , mServProvPassThru(nullptr)
> >+  , mCachedData()
> >+  , mCacheGen(0)
> >+{
> >+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> 
> this is just so it gets created the first time a handler is created right?

Yes.

> 
> >+  MOZ_ASSERT(ctl);
> >+  if (!ctl) {
> >+    return;
> 
> shouldn't you set aHR here?

Fixed

> 
> >+AccessibleHandler::~AccessibleHandler()
> >+{
> >+  if (mCachedData.mGeckoBackChannel) {
> 
> when is it null?

If there was no payload.

> 
> >+AccessibleHandler::MaybeUpdateCachedData()
> >+{
> >+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> 
> there's no reason we need a ref here right? the dll can't somehow be
> unloaded with this on the stack can it?  and it should have been created
> when this object's ctor was called.
> 
> >+  if (!mDispatchUnk) {
> >+    RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> 
> same

In the future I might want to get smarter about module lifetime. If I do, I'd like to be able to track references.

> 
> >+    if (!ctl) {
> 
> is this just being defensive?
> 

Could be fallible. Because this DLL may run outside of Firefox, it can't depend on our infallible allocators.

> >+AccessibleHandler::put_accName(VARIANT varChild, BSTR szName)
> >+{
> >+  // This matches AccessibleWrap
> >+  return E_NOTIMPL;
> 
> I doubt this one will change, but this dupplication is really going to suck
> as we add more.

I guess I can remove this (I assume that ATs don't try to do this very much)? The point was to try to avoid IPC only to find out that the call isn't implemented anyway.

> 
> >+AccessibleHandler::get_uniqueID(long* uniqueID)
> >+{
> >+  if (!uniqueID) {
> >+    return E_INVALIDARG;
> >+  }
> >+  if (!HasPayload()) {
> 
> when does that happen? I forget if you explained somewhere sorry.

Realistically it shouldn't, but if it does then we can handle it.

> 
> >+    HRESULT hr = ResolveIA2();
> >+    if (FAILED(hr)) {
> >+      return hr;
> >+    }
> >+    return mIA2PassThru->get_uniqueID(uniqueID);
> >+  }
> >+  BEGIN_CACHE_ACCESS;
> 
> is that necessary? it should never be necessary to invalidate or update the
> unique id since it should be constant for the proxied object right?

Yeah, we can remove this for unique id.

> 
> >+AccessibleHandler::GetClassInfo(ITypeInfo** aOutTypeInfo)
> >+{
> >+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> 
> we don't need a ref here either for similar reasons right? though I doubt
> perf really matters.

Yeah, similar to other stuff. But I'd like to leave it in.

> 
> >+  /**
> >+   * Handlers aggregate their proxies. This means that their proxies delegate
> >+   * their IUnknown implementation to us.
> >+   *
> >+   * mDispatchUnk and the result of Handler::GetProxy() are both strong
> >+   * references to the aggregated objects. OTOH, any interfaces that are QI'd
> >+   * from those aggregated objects have delegated unknowns.
> >+   *
> >+   * AddRef'ing an interface with a delegated unknown ends up incrementing the
> >+   * refcount of the *aggregator*. Since we are the aggregator of mDispatchUnk
> >+   * and of the wrapped proxy, holding a strong reference to any interfaces
> >+   * QI'd off of those objects would create a reference cycle.
> >+   *
> >+   * We may hold onto pointers to those references, but when we query them we
> >+   * must immediately Release() them to prevent these cycles.
> 
> there not really cycles as much as refs to ourself right? or are they to
> that identity object thing?

It's all transitive. We tell it to aggregate us, but we ourselves are aggregated, so effectively it is the identity whose refcount is being incremented.

> 
> maybe worth explicitly calling out this is safe because the raw pointers
> point to the agrigated things we already have a ref to?

Sure.
Comment on attachment 8852247 [details] [diff] [review]
Add invalidation handling to a11y code (r4)

Gonna punt this to a later bug.
Attachment #8852247 - Flags: review?(tbsaunde+mozbugs)
> > 
> > >+AccessibleHandler::MaybeUpdateCachedData()
> > >+{
> > >+  RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> > 
> > there's no reason we need a ref here right? the dll can't somehow be
> > unloaded with this on the stack can it?  and it should have been created
> > when this object's ctor was called.
> > 
> > >+  if (!mDispatchUnk) {
> > >+    RefPtr<AccessibleHandlerControl> ctl(gControlFactory.GetOrCreateSingleton());
> > 
> > same
> 
> In the future I might want to get smarter about module lifetime. If I do,
> I'd like to be able to track references.

not sure how that would work, but ok.

> 
> > 
> > >+    if (!ctl) {
> > 
> > is this just being defensive?
> > 
> 
> Could be fallible. Because this DLL may run outside of Firefox, it can't
> depend on our infallible allocators.

bleh, but makes sense.

> > >+AccessibleHandler::put_accName(VARIANT varChild, BSTR szName)
> > >+{
> > >+  // This matches AccessibleWrap
> > >+  return E_NOTIMPL;
> > 
> > I doubt this one will change, but this dupplication is really going to suck
> > as we add more.
> 
> I guess I can remove this (I assume that ATs don't try to do this very
> much)? The point was to try to avoid IPC only to find out that the call
> isn't implemented anyway.

no, I think its fine, its more a general problem, and we can't just avoid dupplicating anything or we wouldn't have a useful cache.

> > 
> > >+AccessibleHandler::get_uniqueID(long* uniqueID)
> > >+{
> > >+  if (!uniqueID) {
> > >+    return E_INVALIDARG;
> > >+  }
> > >+  if (!HasPayload()) {
> > 
> > when does that happen? I forget if you explained somewhere sorry.
> 
> Realistically it shouldn't, but if it does then we can handle it.

yeah, this is an area where being super careful about weird shit probably makes sense.
Depends on: 1354293
See Also: → 1368413
You need to log in before you can comment on or make changes to this bug.