739568 - nsXULTreeGridRowAccesible should cache its kids as nsXULTreeGridCellAccessibles not generic accessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

1. change nsAccessibleHashtable to nsRefPtrHashtable<nsHashKey<const void>, nsXULTreeGridCellAccessible> as the type of mAccessibleCache in nsXULTreeGridRowAccessible (accessible/src/xul/nsXULTreeGridAccessible.h)
2. change GetCellAccessible to return a nsXULTreeGridCellAccessible* instead of a nsAccessible*
3. while your here and since it won't be needed any more remove the cid macro stuff from nsXULTreeGridCellAccessible

Comment 1

[Mark Capella [:capella]]

How do you want to handle consumers of the GetCellAceessible function? GetCellAt() might be ok as is, RowInvalidated() might be modified with a recast (?), but ChildAtPoint(), GetChildAt(), and GetSiblingAt() return nsAccessible values.

I guess I should have asked what the benefit of changing the internal cache contents is here...

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #1)
> How do you want to handle consumers of the GetCellAceessible function?
> GetCellAt() might be ok as is, RowInvalidated() might be modified with a
> recast (?), but ChildAtPoint(), GetChildAt(), and GetSiblingAt() return
> nsAccessible values.

an nsXULTreeGridCellAccessible* can be used wherever you need a nsAccessible*, so as I sort of said in the description if the caller downcasts the result then remove that since you already have the right type, and if the caller doesn't need a nsXULTreeGridCellAccessible* only a nsAccessible* it should just keep working.

> I guess I should have asked what the benefit of changing the internal cache
> contents is here...

it means we don't need to downcast what we pull out of it.

Comment 3

[Mark Capella [:capella]]

Nah, I didn't see anything like that in the description, so I thought I'd ask.

This seems to be growing in complexity anyhow, since now I see the CycleCollectorTraverse macros / templates in nsAccCache.h such as CycleCollectorTraverseCache, ClearCache, ... etc are build around the type / keyword "nsAccessibleHashtable".

In other words unless I'm way off, we can't simply replace "nsAccessibleHashtable to nsRefPtrHashtable<nsHashKey<const void>, nsXULTreeGridCellAccessible>" without affecting cycle collection, and cloning blocks of code to support it.

Is this something you're familiar with?

Comment 4

[alexander :surkov (:asurkov)]

how about to provide templated versions of ClearCache etc and do template instantiation for nsAccessibleHashtable?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> how about to provide templated versions of ClearCache etc and do template
> instantiation for nsAccessibleHashtable?

hm, hadn't thought about that problem, but that solution seems fine.

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v1) - WIP

Attaching my current patch for the record and as-is before any GG changes ...

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 616168 [details] [diff] [review]
Patch (v1) - WIP

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

::: accessible/src/xul/nsXULTreeAccessible.cpp
@@ +243,5 @@
>    if (aWhichChild == eDeepestChild && child) {
>      // Look for accessible cell for the found item accessible.
>      nsRefPtr<nsXULTreeItemAccessibleBase> treeitem = do_QueryObject(child);
>  
> +    nsAccessible *cell = reinterpret_cast<nsAccessible*>(treeitem->GetCellAccessible(column));

type* name

it returns nsXULTreeGridCellAccessible*, you don't need casting here

@@ +598,5 @@
>    // Fire destroy event for removed tree items and delete them from caches.
>    for (PRInt32 rowIdx = aRow; rowIdx < aRow - aCount; rowIdx++) {
>  
>      void* key = reinterpret_cast<void*>(rowIdx);
> +    nsAccessible* treeItem = reinterpret_cast<nsAccessible*>(mAccessibleCache.GetWeak(key));

same

@@ +624,5 @@
>  
>    for (PRInt32 rowIdx = newRowCount; rowIdx < oldRowCount; ++rowIdx) {
>  
>      void *key = reinterpret_cast<void*>(rowIdx);
> +    nsAccessible* treeItem = reinterpret_cast<nsAccessible*>(mAccessibleCache.GetWeak(key));

same

::: accessible/src/xul/nsXULTreeAccessible.h
@@ +216,5 @@
>    /**
>     * Return cell accessible for the given column. If XUL tree accessible is not
>     * accessible table then return null.
>     */
> +  virtual nsXULTreeGridCellAccessible* GetCellAccessible(nsITreeColumn *aColumn)

type* name

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +726,5 @@
>    // Return if we failed to find tree cell in the row for the given point.
>    if (row != mRow || !column)
>      return nsnull;
>  
> +  return reinterpret_cast<nsAccessible*>(GetCellAccessible(column));

no casting and below

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 616168 [details] [diff] [review]
Patch (v1) - WIP

>+      nsXULTreeGridCellAccessible* cellAccessible = GetCellAccessible(column);
>       if (cellAccessible) {
>         nsRefPtr<nsXULTreeGridCellAccessible> cellAcc = do_QueryObject(cellAccessible);

no longer needed

Comment 9

[alexander :surkov (:asurkov)]

Mark, please address comments and request review.

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v2)

Ok, to catch this one up, I've attached the patch as-is with code nits addressed ... but I'm still looking at comment 4 as templates are new to me.

I'm also attaching a screen shot of the build errors this causes ... it shows the GC errors that are expected at this point, but also the reasons why I had placed the reinterpret_casts into the code ...

Comment 11

[Mark Capella [:capella]]

Attachment: GC and reinterpret_cast Build errors

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #10)
> Created attachment 627433 [details] [diff] [review]
> Patch (v2)
> 
> Ok, to catch this one up, I've attached the patch as-is with code nits
> addressed ... but I'm still looking at comment 4 as templates are new to me.

for this they should be pretty simple just take a template argument of the type, and update the callers to pass the type of accessible in the cache as the template rg.

Comment 13

[Mark Capella [:capella]]

Freeing up items I'm not active on @ the moment ...

Comment 14

[Tiziana Sellitto [:tiziana]]

Mark, do you think this is still a good first bug or something is changed in the meanwhile?

Comment 15

[Trevor Saunders (:tbsaunde)]

if anything this should be simpler now, there's a generic support for cycle collecting nsRefPtrHashtables now so it should be possible to remove the stuff in nsAccCache.h and just replace it with NS_IMPL_TRAVERSE(mCache) or whatever the macro is.s

Comment 16

[Iosif Adrian Mihai]

Hello everyone! I am new here and I would like to try to fix this bug. I have read the comments so far, however from what I've understood, things have changed recently (comment 15) and the approach should differ from what has been done so far. Could someone help me and please bring me up to date with what should be done here and some hints to how to do it? Thank you!

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Iosif Adrian Mihai from comment #16)
> Hello everyone! I am new here and I would like to try to fix this bug. I
> have read the comments so far, however from what I've understood, things
> have changed recently (comment 15) and the approach should differ from what
> has been done so far. Could someone help me and please bring me up to date
> with what should be done here and some hints to how to do it? Thank you!

I think comment 15 explained that.  If you have specific question maybe I can help more, otherwise I guess you should just try and fix it and see what happens.

Comment 18

[diwas joshi [:dij]]

hello, is somebody working on this bug because i would like to work on this bug, this would be my first bug so i might need some extra guidance debugging it. thanks!

Comment 19

[diwas joshi [:dij]]

Attachment: bug739568.diff

Comment 20

[diwas joshi [:dij]]

Attachment: bug739568.patch

i didn't remove the cid macro stuff from nsXULTreeGridCellAccessible because it is being used by another macro NS_INTERFACE_TABLE_INHERITED(XULTreeGridCellAccessible,
                               XULTreeGridCellAccessible) in XULTreeGridAccessible.cpp, do we really need to remove that it seems like it is being used here.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to diwas joshi [:dij] from comment #20)
> Created attachment 8515598 [details] [diff] [review]
> bug739568.patch
> 
> i didn't remove the cid macro stuff from nsXULTreeGridCellAccessible because
> it is being used by another macro
> NS_INTERFACE_TABLE_INHERITED(XULTreeGridCellAccessible,
>                                XULTreeGridCellAccessible)

if you don't have queries left then you don't need this macros anymore (make sure to fix .h part)

Comment 22

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8515598 [details] [diff] [review]
bug739568.patch

>+++ b/accessible/xul/XULTreeAccessible.cpp
>@@ -177,24 +177,24 @@ XULTreeAccessible::NativeRole()
> 
> ////////////////////////////////////////////////////////////////////////////////
> // XULTreeAccessible: Accessible implementation (DON'T put methods here)
> 
> Accessible*
> XULTreeAccessible::ChildAtPoint(int32_t aX, int32_t aY,
>                                 EWhichChildAtPoint aWhichChild)
> {
>-  nsIFrame *frame = GetFrame();
>+  nsIFrame* frame = GetFrame();

this is extranious,  please remove it

>   if (!frame)
>     return nullptr;
> 
>-  nsPresContext *presContext = frame->PresContext();
>+  nsPresContext* presContext = frame->PresContext();

likewise, and several other places below.

>@@ -208,17 +208,17 @@ XULTreeAccessible::ChildAtPoint(int32_t 
>   if (row == -1 || !column)
>     return AccessibleWrap::ChildAtPoint(aX, aY, aWhichChild);
> 
>   Accessible* child = GetTreeItemAccessible(row);
>   if (aWhichChild == eDeepestChild && child) {
>     // Look for accessible cell for the found item accessible.
>     nsRefPtr<XULTreeItemAccessibleBase> treeitem = do_QueryObject(child);
> 
>-    Accessible* cell = treeitem->GetCellAccessible(column);
>+    Accessible* cell = reinterpret_cast<Accessible*>(treeitem->GetCellAccessible(column));

cast should be unnecessary.

>+++ b/accessible/xul/XULTreeAccessible.h
>@@ -11,16 +11,17 @@
> #include "nsITreeColumns.h"
> #include "XULListboxAccessible.h"
> 
> class nsTreeBodyFrame;
> 
> namespace mozilla {
> namespace a11y {
> 
>+class XULTreeGridCellAccessible;

blank line here please

>+++ b/accessible/xul/XULTreeGridAccessible.cpp
>@@ -126,17 +126,17 @@ XULTreeGridAccessible::CellAt(uint32_t a
>     nsCoreUtils::GetSensibleColumnAt(mTree, aColumnIndex);
>   if (!column)
>     return nullptr;
> 
>   nsRefPtr<XULTreeItemAccessibleBase> rowAcc = do_QueryObject(row);
>   if (!rowAcc)
>     return nullptr;
> 
>-  return rowAcc->GetCellAccessible(column);
>+  return reinterpret_cast<Accessible*>(rowAcc->GetCellAccessible(column));

cast shouldn't be required, here and a couple places below.

>+++ b/accessible/xul/XULTreeGridAccessible.h
>@@ -10,16 +10,17 @@
> #include "TableAccessible.h"
> #include "TableCellAccessible.h"
> #include "xpcAccessibleTable.h"
> #include "xpcAccessibleTableCell.h"
> 
> namespace mozilla {
> namespace a11y {
> 
>+class XULTreeGridCellAccessible;

blank line before comment please

> 
>   // XULTreeItemAccessibleBase
>-  virtual Accessible* GetCellAccessible(nsITreeColumn* aColumn) const MOZ_OVERRIDE;
>+  virtual XULTreeGridCellAccessible* GetCellAccessible(nsITreeColumn* aColumn)
>+    const MOZ_OVERRIDE;

MOZ_FINAL too please

Comment 23

[diwas joshi [:dij]]

Attachment: bug739568.patch

Comment 24

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8516159 [details] [diff] [review]
bug739568.patch

>+++ b/accessible/xul/XULTreeAccessible.cpp
>@@ -26,17 +26,17 @@
> #include "nsIDOMXULMenuListElement.h"
> #include "nsIDOMXULMultSelectCntrlEl.h"
> #include "nsIDOMXULTreeElement.h"
> #include "nsITreeSelection.h"
> #include "nsIMutableArray.h"
> #include "nsTreeBodyFrame.h"
> #include "nsTreeColumns.h"
> #include "nsTreeUtils.h"
>-
>+#include "XULTreeGridAccessible.h"

include it after States.h instead of here

>     nsCOMPtr<nsITreeColumn> column;
>     treeColumns->GetColumnAt(colIdx, getter_AddRefs(column));
>     if (column && !nsCoreUtils::IsColumnHidden(column)) {
>-      Accessible* cellAccessible = GetCellAccessible(column);
>+      XULTreeGridCellAccessible* cellAccessible = GetCellAccessible(column);
>       if (cellAccessible) {
>-        nsRefPtr<XULTreeGridCellAccessible> cellAcc = do_QueryObject(cellAccessible);
>-
>-        nameChanged |= cellAcc->CellInvalidated();
>+        nameChanged |= cellAccessible->CellInvalidated();

you should use nsRefPtr<XULTreeGridCellAccessible> here since otherwise its possible for all refs to the cell to go away before the function finishes.  (that's totally horrible, but that's the way XUL is).

>-// XULTreeGridCellAccessible: nsISupports implementation
>-
>-NS_IMPL_CYCLE_COLLECTION_INHERITED(XULTreeGridCellAccessible, LeafAccessible,
>-                                   mTree, mColumn)

you should leave that

>-
>-NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED(XULTreeGridCellAccessible)
>-  NS_INTERFACE_TABLE_INHERITED(XULTreeGridCellAccessible,
>-                               XULTreeGridCellAccessible)
>-NS_INTERFACE_TABLE_TAIL_INHERITING(LeafAccessible)
>-NS_IMPL_ADDREF_INHERITED(XULTreeGridCellAccessible, LeafAccessible)
>-NS_IMPL_RELEASE_INHERITED(XULTreeGridCellAccessible, LeafAccessible)

and replace this by NS_IMPL_ISSUPORTS_INHERITED0

> class XULTreeGridCellAccessible : public LeafAccessible,
>                                   public TableCellAccessible
> {
> public:
> 
>   XULTreeGridCellAccessible(nsIContent* aContent, DocAccessible* aDoc,
>                             XULTreeGridRowAccessible* aRowAcc,
>                             nsITreeBoxObject* aTree, nsITreeView* aTreeView,
>                             int32_t aRow, nsITreeColumn* aColumn);
> 
>-  // nsISupports
>-  NS_DECL_ISUPPORTS_INHERITED
>-  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(XULTreeGridCellAccessible,
>-                                           LeafAccessible)

keep these

Comment 25

[diwas joshi [:dij]]

Attachment: bug739568.patch

i have corrected the mistakes in the previous patch.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try first? :)

Comment 27

[alexander :surkov (:asurkov)]

there are failures (https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3000937&repo=try)


Assertion failure: p == &_cycleCollectorGlobal (XULTreeGridCellAccessible should QI to its own CC participant), at /builds/slave/try-l64-d-00000000000000000000/build/accessible/xul/XULTreeGridAccessible.h:127

26 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/attributes/test_obj.html | application terminated with exit code 11

PROCESS-CRASH | chrome://mochitests/content/a11y/accessible/tests/mochitest/attributes/test_obj.html | application crashed [@ mozilla::a11y::XULTreeGridCellAccessible::CheckForRightParticipant()]

TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks!

Return code: 1

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #27)
> there are failures
> (https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3000937&repo=try)
> 
> 
> Assertion failure: p == &_cycleCollectorGlobal (XULTreeGridCellAccessible
> should QI to its own CC participant), at
> /builds/slave/try-l64-d-00000000000000000000/build/accessible/xul/
> XULTreeGridAccessible.h:127

I guess NS_IMPL_ISUPPORTS_INHERITED0 doesn't work for ccable things, so you need to find the right set of macros that do.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/992784f79f96