Bug 185210 – Use RetainPtr for form input type

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

Switch the InputType of the HTMLInputElement from std::unique_ptr<> to a RefPtr<>, since we sometimes want to hold a temporary reference to the value (i.e., it's not truly a unique_ptr), and the current implementation makes this impossible.

[Brent Fulgham]

<rdar://problem/39734040>

[Brent Fulgham]

Created attachment 339328 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 339328 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:130
> -    , m_inputType(createdByParser ? nullptr : InputType::createText(*this))
> +    , m_inputType(createdByParser ? RefPtr<InputType>() : InputType::createText(*this))

We can't use nullptr here!?
Alternatively, you can just assign in the constructor body instead.

[Brent Fulgham]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 339328 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339328&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:130
> > -    , m_inputType(createdByParser ? nullptr : InputType::createText(*this))
> > +    , m_inputType(createdByParser ? RefPtr<InputType>() : InputType::createText(*this))
> 
> We can't use nullptr here!?
> Alternatively, you can just assign in the constructor body instead.

Yeah -- the type difference between nullptr_t and RefPtr<InputType> makes the compiler angry.

I'll do the assignment in the constructor body as you suggest.

[Brent Fulgham]

Committed r231291: <https://trac.webkit.org/changeset/231291>

[Darin Adler]

I’m really concerned about the safety of this change. While it makes it safer to access an InputType by extending the lifetime of the InputType itself as needed when someone uses a RefPtr for the InputType object, in those same cases, the reference back from the InputType to the HTMLInputElement is no longer safe. That reference can now point to a deallocated HTMLInputElement, and will in the very same cases that where the InputType object would have been deallocated before.

Specifically this comment is now false:

    // Raw pointer because the HTMLInputElement object owns this InputType object.
    HTMLInputElement& m_element;

HTMLInputElement is no longer the sole owner of the InputType object.

Anywhere we hold a "temporary reference" to an InputType is still unsafe if that InputType makes any use of m_element, unless the code also holds a "temporary reference" to the input element as well.

[Brent Fulgham]

(In reply to Darin Adler from comment #6)
> I’m really concerned about the safety of this change. While it makes it
> safer to access an InputType by extending the lifetime of the InputType
> itself as needed when someone uses a RefPtr for the InputType object, in
> those same cases, the reference back from the InputType to the
> HTMLInputElement is no longer safe. That reference can now point to a
> deallocated HTMLInputElement, and will in the very same cases that where the
> InputType object would have been deallocated before.
> 
> Specifically this comment is now false:
> 
>     // Raw pointer because the HTMLInputElement object owns this InputType
> object.
>     HTMLInputElement& m_element;
> 
> HTMLInputElement is no longer the sole owner of the InputType object.
> 
> Anywhere we hold a "temporary reference" to an InputType is still unsafe if
> that InputType makes any use of m_element, unless the code also holds a
> "temporary reference" to the input element as well.

That's a good point. This should probably be a WeakPtr to avoid the situation you describe. Could you file a Bugzilla to do that?

[Ryosuke Niwa]

(In reply to Brent Fulgham from comment #7)
> (In reply to Darin Adler from comment #6)
> > I’m really concerned about the safety of this change. While it makes it
> > safer to access an InputType by extending the lifetime of the InputType
> > itself as needed when someone uses a RefPtr for the InputType object, in
> > those same cases, the reference back from the InputType to the
> > HTMLInputElement is no longer safe. That reference can now point to a
> > deallocated HTMLInputElement, and will in the very same cases that where the
> > InputType object would have been deallocated before.
> > 
> > Specifically this comment is now false:
> > 
> >     // Raw pointer because the HTMLInputElement object owns this InputType
> > object.
> >     HTMLInputElement& m_element;
> > 
> > HTMLInputElement is no longer the sole owner of the InputType object.
> > 
> > Anywhere we hold a "temporary reference" to an InputType is still unsafe if
> > that InputType makes any use of m_element, unless the code also holds a
> > "temporary reference" to the input element as well.
> 
> That's a good point. This should probably be a WeakPtr to avoid the
> situation you describe. Could you file a Bugzilla to do that?

If you're going to change that to use WeakPtr. Please be sure to perf-test it prior to landing. I'd expect it to be a Speedometer regression.

[Darin Adler]

(In reply to Brent Fulgham from comment #7)
> Could you file a Bugzilla to do that?

I’m not sure why you’re asking me to file the bug.

[Brent Fulgham]

(In reply to Darin Adler from comment #9)
> (In reply to Brent Fulgham from comment #7)
> > Could you file a Bugzilla to do that?
> 
> I’m not sure why you’re asking me to file the bug.

I filed it (Bug 186096). Maybe you can take a look when you get a chance. I'm still validating to see if I broke anything with that change, and I'll need to run perf tests before completing the work, but I'd appreciate your input on the direction of the change.