1304449 - Rework MSAA child id generation for e10s

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

In bug 1297549, Trevor suggested modifying child ids to use a scheme where N bits are allocated to identify the content process, then using the remaining (31 - N) bits as the identity within that process.

After landing the stopgap solution in bug 1297549, I sat down to investigate how we could accomplish this. I have written a patch that allocates 7 bits to identify content, leaving 24 bits to unique ids. Since content ID 0 is reserved for the chrome process, that leaves room for 127 content processes.

In conjunction with that change, I have been reworking child resolution such that we now only need to marshal COM proxies when creating top-level documents. This greatly speeds up SendShowEvent and effectively eliminates the Talos a11yr regressions.

Furthermore, having the content id embedded in the child id allows us to do some exciting things with respect to searching, as it allows us to prune entire subtrees off of the search space. For example, if we see that the content id does not belong to chrome, we can skip chrome accessibles entirely and jump straight to remote. This brings even more benefits in multi-e10s land.

This patch, combined with a few other fixes that I have in my queue for improving MainThreadHandoff latency, actually make a11yr *better* by ~2%.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Also note that, as a consequence of the fact that not every ProxyAccessible has a valid COM proxy, XPCOM tests will need to lazily load those as needed.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Modified ID Generation

This patch makes IDSet a bit more generic and adds a MsaaIdGenerator class that uses IDSet to generate the unique ids and then adds in content id stuff. AccessibleWrap is changed in a later patch to use MsaaIdGenerator instead of IDSet.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 - Make ContentParent obtain/release MSAA IDs as needed

(Feel free to redirect as necessary)

Each ContentParent instance receives a child id. This is not directly usable by a11y, so instead we add a function that lazily obtains an a11y content id based on the ContentParent's child id. Of course, when the ContentParent shuts down, it needs to then release any such id.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3 - AccessibleWrap changes

In addition to using the MsaaIdGenerator, this patch also reworks traversal so that it does so in terms of COM IAccessibles instead of native accessibles. This makes it easier to jump to content and means that we don't need to manage as much stuff in the chrome process.

Also note that, in the interest of simplicity (and since non-e10s mode is supposed to be going away by the end of the year according to blassey), I have removed the old scheme of negating the accessible's pointer in favor of using MsaaIdGenerator exclusively.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4 - Glue the rest together

This part is essentially all about gluing together the changes in the other patches. Instead of passing COM proxies in SendShowEvent, we now just pass MSAA IDs -- in fact, ProxyAccessibles now need MSAA IDs generated by content. It also ends up reverting a lot of the changes made in bug 1297549 since they are no longer necessary.

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8793857 [details] [diff] [review]
Part 1 - Modified ID Generation

Given you need this to fix crashes r=me with questions answered I guess.  I'd really like to see this be clearer, but I also don't want to review all the bit manipulation junk again.

> /**
>- * On windows an accessible's id must be a negative 32 bit integer.  It is
>+ * On windows an accessible's id must be a negative 32 bit integer. It is
>  * important to support recycling arbitrary IDs because accessibles can be
>  * created and destroyed at any time in the life of a page.  IDSet provides 2
>- * operations: generate an ID in the range [ - 2^31, 0 ), and release an ID so
>+ * operations: generate an ID in the range (0, mMaxId], and release an ID so
>  * it can be allocated again.  Allocated ID are tracked by a sparse bitmap
>  * implemented with a splay tree.  Nodes in the tree are keyed by the upper N
>- * bits of the bitwise negation of the ID, and the node contains a bitmap
>- * tracking the allocation of 2^(32 - N) IDs.
>+ * bits of the ID, and the node contains a bitmap tracking the allocation of
>+ * 2^(ceil(log2(mMaxId)) - N) IDs.

I think there are some bugs in the top end of the range caused by integer division, but I think they are preexisting, and I don't really care.

>+    , mIdx(0)
>+    , mMaxId(INT32_MAX)
>+    , mMaxIdx(INT32_MAX / bitsPerElt)

I think that is actually off by 1, INT32_MAX is 2^31 - 1 which doesn't divide exactly by 128 so you have a remainder.  However you can evenly divide a set from 0 to 2^32 - 1 into chunks of size 128.  That all said when this is all done we are talking about 128 out of about 16m possible ids so I don't think this is worth caring about.

>+  void SetMaxIdBits(const uint32_t aMaxIdBits)
>+  {
>+    // Can't call this unless we're empty
>+    MOZ_ASSERT(mBitSet.empty());

hmm, is it actually worth supporting this? its not clear to me there's value in not just having a constant limmit.  Or initializing it only in the ctor.

>+    mMaxId = (1UL << aMaxIdBits) - 1UL;
>+    mMaxIdx = mMaxId / bitsPerElt;

I think this has a similar division issue and I don't care for the same reason.

>   static const unsigned int wordsPerElt = 2;
>   static const unsigned int bitsPerWord = 64;
>   static const unsigned int bitsPerElt = wordsPerElt * bitsPerWord;
>-  static const uint32_t sMaxIdx = INT32_MAX / bitsPerElt;

heh, I probably even new about the division business when I wrote this and decided I didn't care then.

>+// is allocated to the content ID vs the unique ID. They must always sum to 31,
>+// ie. the width of a 32-bit integer less the sign bit.
>+#define NUM_UNIQUE_ID_BITS 24
>+#define NUM_CONTENT_ID_BITS 7

can we use static consts here? I think that should be fine.

>+
>+typedef nsDataHashtable<nsUint64HashKey,uint32_t> ContentIdMap;

I'm not fond of this name because I'm not clear on what "content id" means, and what is being mapped to what.  Also as a nit, space after ',' please.

>+union MsaaID
>+{
>+  int32_t mInt32;
>+  uint32_t mUInt32;
>+  struct
>+  {
>+    uint32_t mUniqueId:NUM_UNIQUE_ID_BITS;
>+    uint32_t mContentId:NUM_CONTENT_ID_BITS;
>+    uint32_t mSignBit:1;
>+  }

should we pragma packed this? I believe the bitfield rules require the right thing anyway, but it can't hurt?

>+class MsaaIDBuilder
>+{
>+public:
>+  MsaaIDBuilder(uint32_t aID, uint32_t aContentID)
>+  {
>+    mID.mCracked.mSignBit = 1;
>+    mID.mCracked.mUniqueId = ~aID;
>+    mID.mCracked.mContentId = ~aContentID;

I'm unclear on which part is what here.

Also making this a class seems silly to me, its basically just a function uint32_t, uint32_t -> uint32_t

>+class MsaaIDCracker
>+{
>+public:
>+  MsaaIDCracker(uint32_t aMsaaID)
>+  {
>+    mID.mUInt32 = ~aMsaaID;
>+  }
>+
>+  uint32_t GetContentId()
>+  {
>+    return mID.mCracked.mContentId;

content id is what process it is in right? so can we s/content/process/ ?

>+  uint32_t GetUniqueId()
>+  {
>+    return mID.mCracked.mUniqueId;

Similar opinion of this class as the other one though this is more like two separate functions.

>+uint32_t
>+MsaaIdGenerator::GetID()
>+{
>+  static const bool isE10s = ResolveContentId();
>+  MOZ_ASSERT((XRE_IsParentProcess() && !mContentId) ||
>+             (XRE_IsContentProcess() && mContentId));

I think this would be clearer with if (is parent process) assert else if content process assert else unreachable but maybe that's just me

>+  uint32_t id = mIdSet.GetID();
>+  MOZ_ASSERT(id <= ((1UL << NUM_UNIQUE_ID_BITS) - 1UL) || !isE10s);

When is isE10s false?

>+void
>+MsaaIdGenerator::ReleaseID(uint32_t aID)
>+{
>+  detail::MsaaIDCracker cracked(aID);
>+  if (cracked.GetContentId() != mContentId) {
>+    // This ID was generated elsewhere, so we just ignore it

when can that happen? is this for the accessibles that wrap proxies?  can we rearrange this to assert that is what is happening?

>+MsaaIdGenerator::ResolveContentId()
>+{
>+  mIdSet.SetMaxIdBits(NUM_UNIQUE_ID_BITS);
>+  if (XRE_IsParentProcess()) {
>+    return true;

afaik that is true in the main process for a non e10s browser as well.

>+  }
>+  dom::ContentChild* contentChild = dom::ContentChild::GetSingleton();
>+  if (!contentChild->SendGetA11yContentId(&mContentId)) {

can this actually fail?

>+static StaticAutoPtr<detail::ContentIdMap> sContentIdMap;

can you comment what this is?  As a guess I think it has something to do with mapping gecko's notion of what process we are in to the id used in MSAA id?

>+  const uint32_t kBitsPerElement = sizeof(sContentIdBitmap[0]) * kBitsPerByte;

seems kind of over engineered, if you have uint64_t without exactly 64 bits your platform is just wrong.

>+  uint32_t value = 0;
>+  if (sContentIdMap->Get(aIPCContentID, &value)) {

interestingly in concept this dupplicates the bitmap and hash table business used for the full id, but the tighter constraints do make the separate impl reasonable.

>+    return value;
>+  }
>+
>+  uint32_t index = 0;
>+  for (; index < ArrayLength(sContentIdBitmap); ++index) {
>+    if (sContentIdBitmap[index] == UINT64_MAX) {
>+      continue;
>+    }
>+    uint32_t bitIndex = CountTrailingZeroes64(~sContentIdBitmap[index]);
>+    MOZ_ASSERT(!(sContentIdBitmap[index] & (1ULL << bitIndex)));
>+    MOZ_ASSERT(bitIndex != 0 || index != 0);
>+    sContentIdBitmap[index] |= (1ULL << bitIndex);
>+    value = index * kBitsPerElement + bitIndex;
>+    break;

I'd like to write this in the more obvious way of
for elt in elts
  for i in [0, 64)
    if (elt & 1 << i) == 0
      elt |= 1 << i

but I guess msvc isn't smart enough to not make a hash of that?

>+  }
>+
>+  if (index == ArrayLength(sContentIdBitmap)) {
>+    // Nothing
>+    return 0;

if we hit this things aren't going to go well right? shouldn't we assert?

>+{
>+  MOZ_ASSERT(XRE_IsParentProcess() && NS_IsMainThread());
>+  // Since Content IDs are generated lazily, ContentParent might attempt
>+  // to release on an ID that was never allocated to begin with.

This is when there has never been a content process with accessibles and then we shut one down?

>+  auto mapping = sContentIdMap->GetAndRemove(aIPCContentID);

imho its not clear enough what mapping's type is to use auto here.

>+  if (!mapping) {

can be false when a particular process didn't have accessibles but another one did right?

>+  const uint32_t kBitsPerElement = sizeof(sContentIdBitmap[0]) * 8;

seems like it would be better to have one of these next to the definition of the bitmap.

>+
>+class MsaaIdGenerator

comment how all this works please :)

it also seems like this class does two things, it deals with MSAA ids, and it has this business about mapping gecko process ids to the number we will use in MSAA id, which is I guess related, but sort of separate.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Modified ID Generation (r2)

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> Comment on attachment 8793857 [details] [diff] [review]
> Part 1 - Modified ID Generation
> 
> Given you need this to fix crashes r=me with questions answered I guess. 
> I'd really like to see this be clearer, but I also don't want to review all
> the bit manipulation junk again.

Any comments I add here will be answering your questions. I have tried to address most of your other remarks in this revision as long as they don't materially affect the behavior of the code.

> >+  uint32_t id = mIdSet.GetID();
> >+  MOZ_ASSERT(id <= ((1UL << NUM_UNIQUE_ID_BITS) - 1UL) || !isE10s);
> 
> When is isE10s false?

That variable was vestigial (it meant something before I eliminated the old non-e10s, 32-bit, "negate the pointer" implementation), so I have eliminated that completely since it no longer matters.

> 
> >+void
> >+MsaaIdGenerator::ReleaseID(uint32_t aID)
> >+{
> >+  detail::MsaaIDCracker cracked(aID);
> >+  if (cracked.GetContentId() != mContentId) {
> >+    // This ID was generated elsewhere, so we just ignore it
> 
> when can that happen? is this for the accessibles that wrap proxies?  can we
> rearrange this to assert that is what is happening?

That is correct -- chrome will host proxies whose ids were generated by content. I have modified MsaaIdGenerator::ReleaseID to accept the AccessibleWrap pointer instead of its id so that we can quickly and easily perform this sanity check.

> >+  dom::ContentChild* contentChild = dom::ContentChild::GetSingleton();
> >+  if (!contentChild->SendGetA11yContentId(&mContentId)) {
> 
> can this actually fail?

No. Changed to use Unused.

> 
> >+static StaticAutoPtr<detail::ContentIdMap> sContentIdMap;
> 
> can you comment what this is?  As a guess I think it has something to do
> with mapping gecko's notion of what process we are in to the id used in MSAA
> id?

Yes. Commented and added some additional details about why ContentParent IDs cannot map directly to MSAA content IDs.

> >+
> >+  if (index == ArrayLength(sContentIdBitmap)) {
> >+    // Nothing
> >+    return 0;
> 
> if we hit this things aren't going to go well right? shouldn't we assert?
> 

Changed to a release assert.

> >+  MOZ_ASSERT(XRE_IsParentProcess() && NS_IsMainThread());
> >+  // Since Content IDs are generated lazily, ContentParent might attempt
> >+  // to release on an ID that was never allocated to begin with.
> 
> This is when there has never been a content process with accessibles and
> then we shut one down?
Correct.

> >+  if (!mapping) {
> 
> can be false when a particular process didn't have accessibles but another
> one did right?

Correct.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Modified ID Generation (r3)

Whoops, got the release assert condition wrong -_-

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8793870 [details] [diff] [review]
Part 4 - Glue the rest together

so, fwiw the way I think I would have split some of this out is first do the moz.build changes, those don't depend on anything.  Then remove the ID setup based on casting pointers to uint32_t.  Finally you can revert the patches to do IDs the previous way before adding the new code, you would need to fold it with the patch adding new code, but you need to do that with these patches too, and I think it would get a much simpler diff.

>         do_GetInterface(mDocument->DocumentNode()->GetDocShell());
>       if (tabChild) {
>         static_cast<TabChild*>(tabChild.get())->
>           SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, id);
> #if defined(XP_WIN)
>-        IAccessibleHolder holder(CreateHolderFromAccessible(childDoc));
>-        ipcDoc->SendCOMProxy(holder);
>+        if (parentIPCDoc) {
>+          ipcDoc->SendMsaaID(AccessibleWrap::GetChildIDFor(childDoc));

have you ever seen this be false? I really don't think it ever should be.  Since I think that has been unclear before it wouldn't be bad to assert that is the case.

> ProxyAccessible::GetCOMInterface(void** aOutAccessible) const
> {
>   if (!aOutAccessible) {
>     return false;
>   }
>+  if (!mCOMProxy) {

blank line after }?

>+    // See if we can lazily obtain a COM proxy
>+    AccessibleWrap* wrap = WrapperFor(this);
>+    bool isDefunct = false;
>+    ProxyAccessible* thisPtr = const_cast<ProxyAccessible*>(this);
>+    thisPtr->mCOMProxy = wrap->GetIAccessibleFor(kChildIdSelf, &isDefunct);

use mutable instead maybe? *shrug*

more importantly GetIAccessibleFor seems to only set aShutdown to false, so remove that arg?

>+++ b/accessible/windows/msaa/Platform.cpp
>@@ -84,24 +84,20 @@ a11y::ProxyDestroyed(ProxyAccessible* aP
>   MOZ_ASSERT(wrapper);
>   if (!wrapper)
>     return;
> 
>   auto doc =
>     static_cast<DocProxyAccessibleWrap*>(WrapperFor(aProxy->Document()));
>   MOZ_ASSERT(doc);
>   if (doc) {
>-#ifdef _WIN64
>     uint32_t id = wrapper->GetExistingID();
>     if (id != AccessibleWrap::kNoID) {
>       doc->RemoveID(id);
>     }
>-#else
>-    doc->RemoveID(-reinterpret_cast<int32_t>(wrapper));

afaik we never add ids to a DocProxyWrapper, and we shouldn't need to after this patch, so I don't see why we need to keep any of this id removing stuff.

r=me with the above delt with.

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8793861 [details] [diff] [review]
Part 3 - AccessibleWrap changes

r=me really sorry about the lag!

> AccessibleWrap::get_accName(
>       /* [optional][in] */ VARIANT varChild,
>       /* [retval][out] */ BSTR __RPC_FAR *pszName)
> {
>   A11Y_TRYBLOCK_BEGIN
> 
>-  if (!pszName)
>+  if (!pszName || varChild.vt != VT_I4)

technically I think you should check in all these methods if the variant type is int before checking the value.

>+  if (varChild.lVal != CHILDID_SELF) {
>+    bool isDefunct = false;
>+    RefPtr<IAccessible> accessible = GetIAccessibleFor(varChild, &isDefunct);
>+    if (!accessible) {
>+      return E_INVALIDARG;
>+    }
>+    if (isDefunct) {
>+      return CO_E_OBJNOTCONNECTED;
>+    }
>+    return accessible->get_accName(kVarChildIdSelf, pszName);

this seems like a lot of boiler plate, but I guess there aren't that many methods on IAccessible

I guess one thing that would help some is getting rid of the IsDefunct check in GetIAccessible, and let the call to the actual method handle that.  It would also save a few instructions on the only case that hopefully matters where the accessible is not dead, but this code isn't going to be super fast anyway so hopefully that doesn't really matter.

>+      navAccessible = IsProxy()
>+        ? WrapperFor(Proxy()->PrevSibling())
>+        : PrevSibling();

I think the IsProxy() cases here should be dead, but updating them is fine for now I guess.

> AccessibleWrap::SetID(uint32_t aID)
> {
>-  MOZ_ASSERT(XRE_IsContentProcess());
>+  MOZ_ASSERT(XRE_IsParentProcess());

should we assert IsProxy() too?

>   mID = aID;
>   DocAccessibleWrap* doc = static_cast<DocAccessibleWrap*>(Document());
>-  DebugOnly<AccessibleWrap*> checkAcc = nullptr;
>-  MOZ_ASSERT(!(checkAcc = doc->GetAccessibleByID(aID)) ||
>-             checkAcc->GetExistingID() == aID);
>-  doc->AddID(aID, this);
>+  if (doc) {

I think doc is always null here because this is a proxy, so afaict this block is dead.

>+  // Chrome should use mID which has been generated by the content process.
>+  if (aAccessible->IsProxy()) {
>     const uint32_t id = static_cast<AccessibleWrap*>(aAccessible)->mID;
>     MOZ_ASSERT(id != kNoID);
>     return id;
>   }
> 
>-#ifdef _WIN64
>   if (!aAccessible->Document() && !aAccessible->IsProxy())

might as well delete the IsProxy() check here

>   auto wrapper = static_cast<DocProxyAccessibleWrap*>(WrapperFor(aDoc));
>+  RefPtr<IAccessible> comProxy;
>+  int32_t wrapperChildId = AccessibleWrap::GetChildIDFor(wrapper);
>+  if (wrapperChildId == aVarChild.lVal) {
>+    wrapper->GetNativeInterface(getter_AddRefs(comProxy));

it seems like you basically always want to call that so might as well pull it out of the if.

>+  VARIANT varChild = aVarChild;
>+
>+  MOZ_ASSERT(aIsDefunct);
>+  *aIsDefunct = false;
>+
>+  RefPtr<IAccessible> result;
>+
>   // if its us real easy - this seems to always be the case

I feel like the first part is kind of obvious, and the second part is clearly just bogus.

>+  if (varChild.lVal == CHILDID_SELF) {
>+    *aIsDefunct = IsDefunct();
>+    GetNativeInterface(getter_AddRefs(result));
>+    if (result || !IsProxy() || IsDefunct()) {

for some reason that condition was not obvious to me.

>+  if (XRE_IsParentProcess() && !sIDGen.IsChromeID(varChild.lVal)) {

can we assert that in a child process we are only ever looking for an id that matches that process?

>+  if (varChild.lVal > 0) {
>+    Accessible* xpAcc = nullptr;
>     // Gecko child indices are 0-based in contrast to indices used in MSAA.
>     if (IsProxy()) {

is that ever true? I can't convince myself it is.

>+    if (!xpAcc) {
>+      return nullptr;
>+    }
>+    *aIsDefunct = xpAcc->IsDefunct();
>+    static_cast<AccessibleWrap*>(xpAcc)->GetNativeInterface(getter_AddRefs(result));
>+    return result.forget();

this is making it a bit slower to call IAccessible methods with a child id, but other than accChild I'd rather not care.  I'm tempted to worry about the extra virtual call to GetNativeInterface in accChild, but maybe that's okish too.

>+AccessibleWrap::GetRemoteIAccessibleFor(const VARIANT& aVarChild, bool* aIsDefunct)
>+{
>+  *aIsDefunct = false;

as far as I can see this function never sets it to true.

>   for (size_t i = 0; i < docCount; i++) {
>     Accessible* outerDoc = remoteDocs->ElementAt(i)->OuterDocOfRemoteBrowser();
>     if (!outerDoc) {
>       continue;
>     }
> 
>     if (outerDoc->Document() != doc) {
>       continue;
>     }

you could also check the process id bits of the id to see if they are the same before calling get_accChild on the proxy right?

>+AccessibleWrap::GetContentIdFor(dom::ContentParentId aIPCContentId)
>+{
>+  return sIDGen.GetContentIDFor(aIPCContentId);

this wrapper seems silly, why don't you use MsaaIDGenerator directly?

>+AccessibleWrap::ReleaseContentIdFor(dom::ContentParentId aIPCContentId)
>+{
>+  sIDGen.ReleaseContentIDFor(aIPCContentId);

same

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

I've addressed all comments on these last two reviews except for this:

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> Comment on attachment 8793861 [details] [diff] [review]
> Part 3 - AccessibleWrap changes
> >+AccessibleWrap::GetContentIdFor(dom::ContentParentId aIPCContentId)
> >+{
> >+  return sIDGen.GetContentIDFor(aIPCContentId);
> 
> this wrapper seems silly, why don't you use MsaaIDGenerator directly?
> 
> >+AccessibleWrap::ReleaseContentIdFor(dom::ContentParentId aIPCContentId)
> >+{
> >+  sIDGen.ReleaseContentIDFor(aIPCContentId);
> 
> same

Those are called from dom::ContentParent which does not have direct access to sIDGen.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Modified ID Generation (r4)

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 - Make ContentParent obtain/release MSAA IDs as needed (r2)

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3 - AccessibleWrap changes (r2)

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4 - Glue the rest together (r2)

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0861da34195

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Aaron Klotz [:aklotz] from comment #11)
> I've addressed all comments on these last two reviews except for this:
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > Comment on attachment 8793861 [details] [diff] [review]
> > Part 3 - AccessibleWrap changes
> > >+AccessibleWrap::GetContentIdFor(dom::ContentParentId aIPCContentId)
> > >+{
> > >+  return sIDGen.GetContentIDFor(aIPCContentId);
> > 
> > this wrapper seems silly, why don't you use MsaaIDGenerator directly?
> > 
> > >+AccessibleWrap::ReleaseContentIdFor(dom::ContentParentId aIPCContentId)
> > >+{
> > >+  sIDGen.ReleaseContentIDFor(aIPCContentId);
> > 
> > same
> 
> Those are called from dom::ContentParent which does not have direct access
> to sIDGen.

that seems kind of silly perhaps, but fair enough for now.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04251bf20c2708abd518c21b9609351df20398f7
Bug 1304449: Part 1 - Modify MSAA IDs to be partitioned based on content id; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/5343421a3efd4aa79908141ee8f6a75fcef544ff
Bug 1304449: Part 2 - Modify ContentParent to obtain/release MSAA ids; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf589c2288ad6a56387f5d4ade06a5a0344c6cd6
Bug 1304449: Part 3 - Modify AccessibleWrap to traverse using COM interfaces; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc58d0708e7ee5f0c06bf0ef760a591153a6fef
Bug 1304449: Part 4 - Change Windows a11y MSAA id generation to partition based on content process id; r=tbsaunde

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04251bf20c27
https://hg.mozilla.org/mozilla-central/rev/5343421a3efd
https://hg.mozilla.org/mozilla-central/rev/cf589c2288ad
https://hg.mozilla.org/mozilla-central/rev/7cc58d0708e7

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like the latest changeset in this series caused a massive crash rate for Marionette e10s tests on Windows as XP as seen on bug 1299216 and bug 1141483.

Comment 21

[Alison Shiue]

This patch did a perf improvement on perfherder, thanks!

Improvements:

 94%  a11yr summary windows8-64 pgo e10s     6947.6 -> 434.29
 94%  a11yr summary windows7-32 pgo e10s     7452.46 -> 468.6
 92%  a11yr summary windows8-64 opt e10s     7622.17 -> 601.77
 91%  a11yr summary windows7-32 opt e10s     8267.74 -> 780.83
 70%  a11yr summary windowsxp pgo e10s       1448.26 -> 429.1
 63%  a11yr summary windowsxp opt e10s       1986.7 -> 734.54

For up to date info, please refer: https://treeherder.mozilla.org/perf.html#/alerts?id=3629

Please help to request uplift on firefox51, thank you!

Comment 22

[Gerry Chang [:gchang]]

Hi :aklotz,
Per comment #8 in bug 1303625, can you help create an uplift request for 51 aurora?

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

I can uplift once bug 1308397 lands. They need to uplift as a package deal.

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8798252 [details] [diff] [review]
Part 1 - Modified ID Generation (r4)

Approval Request Comment
[Feature/regressing bug #]: Windows a11y on e10s
[User impact if declined]: Talos regressions, poor stability when e10s is force-enabled.
[Describe test coverage new/current, TreeHerder]: Currently on m-c. Talos is back down to its previous scores.
[Risks and why]: Moderate: This patch, while e10s related, also affects non-e10s code. OTOH enough users force-enable e10s that I think this is worth taking.
[String/UUID change made/needed]: None

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8798253 [details] [diff] [review]
Part 2 - Make ContentParent obtain/release MSAA IDs as needed (r2)

Approval Request Comment
[Feature/regressing bug #]: Windows a11y on e10s
[User impact if declined]: Talos regressions, poor stability when e10s is force-enabled.
[Describe test coverage new/current, TreeHerder]: Currently on m-c. Talos is back down to its previous scores.
[Risks and why]: Moderate: This patch, while e10s related, also affects non-e10s code. OTOH enough users force-enable e10s that I think this is worth taking.
[String/UUID change made/needed]: None

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8798254 [details] [diff] [review]
Part 3 - AccessibleWrap changes (r2)

Approval Request Comment
[Feature/regressing bug #]: Windows a11y on e10s
[User impact if declined]: Talos regressions, poor stability when e10s is force-enabled.
[Describe test coverage new/current, TreeHerder]: Currently on m-c. Talos is back down to its previous scores.
[Risks and why]: Moderate: This patch, while e10s related, also affects non-e10s code. OTOH enough users force-enable e10s that I think this is worth taking.
[String/UUID change made/needed]: None

Comment 27

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8798255 [details] [diff] [review]
Part 4 - Glue the rest together (r2)

Approval Request Comment
[Feature/regressing bug #]: Windows a11y on e10s
[User impact if declined]: Talos regressions, poor stability when e10s is force-enabled.
[Describe test coverage new/current, TreeHerder]: Currently on m-c. Talos is back down to its previous scores.
[Risks and why]: Moderate: This patch, while e10s related, also affects non-e10s code. OTOH enough users force-enable e10s that I think this is worth taking.
[String/UUID change made/needed]: None

Comment 28

[(No longer employed by Mozilla) Aaron Klotz]

Note that approval+landing on aurora also requires bug 1308397 to be uplifted: they must go as a package deal.

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8798252 [details] [diff] [review]
Part 1 - Modified ID Generation (r4)

Fix Talos regressions. Take it in 51 aurora.

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm hitting conflicts trying to uplift part 2 from this bug. Could we get a rebased patch?

Comment 31

[(No longer employed by Mozilla) Aaron Klotz]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c91317b095bd760c0e50ec1b14249e3306974315
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/1b3dcd9245a3fc2f2d081d4a20a712675a63a257
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0e851d57a906c4ea6c8fe64bae0b9b150eb0d79c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/27d41ff392d04ea080baad0ae16b79666f5bc103

Comment 32

[(No longer employed by Mozilla) Aaron Klotz]

Landed a couple bustage fixes due to merge difficulties:

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bb0c02b6c1ce9ed61e2e08b8f37eef5899a7e31
https://hg.mozilla.org/releases/mozilla-aurora/rev/f9f623c85158de2792a8da1026fb5a2b57d7519d

Comment 33

[Alison Shiue]

Talos has detected a perf improvement on aurora, thanks.
Please refer: https://treeherder.mozilla.org/perf.html#/alerts?id=3766