1462465 - cache AccessibleNode in Node

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[dora.tokio]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.170 Safari/537.36



Actual results:

nsNodes::GetAccessibleNode returns a new instance every time it's called.


Expected results:

It is desirable to cache AccessibleNode in Node and make it possible to operate the same instance by GetAccessibleNode

Comment 1

[dora.tokio]

I tried writing a patch, but this failed test for memory leak. [1]
Could you give me some advice?

Since AccessibleNode adopts ref-counting style, NS_ADDREF is explicitly called.

[1]https://treeherder.mozilla.org/#/jobs?repo=try&revision=e16b8de43f0f9fbb6b5795361303befcdd3159a1&selectedJob=179288807

Comment 2

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #1)
> I tried writing a patch, but this failed test for memory leak. [1]
> Could you give me some advice?
> 
> Since AccessibleNode adopts ref-counting style, NS_ADDREF is explicitly
> called.

I think this is because there's no matching RELEASE for NS_ADDREF. However, the taken approach is problematic, since the caller controls object's lifecycle. I'd day Element property should do that, i.e. it should keep a smart pointer instead.

Comment 3

[dora.tokio]

(In reply to alexander :surkov from comment #2)
> I think this is because there's no matching RELEASE for NS_ADDREF. However,
> the taken approach is problematic, since the caller controls object's
> lifecycle. I'd day Element property should do that, i.e. it should keep a
> smart pointer instead.

You mean setting a pointer of RefPtr<AccessibleNode> to SetProperty, that is, writing something like this?

> RefPtr<AccessibleNode>* pAnode = GetProperty(...)
> if (NS_FAILED(rv)) {
>   pAnode = new RefPtr<AccessibleNode>(new AccessibleNode(this));
>   rv = SetProperty(..., pAnode, nsINode::DeleteProperty<RefPtr<AccessibleNode>*>);
>   ...
> }
> RefPtr<AccessibleNode> anode = *pAnode;
> return anode.forget();

If this is it, this is essentially the same as the previous one and causes memory leaks.

In Addition, I think we may have to change the type of AccessibleNode::mDOMNode from RefPtr<nsINode> into nsINode* because when calling document.accessibleNode, AccessibleNode holding RefPtr<nsIDocument> is created by GetAccessibleNode, but it is held by nsIDocument::mPropertyTable at the same time. This may leads to circular references. I guess to solve this problem we need to determine the direction of reference between nsINode and AccessibleNode.

Comment 4

[dora.tokio]

By the way, I found out the warning such as

>"firefox:3374): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed"

appears on the console when calling GetAccessibleNode for the first time since launching Firefox by ./mach run.

It seems this has been that since before. Is it a bug already filed?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #3)
> (In reply to alexander :surkov from comment #2)
> > I think this is because there's no matching RELEASE for NS_ADDREF. However,
> > the taken approach is problematic, since the caller controls object's
> > lifecycle. I'd day Element property should do that, i.e. it should keep a
> > smart pointer instead.
> 
> You mean setting a pointer of RefPtr<AccessibleNode> to SetProperty, that
> is, writing something like this?

yes, something like this, I think a node should own AccessibleNode instance. Olli, correct?

> 
> > RefPtr<AccessibleNode>* pAnode = GetProperty(...)
> > if (NS_FAILED(rv)) {
> >   pAnode = new RefPtr<AccessibleNode>(new AccessibleNode(this));

you shouldn't allocate RefPtr in dynamic memory. Olli, do you know any good code examples or ready-to-use recipe, how to store reference counted objects as a node property?

> In Addition, I think we may have to change the type of
> AccessibleNode::mDOMNode from RefPtr<nsINode> into nsINode* because when
> calling document.accessibleNode, AccessibleNode holding RefPtr<nsIDocument>
> is created by GetAccessibleNode, but it is held by
> nsIDocument::mPropertyTable at the same time. This may leads to circular
> references. I guess to solve this problem we need to determine the direction
> of reference between nsINode and AccessibleNode.

If Node will own AccessibleNode, then AccessibleNode doesn't need to keep a strong reference to its Node I'd say. However, I know some prefer to handle cycle collections rather than dealing with raw pointers. It's Olli's component, so I will take his word on this.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Yeah, assuming the model is node -> AccessibleNode, then Get/SetProperty sounds good.
I guess https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/dom/xbl/nsXBLService.cpp#628 is quite similar case.

Remember, if AccessibleNode is cycle collectable, the node -> accessibleNode edge needs to be traversed/unlinked

Comment 7

[dora.tokio]

(In reply to Olli Pettay [:smaug] from comment #6)
> Yeah, assuming the model is node -> AccessibleNode, then Get/SetProperty
> sounds good.
> I guess
> https://searchfox.org/mozilla-central/rev/
> dc6d85680539cb7e5fe2843d38a30a0581bfefe1/dom/xbl/nsXBLService.cpp#628 is
> quite similar case.
> 
> Remember, if AccessibleNode is cycle collectable, the node -> accessibleNode
> edge needs to be traversed/unlinked

Thank you for an example. I'm afraid to say but what do you mean by traversed/unlinked ?

(In reply to alexander :surkov from comment #5)
> If Node will own AccessibleNode, then AccessibleNode doesn't need to keep a
> strong reference to its Node I'd say. However, I know some prefer to handle
> cycle collections rather than dealing with raw pointers. It's Olli's
> component, so I will take his word on this.

How to do about this matter? I'd say if AccessibleNode holds nsINode*, it needs to be informed of the destruction of that Node somehow.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Gecko has a cycle collector for handling cycles between C++ objects (cycles may cross Javascript too).
In order to handle cycle collection, strong references from object A to B need to be traversed and unlinked.
As an example of using properties and cycle collection, see https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/dom/base/Element.cpp#3401 and how that is used.

Comment 9

[dora.tokio]

(In reply to Olli Pettay [:smaug] from comment #8)
> Gecko has a cycle collector for handling cycles between C++ objects (cycles
> may cross Javascript too).
> In order to handle cycle collection, strong references from object A to B
> need to be traversed and unlinked.
> As an example of using properties and cycle collection, see
> https://searchfox.org/mozilla-central/rev/
> dc6d85680539cb7e5fe2843d38a30a0581bfefe1/dom/base/Element.cpp#3401 and how
> that is used.

Thanks for the link. I read the page about a cycle collector and tried implementing Traverse and Unlink.
However, there remain memory leaks and SEGV [1] maybe because I'm not familiar with the cycle collector yet.
Would you tell me
1. To my understanding, the cycle collector is used to detect a cycle reference by Traverse and then unlink it. However circular references between Node and AccessibleNode(Node -> AccessibleNode & AccessibleNode::mDOMNode -> Node) basically exist as its structure. I'm not sure but I think the cycle collector destructs its structure as soon as traverse called. Is it OK?

2. Whether do I have to define AccessibleNode::Unlink explicitly in which nullptr is assigned to AccessibleNode::mDOMNode?

3. There are many macros of the cycle collector. Could you give me, if any, sites referring to those?

Thank you in advance.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=30b29a31cd3b14796b154244a8a4ba1c7cb48489

Comment 10

[alexander :surkov (:asurkov)]

the problem might be because of there's a cycle collection problem of AccessibleNode itself. See bug for details 1410482. You may want to apply the cycle collection portion of the patch and see if it helps.

Comment 11

[alexander :surkov (:asurkov)]

I landed bug 1410482. Tokio, can you re-run your patch?

Comment 12

[dora.tokio]

(In reply to alexander :surkov from comment #11)
> I landed bug 1410482. Tokio, can you re-run your patch?

Actually I have yet to see what was the cause, but it seems there is not a memory leak any more. [1]
Thank you so much. :)

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1996cfde201393e26adcbdb2f314a282aceafd8f

Comment 13

[dora.tokio]

Attachment: patch

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 8981785 [details] [diff] [review]
patch

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

f=me, but redirecting the review since it's Olli's area

::: dom/base/nsINode.cpp
@@ +2282,4 @@
>  nsINode::GetAccessibleNode()
>  {
>  #ifdef ACCESSIBILITY
> +  nsresult rv;

nit: please initialize it

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8981785 [details] [diff] [review]
patch

In the future, please create patches with more context. -U 8 is good.

+  if (NS_FAILED(rv)) {
+    RefPtr<AccessibleNode> temp = anode = new AccessibleNode(this);
+    rv = SetProperty(nsGkAtoms::accessiblenode, temp.forget().take(),
+                     nsPropertyTable::SupportsDtorFunc, false);
Why last param is false? Feels surprising if AccessibleNode would change when moving a node to another document. I would expect accessible node to be somehow moved too there.

Does the spec for accessible nodes say anything about this?
If not, it really should. It should also define if the same object is supposed to
be returned.
(Perhaps the spec should use [SameObject] webidl annotation? But that would mean adopting node shouldn't change .accessibleNode)

Anyhow, we need some test for adopting nodes with AccessibleNodes to another document.
And test for testing that the same object is returned all the time when node isn't moved to another document.


(Not about this bug, but per current draft spec .accessibleNode should be in Element interface, not in Node)

So, looks good, but adopting node to another document case is unclear. If nothing else, at least a spec bug is needed. And tests testing whatever behavior we end up having.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8981785 [details] [diff] [review]
> patch
> 
> In the future, please create patches with more context. -U 8 is good.
> 
> +  if (NS_FAILED(rv)) {
> +    RefPtr<AccessibleNode> temp = anode = new AccessibleNode(this);
> +    rv = SetProperty(nsGkAtoms::accessiblenode, temp.forget().take(),
> +                     nsPropertyTable::SupportsDtorFunc, false);
> Why last param is false? Feels surprising if AccessibleNode would change
> when moving a node to another document. I would expect accessible node to be
> somehow moved too there.
> 
> Does the spec for accessible nodes say anything about this?
> If not, it really should. It should also define if the same object is
> supposed to
> be returned.
> (Perhaps the spec should use [SameObject] webidl annotation? But that would
> mean adopting node shouldn't change .accessibleNode)

agreed, I don't see a reason why accessibleNode would need to be changed, I'll rise a point at AOM group.

> Anyhow, we need some test for adopting nodes with AccessibleNodes to another
> document.
> And test for testing that the same object is returned all the time when node
> isn't moved to another document.
> 
> 
> (Not about this bug, but per current draft spec .accessibleNode should be in
> Element interface, not in Node)

right, however I'd say that accessibleNode also may make sense on text nodes as well or a document. The spec is not yet stable, and people even think to restrict accessibleNode by shadow DOM. So it should be ok to stick with taken approach for now.

> So, looks good, but adopting node to another document case is unclear. If
> nothing else, at least a spec bug is needed. And tests testing whatever
> behavior we end up having.

agreed, we should have more test coverage

Comment 17

[dora.tokio]

Attachment: patch

(In reply to Olli Pettay [:smaug] from comment #15)
> In the future, please create patches with more context. -U 8 is good.

I usually use hg export. How can I export things like hg diff -U 8?

> +  if (NS_FAILED(rv)) {
> +    RefPtr<AccessibleNode> temp = anode = new AccessibleNode(this);
> +    rv = SetProperty(nsGkAtoms::accessiblenode, temp.forget().take(),
> +                     nsPropertyTable::SupportsDtorFunc, false);
> Why last param is false? Feels surprising if AccessibleNode would change
> when moving a node to another document. I would expect accessible node to be
> somehow moved too there.

Yeah, the last param should be true, thank you.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Does 
[defaults]
diff = -p -U 8
in .hgrc not help with hg export?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8982835 [details] [diff] [review]
patch

>+  RefPtr<AccessibleNode> anode =
>+    static_cast<AccessibleNode*>(GetProperty(nsGkAtoms::accessiblenode, &rv));
>+  if (NS_FAILED(rv)) {
>+    RefPtr<AccessibleNode> temp = anode = new AccessibleNode(this);
Please split this to two lines.
anode = new ...
temp = anode;

> #ifdef ACCESSIBILITY
>-  [Func="mozilla::dom::AccessibleNode::IsAOMEnabled"]
>+  [Func="mozilla::dom::AccessibleNode::IsAOMEnabled", SameObject]
So, is SameObject actually in any spec?
But this is more a question to surkov (who needs to review this patch too to ensure this is the behavior we want.).

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 8982835 [details] [diff] [review]

> > #ifdef ACCESSIBILITY
> >-  [Func="mozilla::dom::AccessibleNode::IsAOMEnabled"]
> >+  [Func="mozilla::dom::AccessibleNode::IsAOMEnabled", SameObject]
> So, is SameObject actually in any spec?
> But this is more a question to surkov (who needs to review this patch too to
> ensure this is the behavior we want.).

Right, I think there's no reason to change AccessibleNode during Node lifecycle. It's not in the spec, so I filed issue on this https://github.com/WICG/aom/issues/121

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 8982835 [details] [diff] [review]
patch

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

r=me with Olli's comments answered

::: accessible/tests/mochitest/aom/test_general.html
@@ +102,5 @@
>      }
>  
> +    // Check if an AccessibleNode is properly cached.
> +    let node = ifrDoc.createElement("div");
> +    let node_anode = node.accessibleNode;

nit: anode is fine

@@ +106,5 @@
> +    let node_anode = node.accessibleNode;
> +    is(node_anode, node.accessibleNode, "an AccessibleNode is properly cached");
> +
> +    // Adopting node to another document doesn't change .accessibleNode
> +    anotherDoc = document.implementation.createDocument("", "", null);

let

@@ +107,5 @@
> +    is(node_anode, node.accessibleNode, "an AccessibleNode is properly cached");
> +
> +    // Adopting node to another document doesn't change .accessibleNode
> +    anotherDoc = document.implementation.createDocument("", "", null);
> +    let new_node = anotherDoc.adoptNode(node);

adopted_node

Comment 22

[dora.tokio]

Attachment: patch

(In reply to Olli Pettay [:smaug] from comment #18)
> Does 
> [defaults]
> diff = -p -U 8
> in .hgrc not help with hg export?

Thank you for the information.

I fixed it.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #21)
> > +    // Check if an AccessibleNode is properly cached.
> > +    let node = ifrDoc.createElement("div");
> > +    let node_anode = node.accessibleNode;
> 
> nit: anode is fine

I noticed the latest patch doesn't address this comment. Are you sure you like 'node_anode' name? This test used 'anode' as name.

Comment 24

[dora.tokio]

Attachment: patch

Simply I overlooked it. Thanks.

By the way, is comment #4 an already resolved bugs?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to dora.tokio from comment #24)
> Created attachment 8983542 [details] [diff] [review]
> patch
> 
> Simply I overlooked it. Thanks.
> 
> By the way, is comment #4 an already resolved bugs?

Comment 26

[Cosmin Sabou [:CosminS]]

Which patch of those two available should be landed? Or do you need both of them? Thanks.

Comment 27

[dora.tokio]

(In reply to Cosmin Sabou [:CosminS] from comment #26)
> Which patch of those two available should be landed? Or do you need both of
> them? Thanks.

Only the lastest patch(8983542) should be landed. Thanks.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85ce7eda501b
Cache AccessibleNode in Node. r=surkov,smaug

Comment 29

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/85ce7eda501b