Bug 213130

Summary: FileInputType should use WeakPtr for FileListCreator lambdas
Product: WebKit Reporter: Andy Estes <aestes>
Component: FormsAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, ddkilzer, esprehn+autocc, ews-watchlist, gyuyoung.kim, mifenton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=213259
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andy Estes 2020-06-12 08:42:13 PDT 
FileInputType should use WeakPtr for FileListCreator lambdas
Comment 1 Andy Estes 2020-06-12 08:42:49 PDT 
<rdar://problem/64276591>
Comment 2 Andy Estes 2020-06-12 09:20:13 PDT Comment hidden (obsolete)
Comment 3 Andy Estes 2020-06-12 09:41:03 PDT Comment hidden (obsolete)
Comment 4 Andy Estes 2020-06-12 09:44:16 PDT Comment hidden (obsolete)
Comment 5 Andy Estes 2020-06-12 09:56:41 PDT Comment hidden (obsolete)
Comment 6 Andy Estes 2020-06-12 10:01:42 PDT 
Created attachment 401745 [details]
Patch
Comment 7 David Kilzer (:ddkilzer) 2020-06-12 10:22:03 PDT 
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?
Comment 8 Andy Estes 2020-06-12 10:41:53 PDT 
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).
Comment 9 David Kilzer (:ddkilzer) 2020-06-12 11:04:08 PDT 
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.
Comment 10 Andy Estes 2020-06-12 11:33:15 PDT 
(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.
Comment 11 EWS 2020-06-12 11:40:29 PDT 
Committed r262962: <https://trac.webkit.org/changeset/262962>

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