Closed
Bug 1075049
Opened 11 years ago
Closed 10 years ago
Builds with "ac_add_options --disable-accessibility" are broken, with "AttributeError: 'NoneType' object has no attribute 'filetype'", due to PContent.ipdl including PDocAccessible
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: dholbert, Assigned: tbsaunde)
References
Details
Attachments
(1 file)
5.64 KB,
patch
|
mshal
:
review+
dholbert
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Summary: Builds with "ac_add_options --disable-accessibility" are broken, with "AttributeError: 'NoneType' object has no attribute 'filetype'" → Builds with "ac_add_options --disable-accessibility" are broken, with "AttributeError: 'NoneType' object has no attribute 'filetype'", due to PContent.ipdl including PDocAccessible
Reporter | ||
Comment 1•11 years ago
|
||
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.)
Flags: needinfo?(trev.saunders)
Reporter | ||
Updated•11 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8498183 -
Flags: review?(mshal)
Comment 4•11 years ago
|
||
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?
Attachment #8498183 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 5•11 years ago
|
||
(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.
Flags: needinfo?(tbsaunde+mozbugs)
Assignee | ||
Comment 6•11 years ago
|
||
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 ;)
Attachment #8498183 -
Flags: review?(dholbert)
Reporter | ||
Comment 7•11 years ago
|
||
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?
Reporter | ||
Comment 8•11 years ago
|
||
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?)
Attachment #8498183 -
Flags: review?(dholbert) → review+
Reporter | ||
Comment 9•11 years ago
|
||
(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.)
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
I squashed this into the relanding of bug 982842
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Assignee: nobody → tbsaunde+mozbugs
You need to log in
before you can comment on or make changes to this bug.
Description
•