Bug 213130 – FileInputType should use WeakPtr for FileListCreator lambdas

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Andy Estes]

FileInputType should use WeakPtr for FileListCreator lambdas

[Andy Estes]

<rdar://problem/64276591>

[Andy Estes]

Created attachment 401737 [details]
Patch

[Andy Estes]

Created attachment 401738 [details]
Patch

[Andy Estes]

Created attachment 401739 [details]
Patch

[Andy Estes]

Created attachment 401742 [details]
Patch

[Andy Estes]

Created attachment 401745 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 401745 [details]
Patch

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

r=me

> Source/WebCore/html/FileInputType.cpp:-107
> -    if (m_fileListCreator)
> -        m_fileListCreator->cancel();

Should we ASSERT(!m_fileListCreator) here instead?

[Andy Estes]

Comment on attachment 401745 [details]
Patch

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

>> Source/WebCore/html/FileInputType.cpp:-107
>> -        m_fileListCreator->cancel();
> 
> Should we ASSERT(!m_fileListCreator) here instead?

m_fileListCreator can still be non-null here. It'd be harmless to leave these lines in, but we don't need to explicitly cancel anymore because the FileListCreator completion handler can now outlive its FileInputType (it'll immediately return when executed).

[David Kilzer (:ddkilzer)]

Comment on attachment 401745 [details]
Patch

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

>>> Source/WebCore/html/FileInputType.cpp:-107
>>> -        m_fileListCreator->cancel();
>> 
>> Should we ASSERT(!m_fileListCreator) here instead?
> 
> m_fileListCreator can still be non-null here. It'd be harmless to leave these lines in, but we don't need to explicitly cancel anymore because the FileListCreator completion handler can now outlive its FileInputType (it'll immediately return when executed).

I was mostly concerned about leaks.  I guess the completion handler stored on FileListCreator could leak, but that assert should be on the FileListCreator destructor.

[Andy Estes]

(In reply to David Kilzer (:ddkilzer) from comment #9)
> Comment on attachment 401745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401745&action=review
> 
> >>> Source/WebCore/html/FileInputType.cpp:-107
> >>> -        m_fileListCreator->cancel();
> >> 
> >> Should we ASSERT(!m_fileListCreator) here instead?
> > 
> > m_fileListCreator can still be non-null here. It'd be harmless to leave these lines in, but we don't need to explicitly cancel anymore because the FileListCreator completion handler can now outlive its FileInputType (it'll immediately return when executed).
> 
> I was mostly concerned about leaks.  I guess the completion handler stored
> on FileListCreator could leak, but that assert should be on the
> FileListCreator destructor.

FileListCreator's destructor does assert that the completion handler is null.

[EWS]

Committed r262962: <https://trac.webkit.org/changeset/262962>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401745 [details].