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.
<rdar://problem/40822676>
Created attachment 346660 [details] Patch
Created attachment 346667 [details] Patch
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.
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.
(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.
(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.
(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.
(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
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.
*** This bug has been marked as a duplicate of bug 188410 ***
Comment on attachment 346667 [details] Patch Clearing r? flag.