Bug 94335 – Remove RefPtr from HTMLMeterElement::m_value

• Status: RESOLVED
• Resolution: LATER
• Component: DOM

[Kentaro Hara]

To avoid reference cycles of RefPtr<Node>s, we want to remove unnecessary RefPtr<Node>s. The rationale is describe in bug 94324.

HTMLMeterElement::m_value does not need to be a RefPtr<Node>, because it is guaranteed to point to a shadow DOM tree of the HTMLMeterElement node, which is guaranteed to exist in the subtree of the HTMLMeterElement node.

[Kentaro Hara]

Created attachment 159091 [details]
Patch

[Kentaro Hara]

Created attachment 159095 [details]
Patch

[Hajime Morrita]

Comment on attachment 159091 [details]
Patch

I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node.
Having raw pointer is too error-prone and the source of cryptic crashes even if its existence is theoretically safe
under certain circumstances.

[Kent Tamura]

(In reply to comment #3)
> (From update of attachment 159091 [details])
> I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node.
> Having raw pointer is too error-prone and the source of cryptic crashes even if its existence is theoretically safe
> under certain circumstances.

We should resolve Bug 87815 first.

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 159091 [details])
> I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node.

- Isn't there any perf concern on traversing the shadow subtree every time?

- A bunch of other code already use raw pointers. Replacing all raw pointers with accessors might not make sense.

> Having raw pointer is too error-prone and the source of cryptic crashes even if its existence is theoretically safe
> under certain circumstances.

As I explained in bug 94324, if this change caused any crash, it means that you are creating a reference cycle (i.e. memory leak).

[Hajime Morrita]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 159091 [details] [details])
> > I prefer just replacing m_value with some accessor that traverses shadow subtree and return specified node.
> 
> - Isn't there any perf concern on traversing the shadow subtree every time?
In this case, no. 
It is only touched when specific attribute value is changed
and it results massive traversals for style recalculation anyway.

> 
> - A bunch of other code already use raw pointers. Replacing all raw pointers with accessors might not make sense.
And they occasionally causes crashes. I hope we had better way for living with it.
I agree that there is no definitely better alternative for now though.

> As I explained in bug 94324, if this change caused any crash, it means that you are creating a reference cycle (i.e. memory leak).

Leaks are better than crashes, in particular touching dangling pointers
which can result a security hole.

I'm not saying we're leaking nor crashing.
I'm talking about how we can minimize the damage from future bugs.

[Kentaro Hara]

(In reply to comment #6)
> Leaks are better than crashes, in particular touching dangling pointers
> which can result a security hole.
> 
> I'm not saying we're leaking nor crashing.
> I'm talking about how we can minimize the damage from future bugs.

Got it.

- I believe this patch is safe and that we should remove "just in case" RefPtrs because they can potentially create reference cycles. But if there is any concern about future crashes, I'm not intending to land this patch.

- The first objective of this patch is to remove unnecessary RefPtrs from the code base, because they make it difficult to reason about reference cycles.

- The second objective is that these RefPtrs cause a memory leak in the new DOM lifetime management (bug 88834). However, because I noticed that it is almost impossible to completely remove all reference cycles from the code base, I'm now developing a new lifetime algorithm that can live with reference cycles. So this bug is not blocking bug 88834.

[Ahmad Saleem]

This still exist:

https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/WebCore/html/HTMLMeterElement.h#L79

[e-mail] - Is this needed anymore? Or this can be marked as "RESOLVED LATER"? Thanks!

[Ryosuke Niwa]

yeah, this is Later. no need to keep this bug open at this point since there isn't any leak or anything.