Bug 158265 – Modernize lambda usage for all callers of RunLoop::dispatch()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modernize lambda usage for all callers of RunLoop::dispatch()

[Brady Eidson]

Created attachment 280256 [details]
Patch

[Chris Dumez]

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 :(

[Chris Dumez]

Also note iOS does not seem to be building.

[Brady Eidson]

(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

[Chris Dumez]

(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.

[Brady Eidson]

> > > 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.

[Brady Eidson]

Created attachment 280265 [details]
Patch

[Brady Eidson]

Comment on attachment 280265 [details]
Patch

Will cq+ after EWS signs off

[WebKit Commit Bot]

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

[Brady Eidson]

FontCacheCoreText.cpp changes from this morning were apparently reverted.

[Brady Eidson]

Created attachment 280271 [details]
Patch

[Brady Eidson]

The patch passed tests in the CQ 42 minutes ago but still hasn't landed. sigh. Landing manually.

[Brady Eidson]

http://trac.webkit.org/changeset/201575