1075049 - Builds with "ac_add_options --disable-accessibility" are broken, with "AttributeError: 'NoneType' object has no attribute 'filetype'", due to PContent.ipdl including PDocAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 Try to build with this in your .mozconfig:
ac_add_options --disable-accessibility


ACTUAL RESULTS:
{
 0:04.94   in file included from `/scratch/work/builds/mozilla-inbound/mozilla/content/media/gmp/PGMP.ipdl', line 8:
 0:04.95   in file included from `/scratch/work/builds/mozilla-inbound/mozilla/dom/ipc/PCrashReporter.ipdl', line 7:
 0:04.95 /scratch/work/builds/mozilla-inbound/mozilla/dom/ipc/PContent.ipdl:16: error: can't locate include file `PDocAccessible.ipdl'
 0:05.25 Traceback (most recent call last):
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/config/pythonpath.py", line 56, in <module>
 0:05.25     main(sys.argv[1:])
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/config/pythonpath.py", line 48, in main
 0:05.25     execfile(script, frozenglobals)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl.py", line 132, in <module>
 0:05.25     if not ipdl.typecheck(ast):
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/__init__.py", line 35, in typecheck
 0:05.25     return TypeCheck().check(ast, errout)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/type.py", line 600, in check
 0:05.25     if not runpass(GatherDecls(builtinUsing, self.errors)):
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/type.py", line 592, in runpass
 0:05.25     tu.accept(tcheckpass)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/ast.py", line 135, in accept
 0:05.25     return visit(self)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/type.py", line 698, in visitTranslationUnit
 0:05.25     pinc.accept(self)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/ast.py", line 135, in accept
 0:05.25     return visit(self)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/type.py", line 762, in visitInclude
 0:05.25     inc.tu.accept(self)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/ast.py", line 135, in accept
 0:05.25     return visit(self)
 0:05.25   File "/scratch/work/builds/mozilla-inbound/mozilla/ipc/ipdl/ipdl/type.py", line 715, in visitTranslationUnit
 0:05.25     if inc.tu.filetype == 'header':
 0:05.25 AttributeError: 'NoneType' object has no attribute 'filetype'
}

hg bisect says this starts with mozilla-central changeset a11adf1705ec.

Comment 1

[Daniel Holbert [:dholbert]]

As discussed with tbsaunde & jdm in IRC: you can't have ifdefs in ipdl, so we can't just add ifdefs around the reference to PDocAccessible.ipdl.

It sounds like we have to unconditionally build PDocAccessible.ipdl (but with a stub implementation), or something like that.

(I think tbsaunde is working on this, or will be soon, from the sound of his discussion w/ jdm earlier.)

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: fix --disable-accessibility

Comment 4

[Michael Shal [:mshal]]

Comment on attachment 8498183 [details] [diff] [review]
fix --disable-accessibility

The build config changes look fine, but you'll probably want someone to review the dom code changes as well.

Will this be changing at all since the backout from bug 1075387?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Michael Shal [:mshal] from comment #4)
> Comment on attachment 8498183 [details] [diff] [review]
> fix --disable-accessibility
> 
> The build config changes look fine, but you'll probably want someone to
> review the dom code changes as well.

yeah, I'll find someone with a rubber stamp.

> 
> Will this be changing at all since the backout from bug 1075387?

no, though I bet it'll change for glandium's tier stuff.

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8498183 [details] [diff] [review]
fix --disable-accessibility

dholbert would you mind doing the honors for the ifdefs in dom/ipc/Contnet*? it should be pretty obvious they're correct ;)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8498183 [details] [diff] [review]
fix --disable-accessibility

[side note: bug 982842, which caused this build breakage, was backed out for causing another bug, and hasn't yet re-landed, so this patch doesn't currently apply on m-c.]

It looks like the dom/ipc ifdefs are just wrapping a11y-specific chunks that are added in attachment 8489635 [details] [diff] [review], which seems reasonable. 

One question:
>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
[...]
> bool
> ContentChild::DeallocPDocAccessibleChild(a11y::PDocAccessibleChild* aChild)
> {
>+#ifdef ACCESSIBILITY
>   delete static_cast<mozilla::a11y::DocAccessibleChild*>(aChild);
>+#endif
>   return true;
> }

In the unlikely event that we actually get a call to this method in a disable-a11y build, it looks like it'd leak.  That's probably bad.

Do we not need to worry about that because this is never called in that case? If so, maybe we should have an #else clause with a NS_ERROR?

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8498183 [details] [diff] [review]
fix --disable-accessibility

I don't really know what this code does, but I'm happy to take your word for it that this just needs a rubber-stamp level of review (per comment 5), so I've verified that these #ifdefs are indeed being added around a11y-flavored code that's new in bug 982842 (attachment 8489635 [details] [diff] [review] specifically).  Seems sane.

So, rs=me, modulo my question in previous comment.

(One other thought there -- maybe we really want to add something like...
  #else
    MOZ_ASSERT(!aParent, "how did our caller get a PDocAccessibleParent in a non-a11y build?")
  #endif

...if this function actually does get called in --disable-a11y builds, but with the nullptr provided from AllocPDocAccessibleParent?)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (One other thought there -- maybe we really want to add something like...
>   #else
>     MOZ_ASSERT(!aParent, "how did our caller get a PDocAccessibleParent in a non-a11y build?")
>   #endif

Er, I meant "PDocAccessibleChild" -- though this applies both to DeallocPDocAccessibleChild *and* DeallocPDocAccessibleParent.)

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Comment on attachment 8498183 [details] [diff] [review]
> fix --disable-accessibility
> 
> [side note: bug 982842, which caused this build breakage, was backed out for
> causing another bug, and hasn't yet re-landed, so this patch doesn't
> currently apply on m-c.]
> 
> It looks like the dom/ipc ifdefs are just wrapping a11y-specific chunks that
> are added in attachment 8489635 [details] [diff] [review], which seems
> reasonable. 
> 
> One question:
> >diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
> [...]
> > bool
> > ContentChild::DeallocPDocAccessibleChild(a11y::PDocAccessibleChild* aChild)
> > {
> >+#ifdef ACCESSIBILITY
> >   delete static_cast<mozilla::a11y::DocAccessibleChild*>(aChild);
> >+#endif
> >   return true;
> > }
> 
> In the unlikely event that we actually get a call to this method in a
> disable-a11y build, it looks like it'd leak.  That's probably bad.

Well, in that case there is no such type as DocAccessibleParent, and PDocAccessibleParent is abstract, so I'm not sure what type you'd have that we'd fail to free.

> Do we not need to worry about that because this is never called in that
> case? If so, maybe we should have an #else clause with a NS_ERROR?

it really shouldn't be called, I guess asserts can't hurt.

Comment 11

[Trevor Saunders (:tbsaunde)]

I squashed this into the relanding of bug 982842