We'll use PDocAccessibleConstructor and do a dance of async messages instead.

This patch accomplishes two things: 1) Replaces SendCOMProxy. Instead we use PDocAccessibleConstructor to send the content proxy to chrome. Chrome then sends an async ParentCOMProxy message to content. 2) We defer execution of DocAccessible::NotifyOfLoad until the parent com proxy has been delivered to content. This prevents a race between the load event and receipt of the parent com proxy.

This revision takes enqueues any calls to DocAccessibleChild::Send* and plays them back when the parent COM proxy is received. This is a bit ugly since Send* functions are no longer generated as virtual by IPDL. For the most part I just overrode them anyway since general the static type of the object being called is DocAccessibleChild. The one exception to that is when sending show events, so I made a virtual function for that case. Of course, if the overriding of non-virtual functions is too nasty for your liking I can use different function names...

(In reply to Aaron Klotz [:aklotz] from comment #3) > Created attachment 8808350 [details] [diff] [review] > Patch (r2) > > This revision takes enqueues any calls to DocAccessibleChild::Send* and > plays them back when the parent COM proxy is received. > > This is a bit ugly since Send* functions are no longer generated as virtual > by IPDL. For the most part I just overrode them anyway since general the > static type of the object being called is DocAccessibleChild. The one > exception to that is when sending show events, so I made a virtual function > for that case. > > Of course, if the overriding of non-virtual functions is too nasty for your > liking I can use different function names... on first thought that would seem safer, you can just drop the Send part, and then have all this logic live in DocAccessibleChildBase, but then checking if you have the parent proxy would be a little more complicated. Also I'm pretty sure we never pass around PDocAccessibleChild* and I'm not sure why we would want to, so the safety thing probably isn't a big deal. So I guess meh this is fine.

Comment on attachment 8808350 [details] [diff] [review] Patch (r2) r=me if you have a good explanation for the last question. ># HG changeset patch ># User Aaron Klotz <aklotz@mozilla.com> ># Date 1478550463 25200 ># Mon Nov 07 13:27:43 2016 -0700 ># Node ID a652622a33ddee7f85615e1073ab16ee117c7c53 ># Parent 2fd096140370a618ad4e41a0f0fbe3d157798896 >Bug 1314707: Replace PDocAccessible::SendCOMProxy with new parameter to PDocAccessibleConstructor; r=tbsaunde probably worth explaining the event business in the commit message. >+++ b/accessible/generic/DocAccessible-inl.h >@@ -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/. */ > > #ifndef mozilla_a11y_DocAccessible_inl_h_ > #define mozilla_a11y_DocAccessible_inl_h_ > > #include "DocAccessible.h" >+#include "mozilla/a11y/DocAccessibleChild.h" you can revert that now right? >+ /** >+ * @return true if the show event was sent immediately, >+ * false if the show event was deferred >+ */ >+ virtual bool MaybeSendShowEvent(ShowEventData& aData, bool aFromUser) >+ { return SendShowEvent(aData, aFromUser); } who cares about the return value? please drop it if nothing does. >+DocAccessibleParent::SetCOMInterface(const RefPtr<IAccessible>& aCOMProxy) This overloading seems weird, can you name one of these better? >+ >+// So that we don't include a bunch of other Windows junk. >+#if !defined(COM_NO_WINDOWS_H) >+#define COM_NO_WINDOWS_H >+#endif // !defined(COM_NO_WINDOWS_H) yuk, windows headers are terrible. >+bool >+DocAccessibleChild::RecvParentCOMProxy(const IAccessibleHolder& aParentCOMProxy) > { >- IAccessibleHolder parentProxy; >- PDocAccessibleChild::SendCOMProxy(aProxy, &parentProxy); >- mParentProxy.reset(parentProxy.Release()); >+ MOZ_ASSERT(!aParentCOMProxy.IsNull()); assert mParentProxy is null too? This reminds me I think I've seen cases where the parent will get replaced, so we may need to handle that, but it should be easier with this patch so that's good. >+ bool RecvParentCOMProxy(const IAccessibleHolder& aParentCOMProxy) override; fwiw personally I prefer explicit virtual, but I guess some people don't, so whatever. >+ bool SendEvent(const uint64_t& aID, const uint32_t& type); >+ bool SendHideEvent(const uint64_t& aRootID, const bool& aFromUser); >+ bool SendStateChangeEvent(const uint64_t& aID, const uint64_t& aState, >+ const bool& aEnabled); >+ bool SendCaretMoveEvent(const uint64_t& aID, const int32_t& aOffset); >+ bool SendTextChangeEvent(const uint64_t& aID, const nsString& aStr, >+ const int32_t& aStart, const uint32_t& aLen, >+ const bool& aIsInsert, const bool& aFromUser); >+ bool SendSelectionEvent(const uint64_t& aID, const uint64_t& aWidgetID, can we make all these return void? I guess not because we Unused << the result. > private: >+ struct DeferredEvent it might be good to comment this setup some maybe some of which belongs here. >+ struct SerializedShow : public DeferredEvent this and the other structs can be final right? >+ , mFromUser(aFromUser) >+ { >+ // Since IDPL doesn't generate a move constructor for ShowEventData, IPDL ;) >--- a/ipc/mscom/COMPtrHolder.h >+++ b/ipc/mscom/COMPtrHolder.h >@@ -33,17 +33,17 @@ public: > { > } > > Interface* Get() const > { > return mPtr.get(); > } > >- MOZ_MUST_USE Interface* Release() >+ MOZ_MUST_USE Interface* Release() const > { > return mPtr.release(); is this necessary? why? that doesn't seem const at all.

(In reply to Trevor Saunders (:tbsaunde) from comment #5) > Comment on attachment 8808350 [details] [diff] [review] > Patch (r2) > > r=me if you have a good explanation for the last question. > > >--- a/ipc/mscom/COMPtrHolder.h > >+++ b/ipc/mscom/COMPtrHolder.h > >@@ -33,17 +33,17 @@ public: > > { > > } > > > > Interface* Get() const > > { > > return mPtr.get(); > > } > > > >- MOZ_MUST_USE Interface* Release() > >+ MOZ_MUST_USE Interface* Release() const > > { > > return mPtr.release(); > > is this necessary? why? that doesn't seem const at all. This is another IPDL-ism due to lack of move semantics. In DocAccessibleChild::RecvParentCOMProxy, we want to move the proxy pointer out of aParentCOMProxy into mParentProxy, but aParentCOMProxy is a const reference.

(In reply to Aaron Klotz [:aklotz] from comment #6) > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > Comment on attachment 8808350 [details] [diff] [review] > > Patch (r2) > > > > r=me if you have a good explanation for the last question. > > > > >--- a/ipc/mscom/COMPtrHolder.h > > >+++ b/ipc/mscom/COMPtrHolder.h > > >@@ -33,17 +33,17 @@ public: > > > { > > > } > > > > > > Interface* Get() const > > > { > > > return mPtr.get(); > > > } > > > > > >- MOZ_MUST_USE Interface* Release() > > >+ MOZ_MUST_USE Interface* Release() const > > > { > > > return mPtr.release(); > > > > is this necessary? why? that doesn't seem const at all. > > This is another IPDL-ism due to lack of move semantics. In > DocAccessibleChild::RecvParentCOMProxy, we want to move the proxy pointer > out of aParentCOMProxy into mParentProxy, but aParentCOMProxy is a const > reference. ah, localizing the evilness with const_cast there seems somewhat better if you can, but I guess this is sort of necessary otherwise.

(In reply to Trevor Saunders (:tbsaunde) from comment #7) > > ah, localizing the evilness with const_cast there seems somewhat better if > you can, but I guess this is sort of necessary otherwise. Sure, I can do that instead.

Revised patch with all comments addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/43835f5fa2b217ba7e2a59fb9f52910a2e06a28a Bug 1314707: Replace PDocAccessible::SendCOMProxy with new parameter to PDocAccessibleConstructor and async RecvParentCOMProxy call in child. Sending of a11y events from child to parent is now deferred until DocAccessibleChild::RecvParentCOMProxy is called; r=tbsaunde

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/841f82fde3b4 for https://treeherder.mozilla.org/logviewer.html#?job_id=38935406&repo=mozilla-inbound

I was playing around with the last patch and I get errors/crash similar to this (with different AT's): IPDL protocol error: Handler returned error code! ###!!! [Parent][DispatchAsyncMessage] Error: PBrowser::Msg_PDocAccessibleConstructor Processing error: message was deserialized, but the handler returned false (indicating failure) IPDL protocol error: Handler returned error code! ###!!! [Parent][DispatchAsyncMessage] Error: PBrowser::Msg_PDocAccessibleConstructor Processing error: message was deserialized, but the handler returned false (indicating failure) ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv ###!!! [Parent][RunMessage] Error: Channel error: cannot send/recv Assertion failure: wrapper, at c:/gecko-dev/accessible/windows/msaa/Platform.cpp:84 #01: mozilla::a11y::DocAccessibleParent::Destroy (c:\gecko-dev\accessible\ipc\docaccessibleparent.cpp:429) #02: mozilla::a11y::DocAccessibleParent::ActorDestroy (c:\gecko-dev\obj-i686-pc-mingw32\dist\include\mozilla\a11y\docaccessibleparent.h:94) #03: mozilla::dom::PBrowserParent::DestroySubtree (c:\gecko-dev\obj-i686-pc-mingw32\ipc\ipdl\pbrowserparent.cpp:5182) #04: mozilla::dom::PContentParent::DestroySubtree (c:\gecko-dev\obj-i686-pc-mingw32\ipc\ipdl\pcontentparent.cpp:8909) #05: mozilla::dom::PContentParent::OnChannelError (c:\gecko-dev\obj-i686-pc-mingw32\ipc\ipdl\pcontentparent.cpp:8878) #06: mozilla::detail::RunnableMethodImpl<void (__thiscall mozilla::ipc::MessageChannel::*)(void),0,1>::Run (c:\gecko-dev\obj-i686-pc-mingw32\dist\include\nsthreadutils.h:812) #07: nsThread::ProcessNextEvent (c:\gecko-dev\xpcom\threads\nsthread.cpp:1216) #08: NS_ProcessNextEvent (c:\gecko-dev\xpcom\glue\nsthreadutils.cpp:361) #09: mozilla::ipc::MessagePump::Run (c:\gecko-dev\ipc\glue\messagepump.cpp:96) #10: MessageLoop::RunInternal (c:\gecko-dev\ipc\chromium\src\base\message_loop.cc:232) #11: MessageLoop::RunHandler (c:\gecko-dev\ipc\chromium\src\base\message_loop.cc:226) #12: MessageLoop::Run (c:\gecko-dev\ipc\chromium\src\base\message_loop.cc:206) #13: nsBaseAppShell::Run (c:\gecko-dev\widget\nsbaseappshell.cpp:158) #14: nsAppShell::Run (c:\gecko-dev\widget\windows\nsappshell.cpp:264) #15: nsAppStartup::Run (c:\gecko-dev\toolkit\components\startup\nsappstartup.cpp:284) #16: XREMain::XRE_mainRun (c:\gecko-dev\toolkit\xre\nsapprunner.cpp:4467) #17: XREMain::XRE_main (c:\gecko-dev\toolkit\xre\nsapprunner.cpp:4600) #18: XRE_main (c:\gecko-dev\toolkit\xre\nsapprunner.cpp:4691) #19: do_main (c:\gecko-dev\browser\app\nsbrowserapp.cpp:282) #20: NS_internal_main (c:\gecko-dev\browser\app\nsbrowserapp.cpp:415) #21: wmain (c:\gecko-dev\toolkit\xre\nswindowswmain.cpp:118) #22: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:255) #23: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x162c4] #24: RtlSubscribeWnfStateChangeNotification[C:\WINDOWS\SYSTEM32\ntdll.dll +0x60719] #25: RtlSubscribeWnfStateChangeNotification[C:\WINDOWS\SYSTEM32\ntdll.dll +0x606e4]

I cannot reproduce those failures locally. :-( It's probably the fact that these tests are run on a VM that we can see this. From what I can see looking at the stacks on those failures, the real problem is that something is causing xul.dll!mozilla::dom::ContentParent::KillHard(char const *), which then injects breakpoints into everything to trigger crash reports. ... I have examined the browser minidump and extracted the reason message from the KillHard call: PBrowser::Msg_PDocAccessibleConstructor Processing error: message was deserialized, but the handler returned false (indicating failure) So it looks like this patch can't land until we fix the PDocAccessibleConstructor problems. :-( OTOH, we now know how to consistently reproduce this. Adding bug 1270916 as a dependency.

Updated to reflect new return type of IPDL Recv* functions.

The first patch is still failing even though the PDocAccessibleConstructor patch landed. I realized that the reason why is that even though the top-level DocAccessibleChild has not yet transmitted any deferred show events, we're still trying to SendPDocAccessibleConstructor calls that depend on them. This additional patch adds deferral of those SendPDocAccessibleConstructor calls as well. This appears to work, but if you can think of a better way of doing this...

OK, this patch allows us to enqueue all the things in each top level DocAccessibleChild.

Revision 5 was crashing during a top-level document shutdown because DocAccessibleChild::GetTopLevelIPCDoc was failing to account for the fact that DocAccessible::mDocumentNode is cleared *before* DocAccessibleChild::Shutdown is called.

More clownshoes.

Trevor what do you think of this latest patch?

Comment on attachment 8812406 [details] [diff] [review] Patch (r7) Sorry about the lag here. >+ >+#if defined(XP_WIN) >+ MOZ_ASSERT(parentIPCDoc); >+ parentIPCDoc->ConstructChildDocInParentProcess(ipcDoc, id, >+ AccessibleWrap::GetChildIDFor(childDoc)); >+#else > nsCOMPtr<nsITabChild> tabChild = > do_GetInterface(mDocument->DocumentNode()->GetDocShell()); > if (tabChild) { > MOZ_ASSERT(parentIPCDoc); > static_cast<TabChild*>(tabChild.get())-> >- SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, id, >-#if defined(XP_WIN) >- AccessibleWrap::GetChildIDFor(childDoc) >-#else >- 0 >+ SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, id, 0, 0); >+ } > #endif might be nice to hide more of this junk in the Construct method, but whatever. >-DocAccessibleChild::SendCOMProxy(const IAccessibleHolder& aProxy) >+DocAccessibleChild::Shutdown() > { >- IAccessibleHolder parentProxy; >- PDocAccessibleChild::SendCOMProxy(aProxy, &parentProxy); >- mParentProxy.reset(parentProxy.Release()); >+ DocAccessibleChild* rootIPCDoc = GetTopLevelIPCDoc(); >+ if (!rootIPCDoc || rootIPCDoc->IsConstructedInParentProcess()) { >+ DocAccessibleChildBase::Shutdown(); So, I think there's still a race here, what happens if the parent process hasn't yet send us a com proxy for the top level doc but we go to shutdown a sub document? I think this will end up sending a shutdown message for an actor we haven't yet told the main process about. I'm not completely sure if this is a problem with other messages, or just this one. >+DocAccessibleChild* >+DocAccessibleChild::GetTopLevelIPCDoc() >+{ >+ if (!mDoc) { >+ // This is possible during shutdown >+ return nullptr; >+ } >+ >+ DocAccessible* topLevelDocAcc = nullptr; >+ >+ if (mDoc->IsRoot()) { >+ topLevelDocAcc = mDoc; >+ } else { >+ topLevelDocAcc = mDoc->RootAccessible(); >+ if (!topLevelDocAcc) { >+ // This is possible during shutdown >+ return nullptr; >+ } >+ } >+ >+ return topLevelDocAcc->IPCDoc(); >+} so, I think a different way to this that may always return something is to get the DocAccessibleChild actor's mannager which is a TabChild, andthen look through the DocAccessibleChilds it manages for one that is a top level doc, I believe there should only be one. That's because I believe actor's should get destroyed bottum up, and there should only be one top level doc in a tab. I'm not completely sure that'll work though. That might fix the race above? >+ bool SendEvent(const uint64_t& aID, const uint32_t& type); >+ bool SendHideEvent(const uint64_t& aRootID, const bool& aFromUser); >+ bool SendStateChangeEvent(const uint64_t& aID, const uint64_t& aState, since bool is now inconsistant with the ipdl generated stuff and the ipc result name seems long and pointless might make sense to just return void, but whatever. >+ { >+ void Dispatch() >+ { >+ Dispatch(mTarget); >+ } >+ >+ virtual ~DeferredEvent() {} >+ >+ protected: >+ explicit DeferredEvent(DocAccessibleChild* aTarget) >+ : mTarget(aTarget) >+ {} >+ >+ virtual void Dispatch(DocAccessibleChild* aIPCDoc) = 0; >+ >+ private: >+ DocAccessibleChild* mTarget; seems like it might be simpler to just put this member in the subclasses, and get rid of the DispatchEvent(DocAccessible*) overload, but doesn't really matter. It'd be nice to reduce the boilerplate a bit, but I guess it can wait. We need to deal with that race, but I think that's much less important than the current issues.

(In reply to Trevor Saunders (:tbsaunde) from comment #25) > Comment on attachment 8812406 [details] [diff] [review] > Patch (r7) > > >-DocAccessibleChild::SendCOMProxy(const IAccessibleHolder& aProxy) > >+DocAccessibleChild::Shutdown() > > { > >- IAccessibleHolder parentProxy; > >- PDocAccessibleChild::SendCOMProxy(aProxy, &parentProxy); > >- mParentProxy.reset(parentProxy.Release()); > >+ DocAccessibleChild* rootIPCDoc = GetTopLevelIPCDoc(); > >+ if (!rootIPCDoc || rootIPCDoc->IsConstructedInParentProcess()) { > >+ DocAccessibleChildBase::Shutdown(); > > So, I think there's still a race here, what happens if the parent process > hasn't yet send us a com proxy for the top level doc but we go to shutdown a > sub document? I think this will end up sending a shutdown message for an > actor we haven't yet told the main process about. I'm not completely sure > if this is a problem with other messages, or just this one. > > >+DocAccessibleChild* > >+DocAccessibleChild::GetTopLevelIPCDoc() > >+{ > >+ if (!mDoc) { > >+ // This is possible during shutdown > >+ return nullptr; > >+ } > >+ > >+ DocAccessible* topLevelDocAcc = nullptr; > >+ > >+ if (mDoc->IsRoot()) { > >+ topLevelDocAcc = mDoc; > >+ } else { > >+ topLevelDocAcc = mDoc->RootAccessible(); > >+ if (!topLevelDocAcc) { > >+ // This is possible during shutdown > >+ return nullptr; > >+ } > >+ } > >+ > >+ return topLevelDocAcc->IPCDoc(); > >+} > > so, I think a different way to this that may always return something is to > get the DocAccessibleChild actor's mannager which is a TabChild, andthen > look through the DocAccessibleChilds it manages for one that is a top level > doc, I believe there should only be one. That's because I believe actor's > should get destroyed bottum up, and there should only be one top level doc > in a tab. I'm not completely sure that'll work though. > > That might fix the race above? > I have moved top-level doc resolution to a function called DocAccessibleChild::PushDeferredEvent that obtains it from the tabChild. Sub-document shutdowns will now call PushDeferredEvent to enqueue its shutdown to its top-level doc, thus placing its shutdown message some point after its own constructor.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f56b053ab5cffd9e050e5a8aca69840a9d6a611 Bug 1314707: Replace PDocAccessible::SendCOMProxy with new parameter to PDocAccessibleConstructor and async RecvParentCOMProxy call in child. Sending of a11y events from child to parent is now deferred until DocAccessibleChild::RecvParentCOMProxy is called; r=tbsaunde

Comment on attachment 8819060 [details] [diff] [review] Patch (r8) Approval Request Comment [Feature/Bug causing the regression]: a11y+e10s [User impact if declined]: Crashes [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: Depends on the uplift for bug 1321935 [Is the change risky?]: No [Why is the change risky/not risky?]: This was originally landed on 52 but was backed out because of the dependent bug 1321935. Now that the dependent bug is fixed and everything looks good on Nightly, this change can return back to 52. [String changes made/needed]: None

I guess I'll wait for bug 1323996 before getting this in aurora.

Comment on attachment 8819060 [details] [diff] [review] Patch (r8) e10s+a11y fix for aurora52

needs rebasing for aurora

One more due to bustage from the IPCResult changeover in version 53. https://hg.mozilla.org/releases/mozilla-aurora/rev/79763f9eac51d555813ae9c9b87ece57e2859b31