Bug 186321 – Clean ups after r232496 and r232511

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit Misc.

[Brent Fulgham]

Clean up the code changes from r232496 and r232511 to move back to a reference-based accessor, with the assertion hidden inside this method. Also create a pointer-based accessor to be used in the handful of places we actually want a nullptr check.

These changes will make the code more readable.

[Radar WebKit Bug Importer]

<rdar://problem/40822676>

[Brent Fulgham]

Created attachment 346660 [details]
Patch

[Brent Fulgham]

Created attachment 346667 [details]
Patch

[Brent Fulgham]

Looks like the wincairo failure is due to no space left on the device. The Windows test failures look like unhappiness with the Python environment.

[Chris Dumez]

Comment on attachment 346667 [details]
Patch

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

> Source/WebCore/html/InputType.h:310
> +    HTMLInputElement* elementPtr() const { return m_element.get(); }

I am not in favor of this change. How is the caller supposed to know when it is safe to can element() instead of elementPtr()? This is *extremely* fragile and error prone. I think we should straighten out the lifetime management instead so that either:
- The HTMLInputElement *really* own the InputType and so InputType would not be RefCounted (would be a unique_ptr in HTMLInputElement). <- my preference
- We null check the input element everywhere in InputType?

In InputType.h:
    // Raw pointer because the HTMLInputElement object owns this InputType object.
    WeakPtr<HTMLInputElement> m_element;

It claims the HTMLInputElement *owns* the InputType and m_element is raw. Both are currently lies it seems.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 346667 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346667&action=review
> 
> > Source/WebCore/html/InputType.h:310
> > +    HTMLInputElement* elementPtr() const { return m_element.get(); }
> 
> I am not in favor of this change. How is the caller supposed to know when it
> is safe to can element() instead of elementPtr()? This is *extremely*
> fragile and error prone. I think we should straighten out the lifetime
> management instead so that either:
> - The HTMLInputElement *really* own the InputType and so InputType would not
> be RefCounted (would be a unique_ptr in HTMLInputElement). <- my preference

To achieve this, we could have InputType subclass CanMakeWeakPtr<InputType>. Then, before calling operations on the HTMLInputElements that may destroy the InputType, the InputType would store makeWeakPtr(*this) in a local variable and could return early
after the call into element() if !weakThis.

> - We null check the input element everywhere in InputType?
> 
> In InputType.h:
>     // Raw pointer because the HTMLInputElement object owns this InputType
> object.
>     WeakPtr<HTMLInputElement> m_element;
> 
> It claims the HTMLInputElement *owns* the InputType and m_element is raw.
> Both are currently lies it seems.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 346667 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=346667&action=review
> > 
> > > Source/WebCore/html/InputType.h:310
> > > +    HTMLInputElement* elementPtr() const { return m_element.get(); }
> > 
> > I am not in favor of this change. How is the caller supposed to know when it
> > is safe to can element() instead of elementPtr()? This is *extremely*
> > fragile and error prone. I think we should straighten out the lifetime
> > management instead so that either:
> > - The HTMLInputElement *really* own the InputType and so InputType would not
> > be RefCounted (would be a unique_ptr in HTMLInputElement). <- my preference
> 
> To achieve this, we could have InputType subclass CanMakeWeakPtr<InputType>.
> Then, before calling operations on the HTMLInputElements that may destroy
> the InputType, the InputType would store makeWeakPtr(*this) in a local
> variable and could return early
> after the call into element() if !weakThis.

I can give this a try if you'd like.

> 
> > - We null check the input element everywhere in InputType?
> > 
> > In InputType.h:
> >     // Raw pointer because the HTMLInputElement object owns this InputType
> > object.
> >     WeakPtr<HTMLInputElement> m_element;
> > 
> > It claims the HTMLInputElement *owns* the InputType and m_element is raw.
> > Both are currently lies it seems.

[Brent Fulgham]

(In reply to Chris Dumez from comment #7)
> > To achieve this, we could have InputType subclass CanMakeWeakPtr<InputType>.
> > Then, before calling operations on the HTMLInputElements that may destroy
> > the InputType, the InputType would store makeWeakPtr(*this) in a local
> > variable and could return early
> > after the call into element() if !weakThis.
> 
> I can give this a try if you'd like.

Have at it! That sounds like a great idea, and much safer. This area of code has been troublesome in the past, so anything we can do to make it safer would be a good thing.

[Chris Dumez]

(In reply to Brent Fulgham from comment #8)
> (In reply to Chris Dumez from comment #7)
> > > To achieve this, we could have InputType subclass CanMakeWeakPtr<InputType>.
> > > Then, before calling operations on the HTMLInputElements that may destroy
> > > the InputType, the InputType would store makeWeakPtr(*this) in a local
> > > variable and could return early
> > > after the call into element() if !weakThis.
> > 
> > I can give this a try if you'd like.
> 
> Have at it! That sounds like a great idea, and much safer. This area of code
> has been troublesome in the past, so anything we can do to make it safer
> would be a good thing.

Here is my proposal:
https://bugs.webkit.org/show_bug.cgi?id=188410

[Ryosuke Niwa]

Comment on attachment 346667 [details]
Patch

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

>>>> Source/WebCore/html/InputType.h:310
>>>> +    HTMLInputElement* elementPtr() const { return m_element.get(); }
>>> 
>>> I am not in favor of this change. How is the caller supposed to know when it is safe to can element() instead of elementPtr()? This is *extremely* fragile and error prone. I think we should straighten out the lifetime management instead so that either:
>>> - The HTMLInputElement *really* own the InputType and so InputType would not be RefCounted (would be a unique_ptr in HTMLInputElement). <- my preference
>>> - We null check the input element everywhere in InputType?
>>> 
>>> In InputType.h:
>>>     // Raw pointer because the HTMLInputElement object owns this InputType object.
>>>     WeakPtr<HTMLInputElement> m_element;
>>> 
>>> It claims the HTMLInputElement *owns* the InputType and m_element is raw. Both are currently lies it seems.
>> 
>> To achieve this, we could have InputType subclass CanMakeWeakPtr<InputType>. Then, before calling operations on the HTMLInputElements that may destroy the InputType, the InputType would store makeWeakPtr(*this) in a local variable and could return early
>> after the call into element() if !weakThis.
> 
> I can give this a try if you'd like.

I don't think it makes sense to have two versions of functions like this as Chris points out.
Because each InputType member functions need to be robust against getting detached from an element,
we should have an ScriptDisallowedScope, and/or create a new RAII which asserts that the input element's type doesn't change.
Hiding asserts into a helper function like this would make it harder to reason about the code.

[Brent Fulgham]

*** This bug has been marked as a duplicate of bug 188410 ***

[Ryosuke Niwa]

Comment on attachment 346667 [details]
Patch

Clearing r? flag.