Last Comment Bug 673405 - Rename GetDocAccessible() to Document()
: Rename GetDocAccessible() to Document()
Status: RESOLVED FIXED
[bk1]
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla13
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on: 672504
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2011-07-22 07:31 PDT by alexander :surkov
Modified: 2012-02-10 05:07 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (18.58 KB, patch)
2011-11-04 09:27 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Proposed patch (18.59 KB, patch)
2011-11-04 09:30 PDT, Hubert Figuiere [:hub]
eitan: review-
Details | Diff | Splinter Review
Proposed patch (36.44 KB, patch)
2011-11-08 13:16 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Proposed patch (36.29 KB, patch)
2011-11-08 17:26 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Proposed patch (36.52 KB, patch)
2011-11-09 14:52 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). (37.44 KB, patch)
2012-01-10 13:36 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r= (39.37 KB, patch)
2012-01-16 14:41 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r= (44.38 KB, patch)
2012-01-23 17:59 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r= (45.27 KB, patch)
2012-01-25 16:06 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r= (39.12 KB, patch)
2012-01-30 19:03 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Rename GetDocAccessible() to Document(). (34.38 KB, patch)
2012-01-31 12:48 PST, Hubert Figuiere [:hub]
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-07-22 07:31:31 PDT
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] 2011-11-04 09:27:15 PDT
Created attachment 572003 [details] [diff] [review]
Proposed patch
Comment 2 Hubert Figuiere [:hub] 2011-11-04 09:30:39 PDT
Created attachment 572005 [details] [diff] [review]
Proposed patch

Previous patch had a bug.
Comment 3 alexander :surkov 2011-11-04 10:07:46 PDT
Hub, please set up your mercurial to produce patches complying general rules https://developer.mozilla.org/en/Installing_Mercurial
Comment 4 Eitan Isaacson [:eeejay] 2011-11-05 10:49:36 PDT
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] 2011-11-08 13:16:42 PST
Created attachment 572982 [details] [diff] [review]
Proposed patch

Updated patch.
Comment 6 Hubert Figuiere [:hub] 2011-11-08 15:57:47 PST
Comment on attachment 572982 [details] [diff] [review]
Proposed patch

will be updating the patch soon as trevor found something
Comment 7 Hubert Figuiere [:hub] 2011-11-08 17:26:55 PST
Created attachment 573051 [details] [diff] [review]
Proposed patch

This is the third iteration, including Trevor's comment.
Comment 8 alexander :surkov 2011-11-09 02:34:26 PST
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] 2011-11-09 13:36:57 PST
(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] 2011-11-09 14:52:16 PST
Created attachment 573339 [details] [diff] [review]
Proposed patch

Latest version fixing the pointer notation.
Comment 11 Hubert Figuiere [:hub] 2011-11-09 14:57:20 PST
(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 2011-11-10 00:22:09 PST
(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] 2011-11-10 11:11:08 PST
> 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] 2011-11-15 12:30:43 PST
Funny I just hit that one when debugging the Mac build. I don't know exactly how though.
Comment 15 alexander :surkov 2011-11-15 21:28:03 PST
(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] 2011-11-16 14:50:05 PST
(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 2011-11-16 20:08:37 PST
(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] 2012-01-10 13:36:40 PST
Created attachment 587456 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document().
Comment 19 Hubert Figuiere [:hub] 2012-01-10 13:37:41 PST
Just updated to match changes in the codebase.
Comment 20 Hubert Figuiere [:hub] 2012-01-16 14:41:57 PST
Created attachment 589029 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
Comment 21 Hubert Figuiere [:hub] 2012-01-17 11:23:11 PST
FYI, That last patch still breaks on a windows build. I'll update it soon.
Comment 22 Hubert Figuiere [:hub] 2012-01-23 17:59:12 PST
Created attachment 590959 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
Comment 23 Hubert Figuiere [:hub] 2012-01-23 18:00:03 PST
Updated patch. Apply on top of patch for bug 672504.
Comment 24 Hubert Figuiere [:hub] 2012-01-25 16:06:47 PST
Created attachment 591647 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
Comment 25 Hubert Figuiere [:hub] 2012-01-30 19:03:20 PST
Created attachment 592959 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
Comment 26 alexander :surkov 2012-01-30 20:55:06 PST
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] 2012-01-31 12:48:43 PST
Created attachment 593194 [details] [diff] [review]
Rename GetDocAccessible() to Document().
Comment 28 alexander :surkov 2012-01-31 17:20:29 PST
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] 2012-02-02 16:09:33 PST
I'll check it in when I have bug 672504 checked in.
Comment 31 Ed Morley [:emorley] 2012-02-07 17:26:39 PST
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 33 Ed Morley [:emorley] 2012-02-10 05:07:04 PST
https://hg.mozilla.org/mozilla-central/rev/f72a9d8fd21d

Note You need to log in before you can comment on or make changes to this bug.