Bug 158265

Created attachment 280256 [details] Patch

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

Also note iOS does not seem to be building.

(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

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

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

Created attachment 280265 [details] Patch

Comment on attachment 280265 [details] Patch Will cq+ after EWS signs off

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

FontCacheCoreText.cpp changes from this morning were apparently reverted.

Created attachment 280271 [details] Patch

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