249421 - Remove getDOMNode from nsIAccessible (it is already available in nsIAccessNode)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Philip K. Warren]

As one part of freezing the accessibility APIs, we need to remove the additional
getDOMNode from nsIAccessible. The DOM node is already accessible via the
nsIAccessNode interface, which nsAccessible implements.

Comment 1

[Philip K. Warren]

Attachment: Patch v1

I really don't know if it is necessary to check the result of QI'ing to
nsIAccessNode, since all nsAccessible objects implement that interface.

Comment 2

[Philip K. Warren]

Comment on attachment 152115 [details] [diff] [review]
Patch v1

Aaron - can you please test this patch on Windows? When I was testing last
night in Windows something about this patch was causing accexplore to crash,
but I don't know if it was just an error in my build or not.

Comment 3

[Aaron Leventhal]

(In reply to comment #2)
> (From update of attachment 152115 [details] [diff] [review])
> Aaron - can you please test this patch on Windows? When I was testing last
> night in Windows something about this patch was causing accexplore to crash,
> but I don't know if it was just an error in my build or not.

Did you rebuild mozilla/widget? Since the vtbl is changed all of the method
calls are off by one. 

Comment 4

[Aaron Leventhal]

Comment on attachment 152115 [details] [diff] [review]
Patch v1

I don't think a null check is necessary. At most an NS_ASSERTION.

I assume the crash is because of the vtbl thing I mentioned. Safest bet is to
rebuild of Mozilla and try it again.

Ask the sr= about whether we really need an NS_ASSERTION for each one.

Comment 5

[Philip K. Warren]

Rebuilding everything from scratch did indeed seem to fix the crash with AccExplore.

Comment 6

[Philip K. Warren]

Comment on attachment 152115 [details] [diff] [review]
Patch v1

Darin: Could you please look over this bug, and comment on the need for an
assertion after the QI, and also if I need to bump the UUID on the interface
when making these changes.

Comment 7

[Darin Fisher]

Comment on attachment 152115 [details] [diff] [review]
Patch v1

>Index: accessible/public/nsIAccessible.idl

All interface changes must be accompanied by changes to the interface's UUID.


>Index: accessible/src/atk/nsHTMLTableAccessibleWrap.cpp

>+  nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(aCaption));
>   nsCOMPtr<nsIDOMNode> domNode;
>+  rv = accessNode->GetDOMNode(getter_AddRefs(domNode));
>   NS_ENSURE_SUCCESS(rv, rv);

hmm... what if the first QI fails?  then accessNode will be null,
and the call to GetDOMNode will cause a crash, no?


>Index: accessible/src/atk/nsXULTreeAccessibleWrap.cpp

>+  nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(column));
>   nsCOMPtr<nsIDOMNode> columnNode;
>+  rv = accessNode->GetDOMNode(getter_AddRefs(columnNode));
>   NS_ENSURE_SUCCESS(rv, rv);

same goes for here.



>Index: accessible/src/base/nsRootAccessible.cpp

>+    nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(accessible));
>     if (accessible) {
>+      accessNode->GetDOMNode(getter_AddRefs(targetNode));

here too... do you want to write "if (accessNode) {" instead?


>Index: accessible/src/html/nsHTMLSelectAccessible.cpp

>+    nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(mParent));
>     nsCOMPtr<nsIDOMNode> selectNode;
>+    accessNode->GetDOMNode(getter_AddRefs(selectNode));

>+    nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(nextSiblingAcc));
>+    accessNode->GetDOMNode(getter_AddRefs(siblingDOMNode));

>+    nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(mParent));
>+    accessNode->GetDOMNode(getter_AddRefs(selectNode));

Ditto.	Trusting that QI never fails seems risky to me.


>Index: accessible/src/msaa/nsAccessibleWrap.cpp

>+      nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(xpAccessible));
>       nsCOMPtr<nsIDOMNode> domNode;
>+      accessNode->GetDOMNode(getter_AddRefs(domNode));


>Index: accessible/src/xul/nsXULMenuAccessible.cpp

>+    nsCOMPtr<nsIAccessNode> accessNode(do_QueryInterface(parentAccessible));
>+    if (accessNode) 
>+      accessNode->GetDOMNode(getter_AddRefs(parentNode));

yeah, like this :-)

Comment 8

[Darin Fisher]

> Darin: Could you please look over this bug, and comment on the need for an
> assertion after the QI, and also if I need to bump the UUID on the interface
> when making these changes.

OK, adding assertions after the QI is fine.

Comment 9

[Aaron Leventhal]

Why do we need to change the UUID on a non-frozen interface when it changes? No
code review has ever asked for that before.

Comment 10

[Darin Fisher]

> Why do we need to change the UUID on a non-frozen interface when it changes? No
> code review has ever asked for that before.

Why?  Simple reason: it allows extensions to optionally make use of non-frozen
interfaces, and downgrade safely when the interface appears to no longer exist.
 If you change the vtable without changing the UUID, then you are violating one
of the fundamental rules of COM (that no interfaces should ever change).  Well,
we change private interfaces all the time, and unfortunately extension and
plugin writers use private interfaces all the time.  Historically, this has led
to crashes and major instability in Mozilla.  So, why not do the obvious thing
and at least protect ourselves from this problem by changing the UUID of
non-frozen interfaces when they change?  (Small amount of additional work on our
end results in a major benefits to extension writers.)  BTW: it only makes sense
to change the UUID of a non-frozen interface... afterall, a frozen interface is
frozen =)

Comment 11

[Philip K. Warren]

Attachment: Patch v2

Update UUID for nsIAccessible, add null checks around all QI's.

Comment 12

[Aaron Leventhal]

Comment on attachment 152950 [details] [diff] [review]
Patch v2

Sorry, I want assertions instead of null checks for the QI to nsIAccessNode
from accessible's.

No sense adding code to release builds that will never be called

Comment 13

[Philip K. Warren]

Attachment: Patch v3

Use assertions instead of null checks.

Comment 14

[Philip K. Warren]

Fixed.

Checking in public/nsIAccessible.idl;
/cvsroot/mozilla/accessible/public/nsIAccessible.idl,v  <--  nsIAccessible.idl
new revision: 1.27; previous revision: 1.26
done
Checking in public/nsIAccessibleCaret.idl;
/cvsroot/mozilla/accessible/public/nsIAccessibleCaret.idl,v  <-- 
nsIAccessibleCaret.idl
new revision: 1.3; previous revision: 1.2
done
Checking in src/atk/nsHTMLTableAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsHTMLTableAccessibleWrap.cpp,v  <-- 
nsHTMLTableAccessibleWrap.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in src/atk/nsXULTreeAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsXULTreeAccessibleWrap.cpp,v  <-- 
nsXULTreeAccessibleWrap.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.112; previous revision: 1.111
done
Checking in src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.98; previous revision: 1.97
done
Checking in src/html/nsHTMLSelectAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp,v  <-- 
nsHTMLSelectAccessible.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in src/msaa/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v  <-- 
nsAccessibleWrap.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in src/xul/nsXULMenuAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,v  <-- 
nsXULMenuAccessible.cpp
new revision: 1.28; previous revision: 1.27
done