The default bug view has changed. See this FAQ.

Rename GetDocAccessible() to Document()

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: hub)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla13
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [bk1])

Attachments

(1 attachment, 10 obsolete attachments)

34.38 KB, patch
surkov
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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).
(Assignee)

Comment 1

6 years ago
Created attachment 572003 [details] [diff] [review]
Proposed patch
Attachment #572003 - Flags: review?(eitan)
(Assignee)

Comment 2

6 years ago
Created attachment 572005 [details] [diff] [review]
Proposed patch

Previous patch had a bug.
Attachment #572003 - Attachment is obsolete: true
Attachment #572003 - Flags: review?(eitan)
Attachment #572005 - Flags: review?(eitan)
(Reporter)

Comment 3

6 years ago
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-
(Assignee)

Comment 5

6 years ago
Created attachment 572982 [details] [diff] [review]
Proposed patch

Updated patch.
Attachment #572005 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #572982 - Flags: review?(surkov.alexander)
(Assignee)

Comment 6

6 years ago
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)
(Assignee)

Comment 7

6 years ago
Created attachment 573051 [details] [diff] [review]
Proposed patch

This is the third iteration, including Trevor's comment.
Attachment #572982 - Attachment is obsolete: true
Attachment #573051 - Flags: review?(surkov.alexander)
(Assignee)

Updated

6 years ago
Whiteboard: [bk1]
(Reporter)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
Created attachment 573339 [details] [diff] [review]
Proposed patch

Latest version fixing the pointer notation.
Attachment #573051 - Attachment is obsolete: true
Attachment #573051 - Flags: review?(surkov.alexander)
Attachment #573339 - Flags: review?(surkov.alexander)
(Assignee)

Comment 11

6 years ago
(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?
(Reporter)

Comment 12

6 years ago
(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?
(Assignee)

Comment 13

6 years ago
> 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.
(Assignee)

Updated

6 years ago
Depends on: 672504
(Assignee)

Comment 14

5 years ago
Funny I just hit that one when debugging the Mac build. I don't know exactly how though.
(Reporter)

Comment 15

5 years ago
(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
(Assignee)

Comment 16

5 years ago
(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.
(Reporter)

Updated

5 years ago
Assignee: nobody → hub
(Reporter)

Updated

5 years ago
Attachment #573339 - Flags: review?(surkov.alexander)
(Reporter)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 587456 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document().
(Assignee)

Comment 19

5 years ago
Just updated to match changes in the codebase.
(Assignee)

Updated

5 years ago
Attachment #573339 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 589029 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
(Assignee)

Updated

5 years ago
Attachment #587456 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
FYI, That last patch still breaks on a windows build. I'll update it soon.
(Assignee)

Comment 22

5 years ago
Created attachment 590959 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
(Assignee)

Updated

5 years ago
Attachment #589029 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Updated patch. Apply on top of patch for bug 672504.
(Assignee)

Comment 24

5 years ago
Created attachment 591647 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
(Assignee)

Updated

5 years ago
Attachment #590959 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
Created attachment 592959 [details] [diff] [review]
Ensure the document accessible isn't null. Rename GetDocAccessible() to Document(). r=
(Assignee)

Updated

5 years ago
Attachment #591647 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #592959 - Flags: review?(surkov.alexander)
(Reporter)

Comment 26

5 years ago
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)
(Assignee)

Updated

5 years ago
Summary: don't rely on non empty result from nsAccessNode::GetDocAccessible() → Rename GetDocAccessible() to Document()
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 years ago
Target Milestone: mozilla12 → ---
(Assignee)

Comment 27

5 years ago
Created attachment 593194 [details] [diff] [review]
Rename GetDocAccessible() to Document().
(Assignee)

Updated

5 years ago
Attachment #592959 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #593194 - Flags: review?(surkov.alexander)
(Reporter)

Comment 28

5 years ago
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+
(Assignee)

Comment 29

5 years ago
I'll check it in when I have bug 672504 checked in.
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b205d34aefc
Target Milestone: --- → mozilla13
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 → ---
(Assignee)

Comment 32

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