Bug 231220 – Clean up shouldAutofocus in HTMLFormControlElement.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Tim Nguyen (:ntim)]

Factor out uses of `element->document()`.

[Tim Nguyen (:ntim)]

Created attachment 440183 [details]
Patch

[youenn fablet]

Comment on attachment 440183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440183&action=review

> Source/WebCore/html/HTMLFormControlElement.cpp:211
> +    Ref document = element->document();

Not sure we need to ref it, we are only using document getters, then calling addConsoleMessage and returning.
I would just do auto& document = element->document();

[Tim Nguyen (:ntim)]

Created attachment 440189 [details]
Patch

[Tim Nguyen (:ntim)]

Committed r283543 (242508@main): <https://commits.webkit.org/242508@main>

[Radar WebKit Bug Importer]

<rdar://problem/83878528>

[Ryosuke Niwa]

Comment on attachment 440189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440189&action=review

> Source/WebCore/html/HTMLFormControlElement.cpp:211
> +    auto& document = element.document();

We should store this in Ref.

[Tim Nguyen (:ntim)]

(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 440189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440189&action=review
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:211
> > +    auto& document = element.document();
> 
> We should store this in Ref.

See comment 2 by Youenn. Making the argument a `const HTMLFormControlElement&` should be sufficient for safety, but if you think Ref is necessary here, please file a bug and mention why.

[Ryosuke Niwa]

(In reply to Tim Nguyen (:ntim) from comment #7)
> (In reply to Ryosuke Niwa from comment #6)
> > Comment on attachment 440189 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=440189&action=review
> > 
> > > Source/WebCore/html/HTMLFormControlElement.cpp:211
> > > +    auto& document = element.document();
> > 
> > We should store this in Ref.
> 
> See comment 2 by Youenn. Making the argument a `const
> HTMLFormControlElement&` should be sufficient for safety, but if you think
> Ref is necessary here, please file a bug and mention why.

An object being const is no guarantee of safety. There are thousands of const_cast in WebKit.

Also, generally, people who leave code comments like this isn't responsible for filing a new bug. It's on you.

[Tim Nguyen (:ntim)]

(In reply to Ryosuke Niwa from comment #8)
> An object being const is no guarantee of safety. There are thousands of
> const_cast in WebKit.
> 
> Also, generally, people who leave code comments like this isn't responsible
> for filing a new bug. It's on you.

I don't mind filing a bug, though this is against what Youenn said in comment 2 (who told me to use `auto&` instead of `Ref`). Why is `Ref` necessary in this case?

[Ryosuke Niwa]

(In reply to Tim Nguyen (:ntim) from comment #9)
> (In reply to Ryosuke Niwa from comment #8)
> > An object being const is no guarantee of safety. There are thousands of
> > const_cast in WebKit.
> > 
> > Also, generally, people who leave code comments like this isn't responsible
> > for filing a new bug. It's on you.
> 
> I don't mind filing a bug, though this is against what Youenn said in
> comment 2 (who told me to use `auto&` instead of `Ref`). Why is `Ref`
> necessary in this case?

Generally, whenever we're strong a pointer or a reference to a ref counted object in stack, we want to use Ref/RefPtr instead of a raw pointer / raw reference unless we're only calling trivial functions (e.g. just returning a member variable). See https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html

[youenn fablet]

(In reply to Ryosuke Niwa from comment #10)
> (In reply to Tim Nguyen (:ntim) from comment #9)
> > (In reply to Ryosuke Niwa from comment #8)
> > > An object being const is no guarantee of safety. There are thousands of
> > > const_cast in WebKit.
> > > 
> > > Also, generally, people who leave code comments like this isn't responsible
> > > for filing a new bug. It's on you.
> > 
> > I don't mind filing a bug, though this is against what Youenn said in
> > comment 2 (who told me to use `auto&` instead of `Ref`). Why is `Ref`
> > necessary in this case?
> 
> Generally, whenever we're strong a pointer or a reference to a ref counted
> object in stack, we want to use Ref/RefPtr instead of a raw pointer / raw
> reference unless we're only calling trivial functions (e.g. just returning a
> member variable). See
> https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html

I think shouldAutofocus is at the boundary.
It is possible to refactor it to only call trivial getters, the only call (addConsoleMessage) could be moved outside if needed by returning a String error message (addConsoleMesage would be called by shouldAutofocus caller if string is not null).

Also, why refing, if we can just call document() each time, which is probably fast.

[Ryosuke Niwa]

(In reply to youenn fablet from comment #11)
> (In reply to Ryosuke Niwa from comment #10)
> > (In reply to Tim Nguyen (:ntim) from comment #9)
> > > (In reply to Ryosuke Niwa from comment #8)
> > > > An object being const is no guarantee of safety. There are thousands of
> > > > const_cast in WebKit.
> > > > 
> > > > Also, generally, people who leave code comments like this isn't responsible
> > > > for filing a new bug. It's on you.
> > > 
> > > I don't mind filing a bug, though this is against what Youenn said in
> > > comment 2 (who told me to use `auto&` instead of `Ref`). Why is `Ref`
> > > necessary in this case?
> > 
> > Generally, whenever we're strong a pointer or a reference to a ref counted
> > object in stack, we want to use Ref/RefPtr instead of a raw pointer / raw
> > reference unless we're only calling trivial functions (e.g. just returning a
> > member variable). See
> > https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html
> 
> I think shouldAutofocus is at the boundary.

There is no such a thing as "at boundary". The rule here dictates that we should be strong it in Ref/RefPtr.

> It is possible to refactor it to only call trivial getters, the only call
> (addConsoleMessage) could be moved outside if needed by returning a String
> error message (addConsoleMesage would be called by shouldAutofocus caller if
> string is not null).

Since such a refactoring hasn't been done, we should just store it in Ref/RefPtr. It's really not useful to avoid ref-counting like that.

> Also, why refing, if we can just call document() each time, which is
> probably fast.

No, document() dereferences m_treeScope and then has to check whether m_treeScope is a document or not. If a node is inside a shadow tree, then it has to perform a secondary lookup to obtain the document. Also, calling document() each time isn't safe either because if you're calling a non-trivial function on Document (or its superclass), then such a non-trivial function could end up invoking some other function (e.g. IPC code) and trigger a synchronous destruction of Document.

Generally, we don't want everyone who has to modify any code in WebCore to having to reason about what is or what isn't safe.

Here, instead of contemplating why calling a given function is safe on a document, we should simply keep the document alive longer.

[youenn fablet]

> There is no such a thing as "at boundary". The rule here dictates that we
> should be strong it in Ref/RefPtr.

There is no use of document after a non-trivial call, that is why I think it is sort of a gray area.

> > It is possible to refactor it to only call trivial getters, the only call
> > (addConsoleMessage) could be moved outside if needed by returning a String
> > error message (addConsoleMesage would be called by shouldAutofocus caller if
> > string is not null).
> 
> Since such a refactoring hasn't been done, we should just store it in
> Ref/RefPtr. It's really not useful to avoid ref-counting like that.

I can look at the refactoring, I usually like splitting safe/non mutable code from mutable code.

> > Also, why refing, if we can just call document() each time, which is
> > probably fast.
> 
> No, document() dereferences m_treeScope and then has to check whether
> m_treeScope is a document or not. If a node is inside a shadow tree, then it
> has to perform a secondary lookup to obtain the document.

Quickly looking at the code, I do not see the tree scope check nor the shadow tree specific path.
I might have missed something.

> Also, calling
> document() each time isn't safe either because if you're calling a
> non-trivial function on Document (or its superclass), then such a
> non-trivial function could end up invoking some other function (e.g. IPC
> code) and trigger a synchronous destruction of Document.

Agreed but then we have a problem since the expectation is that having a valid element guarantees having a valid document reference.
So we should then ref element as well?

[youenn fablet]

> I can look at the refactoring, I usually like splitting safe/non mutable
> code from mutable code.

I had a try at https://bugs.webkit.org/show_bug.cgi?id=231417

[Ryosuke Niwa]

(In reply to youenn fablet from comment #13)
> > There is no such a thing as "at boundary". The rule here dictates that we
> > should be strong it in Ref/RefPtr.
> 
> There is no use of document after a non-trivial call, that is why I think it
> is sort of a gray area.

The problem is that this kind of code is fragile. Any future changes to any one of currently trivial functions can introduce a new use-after-free. We don't want to make WebKit totally unmaintainable by making every function change require a massive audit of every caller as well as every ancestor callers. This immediately results in exponential growth of code to audit, and results either in new use-after-free to be introduced or makes it impossible to make any future code changes. Either option is unacceptable.

> > > It is possible to refactor it to only call trivial getters, the only call
> > > (addConsoleMessage) could be moved outside if needed by returning a String
> > > error message (addConsoleMesage would be called by shouldAutofocus caller if
> > > string is not null).
> > 
> > Since such a refactoring hasn't been done, we should just store it in
> > Ref/RefPtr. It's really not useful to avoid ref-counting like that.
> 
> I can look at the refactoring, I usually like splitting safe/non mutable
> code from mutable code.

It doesn't matter whether such a refactoring is done or not. The above issue withstands regardless.

> > > Also, why refing, if we can just call document() each time, which is
> > > probably fast.
> > 
> > No, document() dereferences m_treeScope and then has to check whether
> > m_treeScope is a document or not. If a node is inside a shadow tree, then it
> > has to perform a secondary lookup to obtain the document.
> 
> Quickly looking at the code, I do not see the tree scope check nor the
> shadow tree specific path.
> I might have missed something.

Node::document is defined as this:

Document& document() const { return treeScope().documentScope(); }
TreeScope& treeScope() const
{
    ASSERT(m_treeScope);
    return *m_treeScope;
}

TreeScope::documentScope is defined as follows:
Document& documentScope() const { return m_documentScope.get(); }

Here, m_documentScope != this when TreeScope is a ShadowRoot.

> > Also, calling
> > document() each time isn't safe either because if you're calling a
> > non-trivial function on Document (or its superclass), then such a
> > non-trivial function could end up invoking some other function (e.g. IPC
> > code) and trigger a synchronous destruction of Document.
> 
> Agreed but then we have a problem since the expectation is that having a
> valid element guarantees having a valid document reference.
> So we should then ref element as well?

That's on the caller. The caller should keep the element alive per rules in https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html.

A subtle issue here is that an element can be the last thing keeping this document alive, and if there is any script execution, then that script can adopt this element to another document, making document eligible for deletion.

There are hundreds of subtle object model subtleties like this scattered across WebCore, and most WebKit engineers aren't familiar with all of them. As such, it's quite silly to rely on everyone to be aware of them all and carefully remembering to only Ref when it's absolutely needed.

This is precisely why we're adopting new rule on how to reply Ref/RefPtr as outlined in https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html. Please go talk with Geoff if you have any questions or concerns.