Closed Bug 673405 Opened 9 years ago Closed 8 years ago

Rename GetDocAccessible() to Document()

Categories

(Core :: Disability Access APIs, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: hub)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [bk1])

Attachments

(1 file, 10 obsolete files)

34.38 KB, patch
surkov
: review+
Details | Diff | Splinter Review
similar to bug 673403
1) rename GetDocAccessible to Document()
2) add null checks

the case when there's no document accessible happens when accessible methods relying on non null document are called when accessible was moving to inaccessible DOM document (before we finished processing, i.e. marking the accessible as defunct). To reproduce this all you need is to move accessible DOM node to inaccessible document, listen for hide event and call the method that relies on not null document accessible (that could be GetName).
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #572003 - Flags: review?(eitan)
Attached patch Proposed patch (obsolete) — Splinter Review
Previous patch had a bug.
Attachment #572003 - Attachment is obsolete: true
Attachment #572003 - Flags: review?(eitan)
Attachment #572005 - Flags: review?(eitan)
Hub, please set up your mercurial to produce patches complying general rules https://developer.mozilla.org/en/Installing_Mercurial
Comment on attachment 572005 [details] [diff] [review]
Proposed patch

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

This patch looks like it fails to consistently check for a null value before doing something unsafe. Also, unless I am misreading a C++ism, it looks like RelatedAccIterator is called a lot on an un-null-checked document pointer. Making RelatedAccIterator worok with a null document would remedy this across the board.

::: accessible/src/base/nsAccessible.cpp
@@ +310,4 @@
>      bool isXUL = mContent->IsXUL();
>      if (isXUL) {
>        // Try XUL <description control="[id]">description text</description>
> +      XULDescriptionIterator iter(Document(), mContent);

Are you sure this call is null-safe? It looks like it constructs a RelatedAccIterator(), and it looks like that constructor is not null-safe for the document argument.

@@ +356,4 @@
>      if (mContent->IsHTML()) {
>        // Unless it is labeled via an ancestor <label>, in which case that would
>        // be redundant.
> +      HTMLLabelIterator iter(Document(), this,

Same as above, RelatedAccIterator does not look null-safe.

@@ +360,5 @@
>                               HTMLLabelIterator::eSkipAncestorLabel);
>        label = iter.Next();
>  
>      } else if (mContent->IsXUL()) {
> +      XULLabelIterator iter(Document(), mContent);

Same

@@ +1120,4 @@
>    nsAutoString label;
>  
>    nsAccessible* labelAcc = nsnull;
> +  HTMLLabelIterator iter(Document(), this);

Same

@@ +1182,4 @@
>      label.Truncate();
>  
>      nsAccessible* labelAcc = nsnull;
> +    XULLabelIterator iter(Document(), mContent);

Same

@@ +1995,4 @@
>    // defined on.
>    switch (aType) {
>      case nsIAccessibleRelation::RELATION_LABEL_FOR: {
> +      Relation rel(new RelatedAccIterator(Document(), mContent,

Same

@@ +2008,4 @@
>        Relation rel(new IDRefsIterator(mContent,
>                                        nsGkAtoms::aria_labelledby));
>        if (mContent->IsHTML()) {
> +        rel.AppendIter(new HTMLLabelIterator(Document(), this));

Ditto

@@ +2012,2 @@
>        } else if (mContent->IsXUL()) {
> +        rel.AppendIter(new XULLabelIterator(Document(), mContent));

Ditto

@@ +2019,4 @@
>        Relation rel(new IDRefsIterator(mContent,
>                                          nsGkAtoms::aria_describedby));
>        if (mContent->IsXUL())
> +        rel.AppendIter(new XULDescriptionIterator(Document(), mContent));

More

@@ +2023,5 @@
>  
>        return rel;
>      }
>      case nsIAccessibleRelation::RELATION_DESCRIPTION_FOR: {
> +      Relation rel(new RelatedAccIterator(Document(), mContent,

More

@@ +2038,4 @@
>        return rel;
>      }
>      case nsIAccessibleRelation::RELATION_NODE_CHILD_OF: {
> +      Relation rel(new RelatedAccIterator(Document(), mContent,

Ditto

@@ +2072,4 @@
>        return rel;
>      }
>      case nsIAccessibleRelation::RELATION_CONTROLLED_BY:
> +      return Relation(new RelatedAccIterator(Document(), mContent,

Likewise

@@ +2076,5 @@
>                                               nsGkAtoms::aria_controls));
>      case nsIAccessibleRelation::RELATION_CONTROLLER_FOR: {
>        Relation rel(new IDRefsIterator(mContent,
>                                        nsGkAtoms::aria_controls));
> +      rel.AppendIter(new HTMLOutputIterator(Document(), mContent));

More

@@ +2083,5 @@
>      case nsIAccessibleRelation::RELATION_FLOWS_TO:
>        return Relation(new IDRefsIterator(mContent,
>                                           nsGkAtoms::aria_flowto));
>      case nsIAccessibleRelation::RELATION_FLOWS_FROM:
> +      return Relation(new RelatedAccIterator(Document(), mContent,

Ditto

::: accessible/src/base/nsRootAccessible.cpp
@@ +389,1 @@
>    NS_ASSERTION(targetDocument, "No document while accessible is in document?!");

Maybe we will need something stronger than an assertion here since actual release builds will ignore this (right?) and crash later when targetDocument is dereferenced in line 553.

::: accessible/src/html/nsHTMLImageMapAccessible.cpp
@@ +115,4 @@
>    if (!mapAreas)
>      return;
>  
> +  nsDocAccessible* document = Document();

It is unsafely accessed in line 133:
    if (!document->BindToDocument(area, nsAccUtils::GetRoleMapEntry(areaContent)) ||
        !AppendChild(area)) {
      return;
    }

::: accessible/src/html/nsHTMLTextAccessible.cpp
@@ +241,4 @@
>    nsBlockFrame* blockFrame = do_QueryFrame(GetFrame());
>    if (blockFrame && blockFrame->HasBullet()) {
>      mBullet = new nsHTMLListBulletAccessible(mContent, mWeakShell);
> +    if (!Document()->BindToDocument(mBullet, nsnull))

Doesn't look safe.

@@ +298,3 @@
>    if (aHasBullet) {
>      mBullet = new nsHTMLListBulletAccessible(mContent, mWeakShell);
>      if (document->BindToDocument(mBullet, nsnull)) {

Need a null check before this.

::: accessible/src/xul/nsXULMenuAccessible.cpp
@@ +797,4 @@
>  nsAccessible*
>  nsXULMenupopupAccessible::ContainerWidget() const
>  {
> +  nsDocAccessible* document = Document();

This isn't null checked later, in line 805
Attachment #572005 - Flags: review?(eitan) → review-
Attached patch Proposed patch (obsolete) — Splinter Review
Updated patch.
Attachment #572005 - Attachment is obsolete: true
Attachment #572982 - Flags: review?(surkov.alexander)
Comment on attachment 572982 [details] [diff] [review]
Proposed patch

will be updating the patch soon as trevor found something
Attachment #572982 - Flags: review?(surkov.alexander)
Attached patch Proposed patch (obsolete) — Splinter Review
This is the third iteration, including Trevor's comment.
Attachment #572982 - Attachment is obsolete: true
Attachment #573051 - Flags: review?(surkov.alexander)
Whiteboard: [bk1]
Technically no document means the accessible was shutdown (that could be broken when presshell goes away before we shutdown the accessible and when document accessible loose its content node). So basically all relation stuffs and etc don't need a check for document accessible if we store document accessible instead a weak reference on presshell.

Nits: please use className* variableName (not className *variableName) or don't use { } around single if statement.
(In reply to alexander surkov from comment #8)

> Nits: please use className* variableName (not className *variableName) or
> don't use { } around single if statement.

I know the braces debate is very personal but I'm of the opinion that they should be mandatory. Also in other places in the same source files this was done so I thought it was fine.
Attached patch Proposed patch (obsolete) — Splinter Review
Latest version fixing the pointer notation.
Attachment #573051 - Attachment is obsolete: true
Attachment #573051 - Flags: review?(surkov.alexander)
Attachment #573339 - Flags: review?(surkov.alexander)
(In reply to alexander surkov from comment #8)
> Technically no document means the accessible was shutdown (that could be
> broken when presshell goes away before we shutdown the accessible and when
> document accessible loose its content node). So basically all relation
> stuffs and etc don't need a check for document accessible if we store
> document accessible instead a weak reference on presshell.

Is there a way for us to safely bail out on this condition?
(In reply to Hub Figuiere [:hub] from comment #11)
> (In reply to alexander surkov from comment #8)
> > Technically no document means the accessible was shutdown (that could be
> > broken when presshell goes away before we shutdown the accessible and when
> > document accessible loose its content node). So basically all relation
> > stuffs and etc don't need a check for document accessible if we store
> > document accessible instead a weak reference on presshell.
> 
> Is there a way for us to safely bail out on this condition?

I'd suggest to fix bug 672504 prior this one. That allows you to get rid many null document checks you introduce by this patch. Would you willing to take it?
> I'd suggest to fix bug 672504 prior this one. That allows you to get rid
> many null document checks you introduce by this patch. Would you willing to
> take it?

Make sense to me.
Depends on: 672504
Funny I just hit that one when debugging the Mac build. I don't know exactly how though.
(In reply to Hub Figuiere [:hub] from comment #14)
> Funny I just hit that one when debugging the Mac build. I don't know exactly
> how though.

right, that's why crashes is our priority. Are you working on this one (bug 672504) or to say it differently, is it on your plate?
Severity: normal → critical
(In reply to alexander surkov from comment #15)
 
> right, that's why crashes is our priority. Are you working on this one (bug
> 672504) or to say it differently, is it on your plate?

I have been a bit sidetracked but I don't mind taking bug 672504.
Assignee: nobody → hub
Attachment #573339 - Flags: review?(surkov.alexander)
(In reply to Hub Figuiere [:hub] from comment #16)
> (In reply to alexander surkov from comment #15)
>  
> > right, that's why crashes is our priority. Are you working on this one (bug
> > 672504) or to say it differently, is it on your plate?
> 
> I have been a bit sidetracked but I don't mind taking bug 672504.

then assigning these bugs to you.
Just updated to match changes in the codebase.
Attachment #573339 - Attachment is obsolete: true
Attachment #587456 - Attachment is obsolete: true
FYI, That last patch still breaks on a windows build. I'll update it soon.
Attachment #589029 - Attachment is obsolete: true
Updated patch. Apply on top of patch for bug 672504.
Attachment #590959 - Attachment is obsolete: true
Attachment #591647 - Attachment is obsolete: true
Attachment #592959 - Flags: review?(surkov.alexander)
Comment on attachment 592959 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=

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

basically this bug is fixed by bug 672504 because alive accessible cannot have null document anymore. So here all you should do is just rename the method.
Attachment #592959 - Flags: review?(surkov.alexander)
Summary: don't rely on non empty result from nsAccessNode::GetDocAccessible() → Rename GetDocAccessible() to Document()
Target Milestone: --- → mozilla12
Target Milestone: mozilla12 → ---
Attachment #592959 - Attachment is obsolete: true
Attachment #593194 - Flags: review?(surkov.alexander)
Comment on attachment 593194 [details] [diff] [review]
Rename GetDocAccessible() to Document().

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

thanks! r=me
Attachment #593194 - Flags: review?(surkov.alexander) → review+
I'll check it in when I have bug 672504 checked in.
Backed out of inbound for mochitest-other leaks:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0b205d34aefc
{
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 9024 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 282 instances of nsWeakReference with size 32 bytes each (9024 bytes total)
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7875d6ee51
Target Milestone: mozilla13 → ---
https://hg.mozilla.org/mozilla-central/rev/f72a9d8fd21d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.