Bug 213130 - FileInputType should use WeakPtr for FileListCreator lambdas
Summary: FileInputType should use WeakPtr for FileListCreator lambdas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-12 08:42 PDT by Andy Estes
Modified: 2020-06-16 15:09 PDT (History)
9 users (show)

See Also:

Attachments
Patch (10.92 KB, patch)
2020-06-12 09:20 PDT, Andy Estes 		no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2020-06-12 09:41 PDT, Andy Estes 		no flags Details | Formatted Diff | Diff
Patch (11.04 KB, patch)
2020-06-12 09:44 PDT, Andy Estes 		no flags Details | Formatted Diff | Diff
Patch (10.65 KB, patch)
2020-06-12 09:56 PDT, Andy Estes 		no flags Details | Formatted Diff | Diff
Patch (10.92 KB, patch)
2020-06-12 10:01 PDT, Andy Estes 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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].