673405 - Rename GetDocAccessible() to Document()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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).

Comment 1

[Hubert Figuiere [:hub]]

Attachment: Proposed patch

Comment 2

[Hubert Figuiere [:hub]]

Attachment: Proposed patch

Previous patch had a bug.

Comment 3

[alexander :surkov (:asurkov)]

Hub, please set up your mercurial to produce patches complying general rules https://developer.mozilla.org/en/Installing_Mercurial

Comment 4

[Eitan Isaacson [:eeejay]]

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

Comment 5

[Hubert Figuiere [:hub]]

Attachment: Proposed patch

Updated patch.

Comment 6

[Hubert Figuiere [:hub]]

Comment on attachment 572982 [details] [diff] [review]
Proposed patch

will be updating the patch soon as trevor found something

Comment 7

[Hubert Figuiere [:hub]]

Attachment: Proposed patch

This is the third iteration, including Trevor's comment.

Comment 8

[alexander :surkov (:asurkov)]

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.

Comment 9

[Hubert Figuiere [:hub]]

(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.

Comment 10

[Hubert Figuiere [:hub]]

Attachment: Proposed patch

Latest version fixing the pointer notation.

Comment 11

[Hubert Figuiere [:hub]]

(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?

Comment 12

[alexander :surkov (:asurkov)]

(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?

Comment 13

[Hubert Figuiere [:hub]]

> 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.

Comment 14

[Hubert Figuiere [:hub]]

Funny I just hit that one when debugging the Mac build. I don't know exactly how though.

Comment 15

[alexander :surkov (:asurkov)]

(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?

Comment 16

[Hubert Figuiere [:hub]]

(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.

Comment 17

[alexander :surkov (:asurkov)]

(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.

Comment 18

[Hubert Figuiere [:hub]]

Attachment: Ensure the document accessible isn't null. Rename GetDocAccessible() to Document().

Comment 19

[Hubert Figuiere [:hub]]

Just updated to match changes in the codebase.

Comment 20

[Hubert Figuiere [:hub]]

Attachment: Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=

Comment 21

[Hubert Figuiere [:hub]]

FYI, That last patch still breaks on a windows build. I'll update it soon.

Comment 22

[Hubert Figuiere [:hub]]

Attachment: Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=

Comment 23

[Hubert Figuiere [:hub]]

Updated patch. Apply on top of patch for bug 672504.

Comment 24

[Hubert Figuiere [:hub]]

Attachment: Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=

Comment 25

[Hubert Figuiere [:hub]]

Attachment: Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=

Comment 26

[alexander :surkov (:asurkov)]

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.

Comment 27

[Hubert Figuiere [:hub]]

Attachment: Rename GetDocAccessible() to Document().

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 593194 [details] [diff] [review]
Rename GetDocAccessible() to Document().

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

thanks! r=me

Comment 29

[Hubert Figuiere [:hub]]

I'll check it in when I have bug 672504 checked in.

Comment 30

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b205d34aefc

Comment 31

[Ed Morley [:emorley]]

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

Comment 32

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f72a9d8fd21d

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f72a9d8fd21d