implement very basic support of OOp accessible documents

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 5 obsolete attachments)

35.45 KB, patch
baku
: review+
bent.mozilla
: review-
Details | Diff | Splinter Review
6.91 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
27.57 KB, patch
Details | Diff | Splinter Review
50.31 KB, patch
davidb
: review+
Details | Diff | Splinter Review
3.29 KB, patch
davidb
: review+
Details | Diff | Splinter Review
1.62 KB, patch
davidb
: review+
Details | Diff | Splinter Review
169.92 KB, text/x-log
Details
5.09 KB, patch
davidb
: review+
Details | Diff | Splinter Review
have a tree of documents that are in content processes, update them with ipc messages and tell the parent process the type of accessible events being fired on documents in content processes.
this patch could certainly use some style cleanup, I think that's mostly stylistic though.

I'm not really a fan of the way ipc more or less owns the child / parent objects but that's not explicit anywere.
I'm not really sure we send delete of the document at the right spot during shutdown but we can figure that out later, however it does have to come after we shut down the kid docs.
Attachment #8390050 - Flags: feedback?(surkov.alexander)
Attachment #8390050 - Flags: feedback?(bent.mozilla)
Having a patch description, class hierarchy, taken approach would be helpful. I didn't touch IPC for a while.
Posted patch initial a11y ipc impl (obsolete) — Splinter Review
now with more comments
Attachment #8390050 - Attachment is obsolete: true
Attachment #8390050 - Flags: feedback?(surkov.alexander)
Attachment #8390050 - Flags: feedback?(bent.mozilla)
Attachment #8390714 - Flags: feedback?(surkov.alexander)
Attachment #8390714 - Flags: feedback?(bent.mozilla)
Comment on attachment 8390714 [details] [diff] [review]
initial a11y ipc impl

Review of attachment 8390714 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/ipc/DocAccessibleParent.h
@@ +64,5 @@
> +  }
> +
> +private:
> +  nsTArray<DocAccessibleParent*> mChildDocs;
> +  DocAccessibleParent* mParentDoc;

why do docaccessible parent contains links to docaccessible parents? shouldn't it be children objects?
Comment on attachment 8390714 [details] [diff] [review]
initial a11y ipc impl

Review of attachment 8390714 [details] [diff] [review]:
-----------------------------------------------------------------

it's hard to say certainly because the uncomplete patch rise more questions than give answers but I don't spot any problems with the approach so f+. Some thoughts:

1) I would consider DocAccessibleIPCParent/Child names to avoid conflicts with accessible tree terms
2) I still don't see a clear usecase of having parent-chain hierarchy of DocAccParent objects
3) Not sure I understand why each process (per DocManager instance) needs to keep a copy of DocAccParent objects for root documents
4) I don't get how DocAccParent::RecvEvent taking event type only is supposed to fire events (no acc object)

I'd like to have something presumably working for review.
Attachment #8390714 - Flags: feedback?(surkov.alexander) → feedback+
> it's hard to say certainly because the uncomplete patch rise more questions
> than give answers but I don't spot any problems with the approach so f+.

I figure we'll find answers and more questions until we're done

> Some thoughts:
> 
> 1) I would consider DocAccessibleIPCParent/Child names to avoid conflicts
> with accessible tree terms

maybe, its longer though and DocAccessibleIPCChild ipcDoc is even more repeditive

> 2) I still don't see a clear usecase of having parent-chain hierarchy of
> DocAccParent objects

maybe we'll end up with doc accessible parent only knowing about parent accessible and children accessibles, but I figure we'll see how it goes and this setup is sane enough.

> 3) Not sure I understand why each process (per DocManager instance) needs to
> keep a copy of DocAccParent objects for root documents

the list is empty accept in the main process.  Sure we could have different class or something for doc manager in child process, but this just seems easier.

> 4) I don't get how DocAccParent::RecvEvent taking event type only is
> supposed to fire events (no acc object)

for now it can't really be useful, but to have an accessible we need to get full tree updating working which makes work much larger.

> I'd like to have something presumably working for review.

I'm not sure how you define presumably working, but it seems to me making everything work in one giant patch won't be much fun.
Comment on attachment 8390714 [details] [diff] [review]
initial a11y ipc impl

Review of attachment 8390714 [details] [diff] [review]:
-----------------------------------------------------------------

Generally this looks fine, though you probably want to override DocAccessibleParent::ActorDestroy as well (soon those overrides will be required I hope). All the messages go in one direction so there's no possibility for racing. Looks good to me (though tbh I have no idea how the a11y code works ;) ).
Attachment #8390714 - Flags: feedback?(bent.mozilla) → feedback+
> Generally this looks fine, though you probably want to override
> DocAccessibleParent::ActorDestroy as well (soon those overrides will be

I'm not sure what needs to happen there in this case DeallocPDocAccessible() still gets called on each actor even in case of an error right?  I guess I could move the first part of DeallocPDocAccessible to ActorDestroy() if that would make more sense though.

> required I hope). All the messages go in one direction so there's no

That is going to change but  all the parent -> child stuff will be morally const accessors, where races can be resolved by saying that data doesn't exist anymore sorry.

> possibility for racing. Looks good to me (though tbh I have no idea how the
> a11y code works ;) ).

I can try to explain if it becomes necessary, but I'm not sure it is yet.
So, this used to work, and I believe its correct, but I'm now seeing the follow error

IPDL protocol error: could not look up PDocAccessible
IPDL protocol error: Error deserializing 'PDocAccessibleParent'
[Parent 29413] ###!!! ASSERTION: IPDL error [PContentParent]: "Error deserializing 'PDocAccessibleParent'". Killing child side as a result.: 'Error', file /src/moz0/ipc/glue/ProtocolUtils.cpp, line 190

which makes no sense, but I'm trying to debug it.
Attachment #8390714 - Attachment is obsolete: true
Attachment #8424124 - Attachment is obsolete: true
Attachment #8425653 - Flags: review?(dbolter)
update: despite appearances this patch review is on my priority list.
Where are the ProxyAccessible.* files?
Posted patch initial a11y ipc impl (obsolete) — Splinter Review
add some more new files
Attachment #8425653 - Attachment is obsolete: true
Attachment #8425653 - Flags: review?(dbolter)
Attachment #8428847 - Flags: review?(dbolter)
Comment on attachment 8428847 [details] [diff] [review]
initial a11y ipc impl

Review of attachment 8428847 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, let's go!  (please check nits and questions)

Please get the thumbs up from a DOM peer for dom/ipc/

Some general nits:
There are some spacing/formatting issues.
Do you need to land the fprintfs?

I'm a rusty reviewer and you and Alex (and Neil) are much better at catching stuff... so I'm worried I didn't catch anything significant here.

::: accessible/src/base/EventQueue.cpp
@@ +560,5 @@
> +    if (ipcDoc) {
> +      if (event->mEventType == nsIAccessibleEvent::EVENT_SHOW)
> +        ipcDoc->ShowEvent(downcast_accEvent(event));
> +      else
> +      ipcDoc->SendEvent(event->GetEventType());

nit: needs more indent.

(/me grumbles about wanting {}'s even for single line if blocks)

::: accessible/src/base/NotificationController.cpp
@@ +223,5 @@
> +    childDoc->SetIPCDoc(ipcDoc);
> +    auto contentChild = dom::ContentChild::GetSingleton();
> +    DocAccessibleChild* parentIPCDoc = mDocument->IPCDoc();
> +    contentChild->SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, reinterpret_cast<uintptr_t>(outerDocAcc->UniqueID()));
> +  }

why not reinterpret_case<uint64_t>?
nit: previous 5 lines need several more indents.

::: accessible/src/base/Role.h
@@ +784,5 @@
>     * Represent a keyboard or keypad key (ARIA role "key").
>     */
> +  KEY = 129,
> +
> +    LAST_ROLE = KEY

nit: too much indent

::: accessible/src/generic/DocAccessible.cpp
@@ +618,5 @@
>  
>    mChildDocuments.Clear();
>  
> +  // XXX thinking about ordering?
> +  DocAccessibleChild::Send__delete__(mIPCDoc);

Should we be worried?

::: accessible/src/ipc/DocAccessibleChild.h
@@ +26,5 @@
> +  { MOZ_COUNT_CTOR(DocAccessibleChild); }
> +  ~DocAccessibleChild()
> +  {
> +    mDoc->SetIPCDoc(nullptr);
> + MOZ_COUNT_DTOR(DocAccessibleChild);

nit: add indent.

::: accessible/src/ipc/DocAccessibleParent.cpp
@@ +30,5 @@
> +    return false;
> +  }
> +
> +  return this->AddSubtree(parent, aData.NewTree(), 0, newChildIdx);
> +  }

nit: remove indent for }

@@ +51,5 @@
> +
> +  auto role = static_cast<a11y::role>(newChild.Role());
> +                   ProxyAccessible* newProxy =
> +                     new ProxyAccessible(aParent, this, role, newChild.Name());
> +                   aParent->AddChildAt(aIdxInParent, newProxy);

nit: last few statements should only be indented 2 spaces.

@@ +57,5 @@
> +
> +  uint32_t accessibles = 1;
> +  uint32_t kids = newChild.ChildrenCount();
> +  for (uint32_t i = 0; i < kids; i++) {
> +    uint32_t consumed = AddSubtree(newProxy, aNewTree, aIdx + 1, i);

Beautifulish

@@ +78,5 @@
> +    NS_ERROR("invalid root being removed!");
> +    return false;
> +  }
> +
> +  aRoot->Shutdown();

Okay - I forgot that Hide means really going away away, sometimes.

::: accessible/src/ipc/DocAccessibleParent.h
@@ +14,5 @@
> +#include "nsHashKeys.h"
> +#include "nsISupportsImpl.h"
> +
> +namespace mozilla {
> +  namespace a11y {

nit: an additional indent crept in the line above and messed up a few subsequent lines.

::: accessible/src/ipc/PDocAccessible.ipdl
@@ +31,5 @@
> +parent:
> +  __delete__();
> +
> +  /*
> +   * Notifiy the parent process the document in the child process is firing an

nit: "Notify"

::: accessible/src/ipc/ProxyAccessible.h
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace a11y {
> +
> +  class DocAccessibleParent;

nit: spacing

@@ +55,5 @@
> +
> +private:
> +  nsTArray<ProxyAccessible*> mChildren;
> +  DocAccessibleParent* mDoc;
> +  role mRole : 31;

31?

::: dom/ipc/ContentChild.cpp
@@ +666,5 @@
> +a11y::PDocAccessibleChild*
> +ContentChild::AllocPDocAccessibleChild(PDocAccessibleChild*, const uint64_t&)
> +{
> +  MOZ_ASSERT(false, "should never call this!");
> +    return nullptr;

nit: remove extra indent.

@@ +673,5 @@
> +bool
> +ContentChild::DeallocPDocAccessibleChild(a11y::PDocAccessibleChild* aChild)
> +{
> +    delete static_cast<mozilla::a11y::DocAccessibleChild*>(aChild);
> +    return true;

nit: remove extra indents.

::: dom/ipc/ContentParent.cpp
@@ +2319,5 @@
> +    parentDoc->RemoveChildDoc(doc);
> +  } else {
> +    GetAccService()->RemoteDocShutdown(doc);
> +  }
> +    

nit: remove spaces.

@@ +2338,5 @@
> +    GetAccService()->RemoteDocAdded(doc);
> +  }
> +  return true;
> +}
> +  

nit: remove spaces
Attachment #8428847 - Flags: review?(dbolter) → review+
Attachment #8428847 - Attachment is obsolete: true
Attachment #8429464 - Flags: review?(bent.mozilla)
(In reply to David Bolter [:davidb] from comment #14)
> Do you need to land the fprintfs?

I'd prefer to until there's real implementations of stuff, and this code is better tested.

> ::: accessible/src/base/NotificationController.cpp
> @@ +223,5 @@
> > +    childDoc->SetIPCDoc(ipcDoc);
> > +    auto contentChild = dom::ContentChild::GetSingleton();
> > +    DocAccessibleChild* parentIPCDoc = mDocument->IPCDoc();
> > +    contentChild->SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, reinterpret_cast<uintptr_t>(outerDocAcc->UniqueID()));
> > +  }
> 
> why not reinterpret_case<uint64_t>?

I'm not really sure what happens on 32 bit arches when you try to reinterpret_cast a 32 bit pointer to a 64 bit int, it might just not compile.  So we cast to a integer type the same size as a pointer and then if the final int type is larger we let integer promotion make sure the high bits are 0.

> ::: accessible/src/generic/DocAccessible.cpp
> @@ +618,5 @@
> >  
> >    mChildDocuments.Clear();
> >  
> > +  // XXX thinking about ordering?
> > +  DocAccessibleChild::Send__delete__(mIPCDoc);
> 
> Should we be worried?

I'm not really sure, it probably doesn't effect anything important, but I remember atk does something weird when root docs shut down, and I won't be suprised if we need to tweek this a little.

> @@ +55,5 @@
> > +
> > +private:
> > +  nsTArray<ProxyAccessible*> mChildren;
> > +  DocAccessibleParent* mDoc;
> > +  role mRole : 31;
> 
> 31?

just convenient number, it could be 8, and I guess a static assert wouldn't hurt, but shrug.
Comment on attachment 8429464 [details] [diff] [review]
implement cross process accessible tree updating

Review of attachment 8429464 [details] [diff] [review]:
-----------------------------------------------------------------

Andrea, would you mind giving this a first pass? Let me know if you have any questions and I'll be happy to jump in.
Attachment #8429464 - Flags: review?(bent.mozilla) → review?(amarchesini)
Comment on attachment 8429464 [details] [diff] [review]
implement cross process accessible tree updating

Review of attachment 8429464 [details] [diff] [review]:
-----------------------------------------------------------------

For bent: It's not fully clear to me the life-time of DocAccessibleChild objects.
Trevor, if you want to give me a feedback about these comments, I can proceed with a second review of your patch.

::: accessible/src/base/DocManager.cpp
@@ +422,5 @@
>                               ApplicationAcc());
>  
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +      DocAccessibleChild* ipcDoc = new DocAccessibleChild(docAcc);
> +      docAcc->SetIPCDoc(ipcDoc);

This setter can be done in the constructor of DocAccessibleChild.

@@ +423,5 @@
>  
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +      DocAccessibleChild* ipcDoc = new DocAccessibleChild(docAcc);
> +      docAcc->SetIPCDoc(ipcDoc);
> +    auto contentChild = dom::ContentChild::GetSingleton();

indentation is wrong here and in the next line.

::: accessible/src/base/NotificationController.cpp
@@ +219,5 @@
>        if (outerDocAcc && outerDocAcc->AppendChild(childDoc)) {
> +        if (mDocument->AppendChildDocument(childDoc)) {
> +          if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +            DocAccessibleChild* ipcDoc = new DocAccessibleChild(childDoc);
> +            childDoc->SetIPCDoc(ipcDoc);

This setter can be moved in the DocAccessibileChild constructor.

::: accessible/src/generic/DocAccessible.cpp
@@ +617,5 @@
>      mChildDocuments[idx]->Shutdown();
>  
>    mChildDocuments.Clear();
>  
> +  // XXX thinking about ordering?

can it be that mIPCDoc is null at this point?
This should be done only if the DocAccessible doesn't run on the main-process.

::: accessible/src/generic/DocAccessible.h
@@ +625,5 @@
>  
>  private:
>  
>    nsIPresShell* mPresShell;
> +  DocAccessibleChild* mIPCDoc;

should it be a nsRefPtr ? What's the life-time of this object?

::: accessible/src/ipc/DocAccessibleParent.cpp
@@ +35,5 @@
> +
> +uint32_t
> +DocAccessibleParent::AddSubtree(ProxyAccessible* aParent,
> +                                const nsTArray<a11y::AccessibleData>& aNewTree,
> +                                uint32_t aIdx, uint32_t aIdxInParent)

I would make this method like this:
bool DocAccessibleParent::AddSubTree(ProxyAccessible* aParent, const nsTArray<a11y::AccessibleData>& aNewTree, uint32_t aIdx, uint32_t aIdxInParent, uint32_t* aConsumed);

aConsumed can be null or not. RecvShowEvent() would use this method with aConsumed == nullptr.

@@ +57,5 @@
> +
> +  uint32_t accessibles = 1;
> +  uint32_t kids = newChild.ChildrenCount();
> +  for (uint32_t i = 0; i < kids; i++) {
> +    uint32_t consumed = AddSubtree(newProxy, aNewTree, aIdx + 1, i);

And this would be:

uint32_t consumed;
if (!AddSubtree(newProxy, aNewTree, aIdx + 1, i, &consumed)) {
  return false;
}

accessibles += consumed;

::: accessible/src/ipc/DocAccessibleParent.h
@@ +40,5 @@
> +   * process it is firing an event.
> +   */
> +  virtual bool RecvEvent(const uint32_t& aType) MOZ_OVERRIDE
> +  {
> +    fprintf(stderr, "\n***\naccessible event type is %x\n***\n", aType);

this is a debug message, I guess. maybe you should add some #ifdef DEBUG

::: accessible/src/ipc/ProxyAccessible.h
@@ +28,5 @@
> +  }
> +  ~ProxyAccessible() { MOZ_COUNT_DTOR(ProxyAccessible); }
> +
> +  void AddChildAt(uint32_t aIdx, ProxyAccessible* aChild)
> +  { mChildren.InsertElementAt(aIdx, aChild); }

nit: mChildren.InsertElementAt(aIdx, aChild in a new line.

::: dom/ipc/ContentParent.cpp
@@ +2302,5 @@
>  
>      return NS_OK;
>  }
>  
> +  a11y::PDocAccessibleParent*

extra space before a11y

@@ +2305,5 @@
>  
> +  a11y::PDocAccessibleParent*
> +ContentParent::AllocPDocAccessibleParent(PDocAccessibleParent* aParent, const uint64_t&)
> +{
> +  fprintf(stderr, "\a\a\a\a\n*****\nnew a11y doc\n******\a\a\a\n");

Is this fprintf() a debug message?

@@ +2320,5 @@
> +  } else {
> +    GetAccService()->RemoteDocShutdown(doc);
> +  }
> +
> +  delete static_cast<a11y::DocAccessibleParent*>(aParent);

delete doc;

@@ +2327,5 @@
> +
> +bool
> +ContentParent::RecvPDocAccessibleConstructor(PDocAccessibleParent* aDoc, PDocAccessibleParent* aParentDoc, const uint64_t& aParentID)
> +{
> +  fprintf(stderr, "\n***\nconstructing new ipc doc accessible at %p parent at %p\n", aDoc, aParentDoc);

fprintf - read above.
Attachment #8429464 - Flags: review?(amarchesini)
> For bent: It's not fully clear to me the life-time of DocAccessibleChild
> objects.

it should be the same as the DocAccessible they're related to.

> ::: accessible/src/base/DocManager.cpp
> @@ +422,5 @@
> >                               ApplicationAcc());
> >  
> > +    if (XRE_GetProcessType() != GeckoProcessType_Default) {
> > +      DocAccessibleChild* ipcDoc = new DocAccessibleChild(docAcc);
> > +      docAcc->SetIPCDoc(ipcDoc);
> 
> This setter can be done in the constructor of DocAccessibleChild.

that's a little magical for my taste, but maybe

> ::: accessible/src/generic/DocAccessible.cpp
> @@ +617,5 @@
> >      mChildDocuments[idx]->Shutdown();
> >  
> >    mChildDocuments.Clear();
> >  
> > +  // XXX thinking about ordering?
> 
> can it be that mIPCDoc is null at this point?

only if its in the main process.

> This should be done only if the DocAccessible doesn't run on the
> main-process.

yeah, oops

> ::: accessible/src/generic/DocAccessible.h
> @@ +625,5 @@
> >  
> >  private:
> >  
> >    nsIPresShell* mPresShell;
> > +  DocAccessibleChild* mIPCDoc;
> 
> should it be a nsRefPtr ? What's the life-time of this object?

basically we own it.  So it could be a nsAutoPtr for documentation purposes, but practically it won't help anything I think.

> ::: accessible/src/ipc/DocAccessibleParent.cpp
> @@ +35,5 @@
> > +
> > +uint32_t
> > +DocAccessibleParent::AddSubtree(ProxyAccessible* aParent,
> > +                                const nsTArray<a11y::AccessibleData>& aNewTree,
> > +                                uint32_t aIdx, uint32_t aIdxInParent)
> 
> I would make this method like this:
> bool DocAccessibleParent::AddSubTree(ProxyAccessible* aParent, const
> nsTArray<a11y::AccessibleData>& aNewTree, uint32_t aIdx, uint32_t
> aIdxInParent, uint32_t* aConsumed);
> 
> aConsumed can be null or not. RecvShowEvent() would use this method with
> aConsumed == nullptr.

honestly that seems more complicated to me.

> ::: accessible/src/ipc/DocAccessibleParent.h
> @@ +40,5 @@
> > +   * process it is firing an event.
> > +   */
> > +  virtual bool RecvEvent(const uint32_t& aType) MOZ_OVERRIDE
> > +  {
> > +    fprintf(stderr, "\n***\naccessible event type is %x\n***\n", aType);
> 
> this is a debug message, I guess. maybe you should add some #ifdef DEBUG

I could, but once we've made this stuff actually useful I intend to remove it because its pretty spammy so I'm not sure there's a point in bothering.

> ::: accessible/src/ipc/ProxyAccessible.h
> @@ +28,5 @@
> > +  }
> > +  ~ProxyAccessible() { MOZ_COUNT_DTOR(ProxyAccessible); }
> > +
> > +  void AddChildAt(uint32_t aIdx, ProxyAccessible* aChild)
> > +  { mChildren.InsertElementAt(aIdx, aChild); }
> 
> nit: mChildren.InsertElementAt(aIdx, aChild in a new line.

afaik that's a valid way to write a one line inline method.
> > This should be done only if the DocAccessible doesn't run on the
> > main-process.
> 
> yeah, oops

Good. Fix this, and for the rest it's r+ for me.

> > nit: mChildren.InsertElementAt(aIdx, aChild in a new line.
> 
> afaik that's a valid way to write a one line inline method.

Yeah. The style guidelines are not clear to me. Sometimes people complain about this one line methods, and sometimes they don't.

I would like bent to take a look at the life-time of the DocAccessibleChild/Parent objects. I'm not 100% sure this is safe enough and bent knows more about how to detect this kind of issues.
Flags: needinfo?(bent.mozilla)
Attachment #8429464 - Flags: review+
Flags: needinfo?(bent.mozilla)
Heyo Ben, any estimate on review? We have a Q2 goal I may need to mark as at risk ;)
Flags: needinfo?(bent.mozilla)
Comment on attachment 8429464 [details] [diff] [review]
implement cross process accessible tree updating

Review of attachment 8429464 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see anywhere here where you're calling 'PDocAccessibleChild::SendShowEvent', which means that you're never getting 'DocAccessibleParent::RecvShowEvent()' called in the parent... Has this code been tested?

::: accessible/src/base/DocManager.h
@@ +72,5 @@
> +   * Notification that a top level document in a content process has gone away.
> +   */
> +  void RemoteDocShutdown(DocAccessibleParent* aDoc)
> +  {
> +    mRemoteDocuments.RemoveElement(aDoc);

I would assert that 'aDoc' exists in mRemoteDocuments before calling RemoveElement. And asserting the opposite below in RemoteDocAdded.

::: accessible/src/generic/DocAccessible.h
@@ +625,5 @@
>  
>  private:
>  
>    nsIPresShell* mPresShell;
> +  DocAccessibleChild* mIPCDoc;

I would just comment here to say that 'mIPCDoc' is exclusively owned by IPDL and that it cannot be manually deleted.

::: accessible/src/ipc/DocAccessibleChild.h
@@ +15,5 @@
> +namespace a11y {
> +class AccShowEvent;
> +
> +  /*
> +   * These objects handle content side communication for a accessible document,

Nit: s/a /an /

::: dom/ipc/ContentParent.cpp
@@ +2306,5 @@
> +  a11y::PDocAccessibleParent*
> +ContentParent::AllocPDocAccessibleParent(PDocAccessibleParent* aParent, const uint64_t&)
> +{
> +  fprintf(stderr, "\a\a\a\a\n*****\nnew a11y doc\n******\a\a\a\n");
> +  return new a11y::DocAccessibleParent;

Please add () here.

@@ +2317,5 @@
> +  a11y::DocAccessibleParent* parentDoc = doc->Parent();
> +  if (parentDoc) {
> +    parentDoc->RemoveChildDoc(doc);
> +  } else {
> +    GetAccService()->RemoteDocShutdown(doc);

You do all this already in ActorDestroy.. Generally 'Dealloc' methods just do delete/Release and any real work happens in ActorDestroy.

::: dom/ipc/PContent.ipdl
@@ +410,5 @@
>       * Notify idle observers in the child
>       */
>      NotifyIdleObserver(uint64_t observerId, nsCString topic, nsString str);
>  parent:
> +    PDocAccessible(nullable PDocAccessible aParent, uint64_t aParentAcc);

What does 'aParentAcc' mean? This needs a comment or better name.
Attachment #8429464 - Flags: review?(bent.mozilla) → review-
(In reply to David Bolter [:davidb] from comment #21)
> Heyo Ben, any estimate on review? We have a Q2 goal I may need to mark as at
> risk ;)

Sorry for the delay!
Flags: needinfo?(bent.mozilla)
Posted patch fixupsSplinter Review
This is an iterdiff on top of the old patch, I'll attach a full patch for reference too.
Attachment #8446695 - Flags: review?(bent.mozilla)
> I don't see anywhere here where you're calling
> 'PDocAccessibleChild::SendShowEvent', which means that you're never getting
> 'DocAccessibleParent::RecvShowEvent()' called in the parent... Has this code
> been tested?

that's totally stupid on my part!  I tested it some, but its tricky to test without writing more stuff on top of it, and the easy testing assumed this part worked.  However after fixing this I did check things were still not asserting, and at this point testing more than that requires checking a giant data tree is correct by poking at in a debugger.  It seems to me its easier to add infrastructure on top of this to use it, which will make finding bugs easier, and then test more fully.
Comment on attachment 8446695 [details] [diff] [review]
fixups

Review of attachment 8446695 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these fixed:

::: accessible/src/base/DocManager.h
@@ +73,5 @@
>     */
>    void RemoteDocShutdown(DocAccessibleParent* aDoc)
>    {
> +    MOZ_ASSERT(mRemoteDocuments.RemoveElement(aDoc),
> +               "Why didn't we find the document!");

MOZ_ASSERT goes away entirely in opt builds so this won't work. You want MOZ_ALWAYS_TRUE.

::: dom/ipc/ContentParent.cpp
@@ +2305,5 @@
>  
>    a11y::PDocAccessibleParent*
>  ContentParent::AllocPDocAccessibleParent(PDocAccessibleParent* aParent, const uint64_t&)
>  {
>    fprintf(stderr, "\a\a\a\a\n*****\nnew a11y doc\n******\a\a\a\n");

All these need to be removed of course.
Attachment #8446695 - Flags: review?(bent.mozilla) → review+
Attachment #8489635 - Flags: review?(dbolter)
Comment on attachment 8489635 [details] [diff] [review]
initial a11y ipc impl

Review of attachment 8489635 [details] [diff] [review]:
-----------------------------------------------------------------

Patch special review caveats:
- I'm rusty (but willing!)
- I'm assuming the scrutiny bar is low here while you iterate.
- I'm assuming you want to get it going enough on Linux before tackling Windows etc.
- I know you want to get this landed soon and get help debugging the tree update stuff.
- I don't think this regresses non-e10s.

r=me.

I'd like Alex to at consume this patch one way or another.

::: accessible/atk/AccessibleWrap.cpp
@@ +139,4 @@
>  };
>  
> +// This is ord with the pointer in MaiAtkObject::accWrap if the wrappie is a
> +// proxy.

nit: I'd spell this "or'd" and "wrap-ee".

@@ +139,5 @@
>  };
>  
> +// This is ord with the pointer in MaiAtkObject::accWrap if the wrappie is a
> +// proxy.
> +static const uintptr_t IS_PROXY = 1;

I'll trust you we can safely steal this bit.

@@ +672,5 @@
>      return aAtkObj->role;
>  
> +  AccessibleWrap* accWrap = GetAccessibleWrap(aAtkObj);
> +  a11y::role role;
> +  if (!accWrap) {

OK. I wonder why we used to do this block before the simpler 'return aAtkObj->role' case... (and if that matters)

::: accessible/ipc/DocAccessibleChild.cpp
@@ +30,5 @@
> +  uint64_t parentID = parent->IsDoc() ? 0 : reinterpret_cast<uint64_t>(parent->UniqueID());
> +  uint32_t idxInParent = aShowEvent->GetAccessible()->IndexInParent();
> +  nsTArray<AccessibleData> shownTree;
> +  ShowEventData data(parentID, idxInParent, shownTree);
> +  SerializeTree(aShowEvent->GetAccessible(), data.NewTree());

(note to self: this NewTree() syntax is weird to me)

::: accessible/ipc/DocAccessibleParent.h
@@ +18,5 @@
> +namespace a11y {
> +
> +/*
> + * These objects live in the main process and comunicate with and represent
> + * a accessible document in a content process.

nit: "an" accessible

@@ +68,5 @@
> +  bool AddChildDoc(DocAccessibleParent* aChildDoc, uint64_t aParentID)
> +  {
> +    ProxyAccessible* outerDoc = mAccessibles.GetEntry(aParentID)->mProxy;
> +    if (!outerDoc)
> +      return false;

As per chat, should we do something with child process if this fails?
Attachment #8489635 - Flags: review?(dbolter) → review+
Posted patch fixupsSplinter Review
Attachment #8491041 - Flags: review?(dbolter)
Comment on attachment 8491041 [details] [diff] [review]
fixups

Review of attachment 8491041 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

::: accessible/ipc/DocAccessibleParent.cpp
@@ +21,5 @@
>  
> +  ProxyAccessible* parent = nullptr;
> +  if (aData.ID()) {
> +    ProxyEntry* e = mAccessibles.GetEntry(aData.ID());
> +    if (e)

Ah hmm. Curious when do we expect e to be null?

::: dom/ipc/ContentParent.cpp
@@ +2784,5 @@
>    auto doc = static_cast<a11y::DocAccessibleParent*>(aDoc);
>    if (aParentDoc) {
>      MOZ_ASSERT(aParentID);
> +    auto parentDoc = static_cast<a11y::DocAccessibleParent*>(aParentDoc);
> +    return parentDoc->AddChildDoc(doc, aParentID);

OK.
Attachment #8491041 - Flags: review?(dbolter) → review+
> ::: accessible/ipc/DocAccessibleParent.cpp
> @@ +21,5 @@
> >  
> > +  ProxyAccessible* parent = nullptr;
> > +  if (aData.ID()) {
> > +    ProxyEntry* e = mAccessibles.GetEntry(aData.ID());
> > +    if (e)
> 
> Ah hmm. Curious when do we expect e to be null?

its part of the below comment.  if we miss a show evnet then the subtree ofr it never gets created in the parent, so if then you insert content in that subtree the parent has no e.
https://hg.mozilla.org/mozilla-central/rev/a11adf1705ec
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1075049
sorry had to back this out since this seems to have caused 1075387 and backout was requested to unbreak the builds
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #36)
> Were those crashes also caused by this one?
> https://crash-stats.mozilla.com/report/
> list?signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3AAddChildDoc%28
> mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%2A%2C%20unsigned%20__int64%29

Yeah looks like it. Trevor it seems mAccessibles.GetEntry(aParentID) can fail in AddChildDoc.
Depends on: 1075387
It turns out I forgot to remove the target of a hide event from its parent so we kept around pointers to deleted objects.
Attachment #8501393 - Flags: review?(dbolter)
Comment on attachment 8501393 [details] [diff] [review]
fix crash in ProxyAccessible::Shutdown

Review of attachment 8501393 [details] [diff] [review]:
-----------------------------------------------------------------

Oops. r=me

::: accessible/ipc/DocAccessibleParent.cpp
@@ +104,5 @@
>      return true;
>    }
>  
> +  ProxyAccessible* parent = root->Parent();
> +  parent->RemoveChild(root);

I guess I can't imagine parent failing.

::: accessible/ipc/ProxyAccessible.h
@@ +41,5 @@
>    /**
> +   * Remove The given child.
> +   */
> +  void RemoveChild(ProxyAccessible* aChild)
> +    { mChildren.RemoveElement(aChild); }

The indent here seems unnecessary.
Attachment #8501393 - Flags: review?(dbolter) → review+
Posted file sms_crash.log
Something in this commit (https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=54dea8172514) is causing on-device tests (Flame) to fail.

The application under test is crashing out.

I've attached a logcat which shows the following error after launching the Messages app:

10-15 09:06:13.660 I/Gecko   ( 1968): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x3C0004,name=PDocAccessible::Msg_ShowEvent) Processing error: message was deserialized, but the handler returned false (indicating failure)

There is no crash report, the Messages app just dies and continues to crash on successive launches. The device is otherwise usable.

A second, similar Settings app crash found in automation is when enabling Bluetooth in the Settings app.

Both started failing with this Flame build:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=54dea8172514


Tomcat, can you hold off on merging this into m-c/b2g-i until it can be diagnosed or backed out?
Flags: needinfo?(cbook)
(In reply to Zac C (:zac) from comment #41)
> 10-15 09:06:13.660 I/Gecko   ( 1968): ###!!! [Parent][DispatchAsyncMessage]
> Error: (msgtype=0x3C0004,name=PDocAccessible::Msg_ShowEvent) Processing
> error: message was deserialized, but the handler returned false (indicating
> failure)

Zac are you using the ffos screen reader? (Or instantiating accessibility in some other way?)
backed this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=49b06fb31f0b together with the other landings from that push (and bug 1074917 which caused conflicts during backout)
Flags: needinfo?(cbook)
Flags: needinfo?(zcampbell)
Yura tells me we probably enable a11y in gaia-ui tests even if the screen reader is off. I'm curious to know if comment 41 is picked up picked up by automated tests.
(In reply to David Bolter [:davidb] from comment #42)
> (In reply to Zac C (:zac) from comment #41)
> > 10-15 09:06:13.660 I/Gecko   ( 1968): ###!!! [Parent][DispatchAsyncMessage]
> > Error: (msgtype=0x3C0004,name=PDocAccessible::Msg_ShowEvent) Processing
> > error: message was deserialized, but the handler returned false (indicating
> > failure)
> 
> Zac are you using the ffos screen reader? (Or instantiating accessibility in
> some other way?)

We do have that kind of code (which Yura wrote) but it shouldn't be touched by these functional tests. So not that I'm aware of, but you've given me an avenue to explore. I'll spend some more time on it today and let know (keeping my ni? until then).
OK I've diagnosed it, found the offending line of code.

I begun by commenting out parts of a11y code from the framework until the tests started working again.

I narrowed it down to this line:
https://github.com/mozilla-b2g/gaia/blob/5c636a7a54b2c86d8ff6bc1aa1e5f9594c7bc586/tests/atoms/accessibility.js#L11

When I comment out _accRetrieval and re-run the tests they pass successfully (but obviously this would break the a11y tests).

Even though the functioanl tests do not use this a11y code it is imported by Marionette so that it's ready to use by the a11y tests. I guess this is clashing with something.

I'll ni? Yura, to explain what this code does and continue with the diagnosis.
Flags: needinfo?(zcampbell) → needinfo?(yzenevich)
and to answer David's ni? properly, accessibility.screenreader was definitely false!
I just spent more time on this working through some ideas that yzen gave me, and eventually I have worked out how to replicate the failure manually:

STR:
1. Open Settings app
2. Open dev settings and enable visibility of screen reader settings
3. Go back to Settings Accessibility menu
4. Enable screen reader
5. Tap home button
6. Load Contacts app (using screen reader commands)

Contacts app will crash immediately upon being loaded.

Gaia-Rev        5f1f0960ae9d22acf2a324ad37a48174d6df87f6
Gecko-Rev       https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2a1889250e
Build-ID        20141015055031
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141015.073454
FW-Date         Wed Oct 15 07:35:05 EDT 2014
Bootloader      L1TC00011840
Flags: needinfo?(yzenevich)
Attachment #8506562 - Flags: review?(dbolter)
(In reply to Zac C (:zac) from comment #48)
> I just spent more time on this working through some ideas that yzen gave me,
> and eventually I have worked out how to replicate the failure manually:


Well, I have no interest in debugging with the B2G toolchain, and less interest in manually vs with tests, but fortunately I don't actually need to run this code on b2g so I'll just let someone else deal with your tests.
Comment on attachment 8506562 [details] [diff] [review]
disable ipc accessibility on B2G

Review of attachment 8506562 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I didn't test it.

::: accessible/base/nsAccessibilityService.h
@@ +241,5 @@
>  /**
> + * Return true if we're in a content process and not B2G.
> + */
> +inline bool
> +IPCAccessibilityActive()

Hmmm... this name doesn't tell me what it does. How about:
IPCInContentProcessAndNotB2G?
Attachment #8506562 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #51)
> Comment on attachment 8506562 [details] [diff] [review]
> disable ipc accessibility on B2G
> 
> Review of attachment 8506562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me but I didn't test it.

I checked we still do the right thing on desktop.  I don't see how it can break anything on B2G since they don't need and never had this stuff anyway.

> ::: accessible/base/nsAccessibilityService.h
> @@ +241,5 @@
> >  /**
> > + * Return true if we're in a content process and not B2G.
> > + */
> > +inline bool
> > +IPCAccessibilityActive()
> 
> Hmmm... this name doesn't tell me what it does. How about:
> IPCInContentProcessAndNotB2G?

Seems kind of long, and I kind of hope it doesn't stay around forever?
https://hg.mozilla.org/mozilla-central/rev/dd29a568fb2f
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla35 → mozilla36
Depends on: 1087498
You need to log in before you can comment on or make changes to this bug.