737724 - make IsDefunct() inline

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

1. add a flag to mFlags in nsAccessible to tell if an accessible is defunct or not.
2. initialize the bit to alive in the constructor and make it defunct in Shutdown()
3. reimplement IsDefunct() as an inline function on nsAccessible that just checks the flag.

Alex sounds good?

Comment 1

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> 1. add a flag to mFlags in nsAccessible to tell if an accessible is defunct
> or not.
> 2. initialize the bit to alive in the constructor and make it defunct in
> Shutdown()

sounds good but it miss cycle collection thing which can make an accessible defunct but never calls Shutdown

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> (In reply to Trevor Saunders (:tbsaunde) from comment #0)
> > 1. add a flag to mFlags in nsAccessible to tell if an accessible is defunct
> > or not.
> > 2. initialize the bit to alive in the constructor and make it defunct in
> > Shutdown()
> 
> sounds good but it miss cycle collection thing which can make an accessible
> defunct but never calls Shutdown

well, but then it calls LastRelease() which calls Shutdown() right, so if Shutdown() stays virtual this should work.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> well, but then it calls LastRelease() which calls Shutdown() right, so if
> Shutdown() stays virtual this should work.

ok, sounds right

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v1)

I thought this would be easy ... compiles, but every single mochitest-a11y test fails miserably ... reviewing from this end ...

Comment 5

[alexander :surkov (:asurkov)]

there's a ton of IsDefunct implementations through a11y code

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

Well DUH... this one works gooder

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 608402 [details] [diff] [review]
Patch (v2)

>      printf("\n");
>    }
> #endif
>+  mFlags &= ~eDefunctAccessible;

|=

> nsAccessible::~nsAccessible()
> {
>+  mFlags |= eDefunctAccessible;

don't bother there's no point.

>+    eTextLeafAccessible = 1 << 16,
>+    eDefunctAccessible = 1 << 17

I'm not sure I  like putting this in the same enum as the accessible type stuff Alex, thoughts?

also, you still haven't removed all of the virtual implementations of IsDefunct()

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> > nsAccessible::~nsAccessible()
> > {
> >+  mFlags |= eDefunctAccessible;
> 
> don't bother there's no point.

right. Do that on Shutdown

> >+    eTextLeafAccessible = 1 << 16,
> >+    eDefunctAccessible = 1 << 17
> 
> I'm not sure I  like putting this in the same enum as the accessible type
> stuff Alex, thoughts?

right, that's not a type. I think we need something like StateFlags with constants placed between ChildrenFlags and AccessibleTypes. It'd be really nice if we can find a way to not adjust constants manually when some flag set is changed.

Comment 9

[Mark Capella [:capella]]

Made all changes, then I found / removed "virtual bool IsDefunct() const;" definitions from (5) files:

nsXULTreeGridAccessible.h, nsXULTreeAccessible.h, nsApplicationAccessible.h, nsAccessNode.h, and nsDocAccessible.h

This is causing compiler errors ... and is where my knowledge of classes starts to get thin ...

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v3)

See the patch (v3) and the below:

c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(218) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(265) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(296) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(338) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(373) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(445) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(458) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(471) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(484) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(497) : error C3861: 'IsDefunct': identifier not found
c:/Users/Master/mozilla-central/accessible/src/msaa/nsAccessNodeWrap.cpp(514) : error C3861: 'IsDefunct': identifier not found

Comment 11

[alexander :surkov (:asurkov)]

because you define IsDefunct on nsAccessible while you should do that on nsAccessNode. The fix getting tricky because mFlags is defined on nsAccessible

Comment 12

[alexander :surkov (:asurkov)]

alternatively you could do mContent check instead of IsDefunct on nsAccessNodeWrap. But let's get Trevor check if it's ok.

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 608589 [details] [diff] [review]
Patch (v3)

>-  virtual bool IsDefunct() const;

you might want to try removing the definitions as well as the declarations...

>   /**
>+   * Flags used to describe whether the accessible is in defunct state.
>+   */
>+  enum StateFlags {
>+    eIsDefunct = 1 << 17 // accessible is Defunct
>+  };

please file a folow up to cleanup the way these enums work.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 608589 [details] [diff] [review]
> Patch (v3)
> 
> >-  virtual bool IsDefunct() const;
> 
> you might want to try removing the definitions as well as the declarations...
> 
> >   /**
> >+   * Flags used to describe whether the accessible is in defunct state.
> >+   */
> >+  enum StateFlags {
> >+    eIsDefunct = 1 << 17 // accessible is Defunct

do 1 << 2 (place it "after" ChildrenFlags), otherwise for any new Accessible Type you need to shift this one, that happens often enough

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v4)

See Patch (v4)

I tried removing the definitions as well as the declarations (comment 13) ... and changed the eIsDefunct = 1 << 17 to be: 1 << 2 (placed it "after" ChildrenFlags) as in (comment 14)

Build still fails as in (comment 10) ... re-reading Alexs (Comments 11 and 12) ...

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> alternatively you could do mContent check instead of IsDefunct on
> nsAccessNodeWrap. But let's get Trevor check if it's ok.

yeah, I think for ISimpleDOMNode stuff like that checking mContent probably makes more sense anyway.

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #15)
> Created attachment 608608 [details] [diff] [review]
> Patch (v4)
> 
> See Patch (v4)
> 
> I tried removing the definitions as well as the declarations (comment 13)
> ... and changed the eIsDefunct = 1 << 17 to be: 1 << 2 (placed it "after"
> ChildrenFlags) as in (comment 14)
> 
> Build still fails as in (comment 10) ... re-reading Alexs (Comments 11 and
> 12) ...

yeah, you need to fix the issue of calling IsDefunct() in nsAccessNode stuff, it looks like I forgot about a couple cases in nsAccessNode too, for those cases it looks as if since they're accessor type things we should only call them on non-defunct accessibles and they're callers will need to make sure that's true, either by checking mContent or IsDefunt()

Comment 18

[Mark Capella [:capella]]

Attachment: Patch (v5)

Ok, I'm posting the patch as I have it so far ... this one may be exceeding my c++ classes capabilities without a larger degree of hand holding ... I'm a little hampered by the fact that this is a technical / performance change making it harder for me to follow, as I can't tell when I've "fixed" something or am even getting close :)

Comment 19

[Mark Capella [:capella]]

Attachment: Patch (v6)

Finally got a clean build - all mochitest --a11y  pass ok ...

Comment 20

[Trevor Saunders (:tbsaunde)]

Comment on attachment 608961 [details] [diff] [review]
Patch (v6)

generally looks fine, I'll bet bet Alex has a clear mind for looking at details :)

Comment 21

[Mark Capella [:capella]]

Comment on attachment 608961 [details] [diff] [review]
Patch (v6)

r=surkov

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 608961 [details] [diff] [review]
Patch (v6)

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

::: accessible/src/base/nsAccessNode.cpp
@@ +216,1 @@
>      return nsnull;

see comment below

@@ +260,1 @@
>      return;

currently we don't support document accessible without mContent but we want do this one day. Maybe you could use mDoc as a check instead.

::: accessible/src/base/nsAccessible.cpp
@@ +2522,2 @@
>    // Invalidate the child count and pointers to other accessibles, also make
>    // sure none of its children point to this parent

group comments into single comment

@@ +2522,3 @@
>    // Invalidate the child count and pointers to other accessibles, also make
>    // sure none of its children point to this parent
> +  mFlags |= eIsDefunct;

new line pls

::: accessible/src/base/nsAccessible.h
@@ +636,5 @@
>     */
>    static void TranslateString(const nsAString& aKey, nsAString& aStringOut);
>  
> +  /**
> +   * Return true/false if the accessible is defunct.

Return true if

@@ +638,5 @@
>  
> +  /**
> +   * Return true/false if the accessible is defunct.
> +   */
> +  inline bool IsDefunct() const { return mFlags & eIsDefunct; }

you don't need inline keyword

@@ +684,5 @@
>    inline void SetChildrenFlag(ChildrenFlags aFlag)
>      { mFlags = (mFlags & ~kChildrenFlagsMask) | aFlag; }
>  
>    /**
> +   * Flags used to describe whether the accessible is in defunct state.

maybe: State flags of the accessible?
@note keep these flags in sync with ChildrenFlags

@@ +687,5 @@
>    /**
> +   * Flags used to describe whether the accessible is in defunct state.
> +   */
> +  enum StateFlags {
> +    eIsDefunct = 1 << 2 // accessible is Defunct

if you want to keep this comment then please lowercase 'Defunct'

@@ +695,2 @@
>     * Flags describing the accessible itself.
>     * @note keep these flags in sync with ChildrenFlags

and StateFlags

::: accessible/src/base/nsApplicationAccessible.cpp
@@ -312,5 @@
>  bool
> -nsApplicationAccessible::IsDefunct() const
> -{
> -  return nsAccessibilityService::IsShutdown();
> -}

thinking aloud: that's equivalent to IsShutdown check since when accessibility gets shutdown via nsAccessibilityService then it destroys application accessible (and thus appacc shutdown is called)

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +219,3 @@
>      return E_FAIL;
>  
>    nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(GetNode()));

it sounds like you need to use result of GetNode() as defunct check

@@ +446,3 @@
>      return E_FAIL;
>  
>    *aNode = MakeAccessNode(GetNode()->GetNodeParent());

same here

@@ +459,1 @@
>      return E_FAIL;

same here

@@ +472,1 @@
>      return E_FAIL;

same

@@ +485,1 @@
>      return E_FAIL;

same

@@ +498,1 @@
>      return E_FAIL;

same

@@ +515,1 @@
>      return E_FAIL;

same

::: accessible/src/xul/nsXULTreeAccessible.cpp
@@ -171,5 @@
>  
> -bool
> -nsXULTreeAccessible::IsDefunct() const
> -{
> -  return nsAccessibleWrap::IsDefunct() || !mTree || !mTreeView;

that's not equivalent, mTreeView can go away without accessible Shutdown(), thus this change should result in crashes
the same for other IsDefunct impls.

Comment 23

[Trevor Saunders (:tbsaunde)]

> @@ +638,5 @@
> >  
> > +  /**
> > +   * Return true/false if the accessible is defunct.
> > +   */
> > +  inline bool IsDefunct() const { return mFlags & eIsDefunct; }
> 
> you don't need inline keyword

I thought you usually prefered it?  (I don't care either way, but we should be consistant)

> > -nsApplicationAccessible::IsDefunct() const
> > -{
> > -  return nsAccessibilityService::IsShutdown();
> > -}
> 
> thinking aloud: that's equivalent to IsShutdown check since when
> accessibility gets shutdown via nsAccessibilityService then it destroys
> application accessible (and thus appacc shutdown is called)

yeah, that makes sense, tehre is actually a little bit of a problem here in that nsApplicationAccessible doesn't call its parent class shutdown.  I'm sort of tempted to change that, but haven't thought it through yet.

> > -nsXULTreeAccessible::IsDefunct() const
> > -{
> > -  return nsAccessibleWrap::IsDefunct() || !mTree || !mTreeView;
> 
> that's not equivalent, mTreeView can go away without accessible Shutdown(),
> thus this change should result in crashes
> the same for other IsDefunct impls.

which others in particular, and where does it go away other thn in a Shutdown()? I can't find a spot off hand.

Comment 24

[alexander :surkov (:asurkov)]

> > that's not equivalent, mTreeView can go away without accessible Shutdown(),
> > thus this change should result in crashes
> > the same for other IsDefunct impls.
> 
> which others in particular, and where does it go away other thn in a
> Shutdown()? I can't find a spot off hand.

I meant all XUL tree related

Comment 25

[Mark Capella [:capella]]

Attachment: Patch (v7)

This is the most current version of the patch with issues addressed as I understand them.

I'm waiting for more information regarding what is outstanding / under discussion in previous comment#24 ...

Comment 26

[Trevor Saunders (:tbsaunde)]

Comment on attachment 609607 [details] [diff] [review]
Patch (v7)

> nsPresContext* nsAccessNode::GetPresContext()
> {
>-  if (IsDefunct())
>+  if (!mContent)
>     return nsnull;

I'd probably remove these checks from here to be consistant with other getters of properties and make sure that this object isn't defunct or has an mContent in the caller as makes sense.

alternatively you could use mDoc here since that's what we actually use as I believe Alex suggested.

> nsAccessNode::ScrollTo(PRUint32 aScrollType)
> {
>-  if (IsDefunct())
>+  if (!mContent)
>     return;

same

> nsAccessNode::Language(nsAString& aLanguage)
> {
>   aLanguage.Truncate();
> 
>-  if (IsDefunct())
>+  if (!mContent)

same

>-  // Invalidate the child count and pointers to other accessibles, also make
>-  // sure none of its children point to this parent
>+  // Mark the accessible as defunct, invalidate the child, remove all children

not as descriptive as the old comment.

>   /**
>-   * Flags describing the accessible itself.
>+   * Flags used to describe state flags of the accessible

how about flags used to describe the state of the accessible?

>+   * Flags describing the accessible itself.

I believe Alex wanted something else here?

>-  if (IsDefunct())
>+  if (!GetNode())
>     return E_FAIL;

use a local variable and save moultiple calls to GetNode?

> STDMETHODIMP nsAccessNodeWrap::get_attributesForNames( 
>     /* [in] */ unsigned short aNumAttribs,
>     /* [length_is][size_is][in] */ BSTR __RPC_FAR *aAttribNames,
>     /* [length_is][size_is][in] */ short __RPC_FAR *aNameSpaceID,
>     /* [length_is][size_is][retval] */ BSTR __RPC_FAR *aAttribValues)
> {
> __try {
>-  if (IsDefunct() || !IsElement())
>+  if (!mContent || !IsElement())

kind of crazy to check mContent and then that GetNode() returns an element.

>-  if (IsDefunct() || IsDocumentNode())
>+  if (!mContent || IsDocumentNode())
>     return E_FAIL;

same

>-  if (IsDefunct() || IsDocumentNode())
>+  if (!mContent || IsDocumentNode())
>     return E_FAIL;

same

>  
>   nsCOMPtr<nsIDOMCSSStyleDeclaration> cssDecl =
>     nsWinUtils::GetComputedStyleDeclaration(mContent);
>   NS_ENSURE_TRUE(cssDecl, E_FAIL);
> 
>   PRUint32 index;
>   for (index = 0; index < aNumStyleProperties; index ++) {
>@@ -437,86 +437,86 @@ nsAccessNodeWrap::MakeAccessNode(nsINode
> 
>   return iNode;
> }
> 
> 
> STDMETHODIMP nsAccessNodeWrap::get_parentNode(ISimpleDOMNode __RPC_FAR *__RPC_FAR *aNode)
> {
> __try {
>-  if (IsDefunct())
>+  if (!GetNode())
>     return E_FAIL;
> 
>   *aNode = MakeAccessNode(GetNode()->GetNodeParent());

use a loca var?

> __try {
>-  if (IsDefunct())
>+  if (!GetNode())
>     return E_FAIL;

same

>-nsXULTreeAccessible::IsDefunct() const
>-{
>-  return nsAccessibleWrap::IsDefunct() || !mTree || !mTreeView;
>-}

plese add null checks per irc.

Comment 27

[Mark Capella [:capella]]

FYI per irc: "you should add null checks into every nsXULTreeAccessbile method that deals with mTreeView "

Comment 28

[Mark Capella [:capella]]

Attachment: Patch (v8)

I think this patch catches all issues up to but not yet including null checks ...

Comment 29

[Trevor Saunders (:tbsaunde)]

Comment on attachment 609694 [details] [diff] [review]
Patch (v8)

> nsAccessNode::ScrollTo(PRUint32 aScrollType)
> {
>-  if (IsDefunct())
>+  if (!mDoc)
>     return;

that's fine, but eventually we should stop checking this here to be consistant with other methods on accessibles which expect the platform layer ensures the accessible is in a good state.

we can either file a follow up for these or you can add IsDefunct checks in nsAccessible::ScrollTo and nsAccessibleWrap::ScrollTo and check mContent in nsAccessNodeWrap::scrollTo

> nsAccessNode::Language(nsAString& aLanguage)
> {
>   aLanguage.Truncate();
> 
>-  if (IsDefunct())
>+  if (!mDoc)
>     return;

same accept where you'd need to check changes a little check mContnet in nsAccessNodeWrap::get_Language and check IsDefunct in nsAccessibleWrap::get_Locale and nsAccessible::GetLanguage

>+    /**
>+     * It is used in nsAccessibleNodeWrap for performance.
>+     * implementation.
>+     */
>+    nsINode* gNode;

nah, I meant local variable to each function.  the little bit of ISimpleDOMNode perf this gets isn't worth the increase in size of every accessible.

otherwise looks good, thanks!

Comment 30

[Mark Capella [:capella]]

Attachment: Patch (v9)

This patch includes mTreeView null checks, and GetNode() local vars where appropriate to functions to avoid multiple calls.

I plan to file a followup bug "to be consistent with other methods on accessibles which expect the platform layer ensures the accessible is in a good state".

Comment 31

[Trevor Saunders (:tbsaunde)]

Comment on attachment 609881 [details] [diff] [review]
Patch (v9)

>+   * Flags used to describe state flags of the accessible.

just "Flags used to describe the state of this accessible."

>+   * Flags describing the accessible itself.

"flags describing the type of this accessible."

>@@ -257,17 +258,17 @@ STDMETHODIMP nsAccessNodeWrap::get_attri
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aAttribNames,
>     /* [length_is][size_is][out] */ short __RPC_FAR *aNameSpaceIDs,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aAttribValues,
>     /* [out] */ unsigned short __RPC_FAR *aNumAttribs)
> {
> __try{
>   *aNumAttribs = 0;
> 
>-  if (IsDefunct() || IsDocumentNode())
>+  if (!GetNode() || IsDocumentNode())

in this case it should be equivelent, but just use mContent.

>     return E_FAIL;
> 
>   PRUint32 numAttribs = mContent->GetAttrCount();
>   if (numAttribs > aMaxAttribs)
>     numAttribs = aMaxAttribs;
>   *aNumAttribs = static_cast<unsigned short>(numAttribs);
> 
>   for (PRUint32 index = 0; index < numAttribs; index++) {
>@@ -288,17 +289,17 @@ __try{
> 
> STDMETHODIMP nsAccessNodeWrap::get_attributesForNames( 
>     /* [in] */ unsigned short aNumAttribs,
>     /* [length_is][size_is][in] */ BSTR __RPC_FAR *aAttribNames,
>     /* [length_is][size_is][in] */ short __RPC_FAR *aNameSpaceID,
>     /* [length_is][size_is][retval] */ BSTR __RPC_FAR *aAttribValues)
> {
> __try {
>-  if (IsDefunct() || !IsElement())
>+  if (!GetNode() || !IsElement())

same

>     return E_FAIL;
> 
>   nsCOMPtr<nsIDOMElement> domElement(do_QueryInterface(mContent));
>   nsCOMPtr<nsINameSpaceManager> nameSpaceManager =
>     do_GetService(NS_NAMESPACEMANAGER_CONTRACTID);
> 
>   PRInt32 index;
> 
>@@ -330,17 +331,17 @@ STDMETHODIMP nsAccessNodeWrap::get_compu
>     /* [in] */ boolean aUseAlternateView,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleProperties,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues,
>     /* [out] */ unsigned short __RPC_FAR *aNumStyleProperties)
> {
> __try{
>   *aNumStyleProperties = 0;
> 
>-  if (IsDefunct() || IsDocumentNode())
>+  if (!GetNode() || IsDocumentNode())

same

> 
> STDMETHODIMP nsAccessNodeWrap::get_computedStyleForProperties( 
>     /* [in] */ unsigned short aNumStyleProperties,
>     /* [in] */ boolean aUseAlternateView,
>     /* [length_is][size_is][in] */ BSTR __RPC_FAR *aStyleProperties,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues)
> {
> __try {
>-  if (IsDefunct() || IsDocumentNode())
>+  if (!GetNode() || IsDocumentNode())

same

>-////////////////////////////////////////////////////////////////////////////////
>-// nsXULTreeItemAccessibleBase: nsAccessNode implementation

leave it, Shutdown() is part of nsAccessNode

other than those nits the idea is still good :) I hope surkov has a clear mind todya to look at details because I don't

Comment 32

[Mark Capella [:capella]]

Attachment: Patch (v10)

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > > -nsApplicationAccessible::IsDefunct() const
> > > -{
> > > -  return nsAccessibilityService::IsShutdown();
> > > -}
> > 
> > thinking aloud: that's equivalent to IsShutdown check since when
> > accessibility gets shutdown via nsAccessibilityService then it destroys
> > application accessible (and thus appacc shutdown is called)
> 
> yeah, that makes sense, tehre is actually a little bit of a problem here in
> that nsApplicationAccessible doesn't call its parent class shutdown.  I'm
> sort of tempted to change that, but haven't thought it through yet.

if there's anything to shutdown in the base class of application accessible

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 609990 [details] [diff] [review]
Patch (v10)

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

::: accessible/src/base/nsAccessNode.cpp
@@ +211,5 @@
>  
>  // nsAccessNode protected
>  nsPresContext* nsAccessNode::GetPresContext()
>  {
> +  if (!mDoc)

!mDoc is equivalent to !mContent (what you did in previous iterations) but I don't mind, all this code makes me think that one day we should remove nsAccessNode

::: accessible/src/base/nsAccessible.cpp
@@ +2517,5 @@
>  
>  void
>  nsAccessible::Shutdown()
>  {
> +  // Mark the accessible as defunct, Invalidate the child count and pointers to 

I -> i in 'Invalidate'

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +262,5 @@
>  {
>  __try{
>    *aNumAttribs = 0;
>  
> +  if (!mContent)

you missed IsDocumentNode() check. Document accessible has not null mContent.

@@ +293,5 @@
>      /* [length_is][size_is][in] */ short __RPC_FAR *aNameSpaceID,
>      /* [length_is][size_is][retval] */ BSTR __RPC_FAR *aAttribValues)
>  {
>  __try {
> +  if (!mContent)

similar issue

@@ +335,5 @@
>  {
>  __try{
>    *aNumStyleProperties = 0;
>  
> +  if (!mContent)

same

@@ +370,5 @@
>      /* [length_is][size_is][in] */ BSTR __RPC_FAR *aStyleProperties,
>      /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues)
>  {
>  __try {
> +  if (!mContent)

same

::: accessible/src/xul/nsXULTreeAccessible.cpp
@@ +477,5 @@
>  {
>    // tree's children count is row count + treecols count.
>    PRInt32 childCount = nsAccessible::GetChildCount();
>    if (childCount == -1)
>      return -1;

i prefer
if (childCount == -1 || !mTreeView)
  return childCount;

@@ +620,5 @@
>    // We dealt with removed tree items already however we may keep tree items
>    // having row indexes greater than row count. We should remove these dead tree
>    // items silently from caches.
> +  if (!mTreeView)
> +    return;

not equivalent to existing code, when no mTreeView you should assume that newRowCount = 0 and continue

I don't think this case is possible at all though. So maybe I'd go with no check here

@@ +654,5 @@
>  
>    nsresult rv;
>    if (endRow == -1) {
> +    if (!mTreeView)
> +      return;

that's not correct as well: we receive notification that rows were changed but null-mTreeView claims no rows at all. That means something goes wrong and therefore we need to make sure we don't have created accessibles for absent tree rows. Probably I'd go with no null check at all since I don't think this case is possible.

@@ +824,5 @@
>  
>  NS_IMETHODIMP
>  nsXULTreeItemAccessibleBase::SetSelected(bool aSelect)
>  {
> +  if (IsDefunct() || !mTreeView)

thinking aloud: all nsXULTreeItemAccessible checks gets redundant when bug 739524 is fixed

@@ +861,5 @@
>    if (aType != nsIAccessibleRelation::RELATION_NODE_CHILD_OF)
>      return Relation();
>  
> +  if (!mTreeView)
> +    return Relation();

I'd say to return early before any relation type checks

@@ +863,5 @@
>  
> +  if (!mTreeView)
> +    return Relation();
> +
> +  PRInt32 parentIndex;

while you are here pls initialize it by -1

@@ +895,5 @@
>    }
>  
>    if (aIndex == eAction_Expand && IsExpandable()) {
> +    if (!mTreeView)
> +      return NS_ERROR_FAILURE;

you added mTreeView check into IsExpandable already

@@ +1013,5 @@
>    // focusable and selectable states
>    PRUint64 state = states::FOCUSABLE | states::SELECTABLE;
>  
>    // expanded/collapsed state
> +  if (IsExpandable() && mTreeView) {

you should return defunct state early in the beginning of method like
if (!mTreeView)
  return states::DEFUNCT;

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ -1120,1 @@
>  

no any single mTreeView check in this file

Comment 35

[Mark Capella [:capella]]

Attachment: Patch (v11)

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 610048 [details] [diff] [review]
Patch (v11)

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

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +325,1 @@
>      return NS_ERROR_FAILURE;

technically no tree view means there's no rows and thus there's no selected rows and it's ok actually (no failure) but you just preserved the logic so it's up to you to fix these GetSelected methods or not.

@@ +547,5 @@
>  NS_IMETHODIMP
>  nsXULTreeGridAccessible::SelectRow(PRInt32 aRowIndex)
>  {
> +  if (!mTreeView)
> +    return NS_ERROR_FAILURE;

but here it means invalid_argument since no rows, again up to you

@@ +566,5 @@
>  NS_IMETHODIMP
>  nsXULTreeGridAccessible::UnselectRow(PRInt32 aRowIndex)
>  {
> +  if (!mTreeView)
> +    return NS_ERROR_FAILURE;

same here

Comment 37

[Mark Capella [:capella]]

Attachment: Patch (v12)

Comment 38

[alexander :surkov (:asurkov)]

Comment on attachment 610060 [details] [diff] [review]
Patch (v12)

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

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +111,1 @@
>      return NS_ERROR_FAILURE;

ok, since you decided to fix return values then let's run through.

!mTreeView means no rows, NS_OK

@@ +325,1 @@
>      return NS_ERROR_FAILURE;

!mTreeView - NS_OK

@@ +531,1 @@
>      return NS_ERROR_FAILURE;

!mTreeView - invalid arg since no row at the given index

Comment 39

[Mark Capella [:capella]]

Attachment: Patch (v13)

Comment 40

[alexander :surkov (:asurkov)]

Comment on attachment 610079 [details] [diff] [review]
Patch (v13)

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

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +107,5 @@
>    NS_ENSURE_ARG_POINTER(arowCount);
>    *arowCount = nsnull;
>  
> +  if (IsDefunct() || !mTreeView)
> +    return NS_OK;

sorry to say this but IsDefunct is still failure :)
I'll fix it before landing

@@ +321,5 @@
>    NS_ENSURE_ARG_POINTER(aRows);
>    *aRows = nsnull;
>  
> +  if (IsDefunct() || !mTreeView)
> +    return NS_OK;

same here

@@ +527,5 @@
>    NS_ENSURE_ARG_POINTER(aIsSelected);
>    *aIsSelected = false;
>  
> +  if (IsDefunct() || !mTreeView)
> +    return NS_ERROR_INVALID_ARG;

and here

Comment 41

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a78aae4fa27

Comment 42

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/7a78aae4fa27