Bug 186321 - Clean ups after r232496 and r232511
Summary: Clean ups after r232496 and r232511
Status: RESOLVED DUPLICATE of bug 188410
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 186096 186304
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-05 14:06 PDT by Brent Fulgham
Modified: 2018-08-21 16:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (114.63 KB, patch)
2018-08-06 14:59 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (114.15 KB, patch)
2018-08-06 17:20 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2018-06-05 14:06:52 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-06-05 14:07:48 PDT
<rdar://problem/40822676>
Comment 2 Brent Fulgham 2018-08-06 14:59:41 PDT
Created attachment 346660 [details]
Patch
Comment 3 Brent Fulgham 2018-08-06 17:20:59 PDT
Created attachment 346667 [details]
Patch
Comment 4 Brent Fulgham 2018-08-07 09:47:43 PDT
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 5 Chris Dumez 2018-08-07 09:56:57 PDT
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.
Comment 6 Chris Dumez 2018-08-07 10:19:37 PDT
(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.
Comment 7 Chris Dumez 2018-08-07 10:33:17 PDT
(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.
Comment 8 Brent Fulgham 2018-08-07 11:16:46 PDT
(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.
Comment 9 Chris Dumez 2018-08-08 13:02:53 PDT
(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 10 Ryosuke Niwa 2018-08-09 16:44:57 PDT
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.
Comment 11 Brent Fulgham 2018-08-21 16:02:10 PDT

*** This bug has been marked as a duplicate of bug 188410 ***
Comment 12 Ryosuke Niwa 2018-08-21 16:21:14 PDT
Comment on attachment 346667 [details]
Patch

Clearing r? flag.