Bug 158265

Summary: Modernize lambda usage for all callers of RunLoop::dispatch()
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=158277
Bug Depends on: 158346    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Brady Eidson 2016-06-01 11:10:33 PDT
Modernize lambda usage for all callers of RunLoop::dispatch()
Comment 1 Brady Eidson 2016-06-01 11:51:56 PDT
Created attachment 280256 [details]
Patch
Comment 2 Chris Dumez 2016-06-01 12:26:49 PDT
Comment on attachment 280256 [details]
Patch

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

R=me with comments.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:-44
> -

I believe we usually have a blank line after #if and namespace statements.

> Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp:107
>          if (auto* monitor = weakPtr.get())

This does not look safe. Shouldn't we assign to a RefPtr<> right away so that we at least keep it alive until the next line?

> Source/WebCore/platform/network/DataURLDecoder.cpp:175
> +    decodeQueue().dispatch([decodeTask = createDecodeTask(url, scheduleContext, WTFMove(completionHandler))]() mutable {

Not a big deal but I kinda preferred the previous code here given the length of the line now.

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:207
> +            RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), securityOrigins = WTFMove(securityOrigins)] {

securityOrigins = indexedDatabaseOrigins() ?

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:210
> +                    websiteData.entries.append(WebsiteData::Entry { securityOrigin, WebsiteDataType::IndexedDBDatabases, 0 });

Do we really need the explicit WebsiteData::Entry ?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:548
> +    // FIXME: Key contains strings, and we're not making isolated copies of them here.

Please drop this comment. It *is* OK because the Key copy constructor creates isolated copies of all its String data members.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:669
> +            // FIXME: Record is a reasonably complex type with non-threadsafe RefCounted objects,

struct Record {
        WTF_MAKE_FAST_ALLOCATED;
    public:
        Key key;
        std::chrono::system_clock::time_point timeStamp;
        Data header;
        Data body;
    };

What's refcounted or unsafe here. I am not sure this FIXME comment is warranted.

> Source/WebKit2/Platform/IPC/Connection.cpp:140
> +                dispatchMessageAndResetDidScheduleDispatchMessagesForConnection(protectedConnection.get());

The .get() is redundant I believe.

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:558
>      RefPtr<StorageManager> storageManager(this);

Did you mean to remove this line?

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:592
> +    m_queue->dispatch([this, protectedThis = Ref<StorageManager>(*this), copiedOrigins, completionHandler = WTFMove(completionHandler)]() mutable {

We should definitely WTFMove(copiedOrigins) in I think. We don't want to copy this whole vector after we already made a deep isolated copy.

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:636
> +            localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin.get());

Do we really need this .get()?

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:639
> +            transientLocalStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin.get());

Do we really need this .get()?

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:274
> +            WTF::RunLoop::main().dispatch([callbackAggregator, origins = WTFMove(origins), websiteData = WTFMove(websiteData)]() mutable {

Do we really need the explicit WTF:: ?

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:377
> +            WTF::RunLoop::main().dispatch([callbackAggregator, origins = WTFMove(origins), websiteData = WTFMove(websiteData)]() mutable {

Do we really need the explicit WTF:: ?

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1682
> +        RunLoop::main().dispatch([lastRef = adoptRef(this)] {

adoptRef(*this) ? Otherwise we get a PassRefPtr :(
Comment 3 Chris Dumez 2016-06-01 12:27:08 PDT
Also note iOS does not seem to be building.
Comment 4 Brady Eidson 2016-06-01 13:10:19 PDT
(In reply to comment #2)
> Comment on attachment 280256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280256&action=review
> 
> R=me with comments.
> 
> > Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp:107
> >          if (auto* monitor = weakPtr.get())
> 
> This does not look safe. Shouldn't we assign to a RefPtr<> right away so
> that we at least keep it alive until the next line?

It's definitely not safe, hence the comment. I didn't feel expert enough in the expectations here to make that change.

I plan to file a followup and assign to Simon.

> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:207
> > +            RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), securityOrigins = WTFMove(securityOrigins)] {
> 
> securityOrigins = indexedDatabaseOrigins() ?

Sure.

> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:210
> > +                    websiteData.entries.append(WebsiteData::Entry { securityOrigin, WebsiteDataType::IndexedDBDatabases, 0 });
> 
> Do we really need the explicit WebsiteData::Entry ?

Not sure - I didn't give much look at things except specifically for the lambdas.
I'll try.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:548
> > +    // FIXME: Key contains strings, and we're not making isolated copies of them here.
> 
> Please drop this comment. It *is* OK because the Key copy constructor
> creates isolated copies of all its String data members.

This is a break from all other classes that have explicit isolatedCopys which let somebody reading the code verify that things are safe.

We should change that.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:669
> > +            // FIXME: Record is a reasonably complex type with non-threadsafe RefCounted objects,
> 
> struct Record {
>         WTF_MAKE_FAST_ALLOCATED;
>     public:
>         Key key;
>         std::chrono::system_clock::time_point timeStamp;
>         Data header;
>         Data body;
>     };
> 
> What's refcounted or unsafe here. I am not sure this FIXME comment is
> warranted.

It was based on the fact that Key doesn't follow the established pattern in the project of having explicit isolatedCopies.

> 
> > Source/WebKit2/Platform/IPC/Connection.cpp:140
> > +                dispatchMessageAndResetDidScheduleDispatchMessagesForConnection(protectedConnection.get());
> 
> The .get() is redundant I believe.
> 
> > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:558
> >      RefPtr<StorageManager> storageManager(this);
> 
> Did you mean to remove this line?

Yes - The protector is moved into the lambda scope.

> > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:592
> > +    m_queue->dispatch([this, protectedThis = Ref<StorageManager>(*this), copiedOrigins, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> We should definitely WTFMove(copiedOrigins) in I think. We don't want to
> copy this whole vector after we already made a deep isolated copy.

Definitely, just missed this one.

> > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:636
> > +            localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin.get());
> 
> Do we really need this .get()?
> 
> > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:639
> > +            transientLocalStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin.get());
> 
> Do we really need this .get()?
> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:274
> > +            WTF::RunLoop::main().dispatch([callbackAggregator, origins = WTFMove(origins), websiteData = WTFMove(websiteData)]() mutable {
> 
> Do we really need the explicit WTF:: ?
> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:377
> > +            WTF::RunLoop::main().dispatch([callbackAggregator, origins = WTFMove(origins), websiteData = WTFMove(websiteData)]() mutable {
> 
> Do we really need the explicit WTF:: ?
> 
> > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1682
> > +        RunLoop::main().dispatch([lastRef = adoptRef(this)] {
> 
> adoptRef(*this) ? Otherwise we get a PassRefPtr :(

lulz
Comment 5 Chris Dumez 2016-06-01 13:12:14 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > Comment on attachment 280256 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=280256&action=review
> > 
> > R=me with comments.
> > 
> > > Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp:107
> > >          if (auto* monitor = weakPtr.get())
> > 
> > This does not look safe. Shouldn't we assign to a RefPtr<> right away so
> > that we at least keep it alive until the next line?
> 
> It's definitely not safe, hence the comment. I didn't feel expert enough in
> the expectations here to make that change.
> 
> I plan to file a followup and assign to Simon.
> 
> > 
> > > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:207
> > > +            RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), securityOrigins = WTFMove(securityOrigins)] {
> > 
> > securityOrigins = indexedDatabaseOrigins() ?
> 
> Sure.
> 
> > 
> > > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:210
> > > +                    websiteData.entries.append(WebsiteData::Entry { securityOrigin, WebsiteDataType::IndexedDBDatabases, 0 });
> > 
> > Do we really need the explicit WebsiteData::Entry ?
> 
> Not sure - I didn't give much look at things except specifically for the
> lambdas.
> I'll try.
> 
> > > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:548
> > > +    // FIXME: Key contains strings, and we're not making isolated copies of them here.
> > 
> > Please drop this comment. It *is* OK because the Key copy constructor
> > creates isolated copies of all its String data members.
> 
> This is a break from all other classes that have explicit isolatedCopys
> which let somebody reading the code verify that things are safe.
> 
> We should change that.

Yes, this is on my TODO list. I'll probably get to it soon.
Comment 6 Brady Eidson 2016-06-01 13:13:50 PDT
> > > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:558
> > >      RefPtr<StorageManager> storageManager(this);
> > 
> > Did you mean to remove this line?
> 
> Yes - The protector is moved into the lambda scope.

Lambda capture.
Comment 7 Brady Eidson 2016-06-01 14:05:29 PDT
Created attachment 280265 [details]
Patch
Comment 8 Brady Eidson 2016-06-01 14:06:17 PDT
Comment on attachment 280265 [details]
Patch

Will cq+ after EWS signs off
Comment 9 WebKit Commit Bot 2016-06-01 15:05:05 PDT
Comment on attachment 280265 [details]
Patch

Rejecting attachment 280265 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 280265, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ss/WebProcessProxy.cpp
patching file Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp
patching file Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp
patching file Source/WebKit2/WebProcess/Plugins/PluginView.cpp
patching file Source/WebKit2/WebProcess/WebPage/EventDispatcher.cpp
patching file Source/WebKit2/WebProcess/WebPage/ViewUpdateDispatcher.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/1419923
Comment 10 Brady Eidson 2016-06-01 15:11:53 PDT
FontCacheCoreText.cpp changes from this morning were apparently reverted.
Comment 11 Brady Eidson 2016-06-01 15:14:27 PDT
Created attachment 280271 [details]
Patch
Comment 12 Brady Eidson 2016-06-01 16:35:57 PDT
The patch passed tests in the CQ 42 minutes ago but still hasn't landed. sigh. Landing manually.
Comment 13 Brady Eidson 2016-06-01 16:38:00 PDT
http://trac.webkit.org/changeset/201575