Bug 112943

Created attachment 194310 [details] Patch v1

Attachment 194310 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/loader/ResourceBuffer.cpp', u'Source/WebCore/loader/ResourceBuffer.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/platform/SharedBuffer.h', u'Source/WebCore/platform/cf/SharedBufferCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm', u'Source/WebKit2/Platform/SharedMemory.h', u'Source/WebKit2/Platform/mac/SharedMemoryMac.cpp', u'Source/WebKit2/Shared/ShareableResource.cpp', u'Source/WebKit2/Shared/ShareableResource.h', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.messages.in', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1 Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:113: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:111: The parameter name "handle" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Shared/ShareableResource.cpp:68: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:69: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:70: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:71: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:72: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:74: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262: One space before end of line comments [whitespace/comments] [5] Total errors found: 9 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #2) > Attachment 194310 [details] did not pass style-queue: > Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:113: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:111: The parameter name "handle" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit2/Shared/ShareableResource.cpp:68: Use 0 instead of NULL. [readability/null] [5] > Source/WebKit2/Shared/ShareableResource.cpp:69: Use 0 instead of NULL. [readability/null] [5] > Source/WebKit2/Shared/ShareableResource.cpp:70: Use 0 instead of NULL. [readability/null] [5] > Source/WebKit2/Shared/ShareableResource.cpp:71: Use 0 instead of NULL. [readability/null] [5] > Source/WebKit2/Shared/ShareableResource.cpp:72: Use 0 instead of NULL. [readability/null] [5] > Source/WebKit2/Shared/ShareableResource.cpp:74: Use 0 instead of NULL. [readability/null] [5] > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262: One space before end of line comments [whitespace/comments] [5] > Total errors found: 9 in 21 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Fixed the 3 that should've been fixed, leaving standard CF-style in place for the 6 others.

Comment on attachment 194310 [details] Patch v1 Attachment 194310 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17250145

Comment on attachment 194310 [details] Patch v1 Attachment 194310 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17290097

Comment on attachment 194310 [details] Patch v1 Attachment 194310 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17161975

Created attachment 194319 [details] Patch v2 - Style and build fixes

Attachment 194319 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/loader/ResourceBuffer.cpp', u'Source/WebCore/loader/ResourceBuffer.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/platform/SharedBuffer.h', u'Source/WebCore/platform/cf/SharedBufferCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm', u'Source/WebKit2/Platform/SharedMemory.h', u'Source/WebKit2/Platform/mac/SharedMemoryMac.cpp', u'Source/WebKit2/Shared/ShareableResource.cpp', u'Source/WebKit2/Shared/ShareableResource.h', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.messages.in', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1 Source/WebKit2/Shared/ShareableResource.cpp:68: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:69: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:70: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:71: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:72: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:74: Use 0 instead of NULL. [readability/null] [5] Total errors found: 6 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

View in context: https://bugs.webkit.org/attachment.cgi?id=194310&action=review > Source/WebCore/loader/cache/CachedResource.cpp:979 > + // Verify the buffers are the same as if the encoded data is different than what we're replacing, > + // we don't want to replace it. I think it's almost self-evident that we wouldn't want to replace our content with something that didn't match -- what's not self-evident is how that could ever happen. I think that's a better thing to comment about. Something like: This function supports replacing an item with its value in the disk cache. Since the disk cache is race-y, we can't assume that 'newBuffer' actually matches m_data. > Source/WebCore/loader/cache/CachedResource.cpp:981 > + if (m_data->size() != newBuffer->size() || !memcmp(m_data->data(), newBuffer->data(), m_data->size())) > + return; This is wrong. memcmp returns 0 when the two buffer are equal. > Source/WebCore/loader/cache/CachedResource.h:268 > + void tryReplacingEncodedData(PassRefPtr<SharedBuffer>); Your other function is named "tryWrap", in the imperative form, so I think we should do the same here: "tryReplace". > Source/WebCore/platform/cf/SharedBufferCF.cpp:103 > + else > + append(newContents); I don't think this else case makes sense. We don't have a use case where we would want to append 'newContents', since the optimization will fail if we do. I think it would be better to rename this function to 'tryReplaceContents', and move the buffer comparison into this function as well. Then, we can just return early if 'newContents' lacks an optimized form. >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262 >> + ref(); // Balanced by a deref() in diskCacheTimerFired(). > > One space before end of line comments [whitespace/comments] [5] Style bot is right, for once. Nit: It's adoptRef(), not deref(). > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:44 > +static bool CFURLCacheIsMemMappedData(CFURLCacheRef cache, CFDataRef data) Can we use the SOFT_LINK_OPTIONAL macro here instead of rolling our own? > Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:110 > + sharedMemory->m_shouldVMDeallocateData = true; What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true. > Source/WebKit2/Shared/ShareableResource.cpp:61 > + resource->deref(); For your timer, you chose adoptRef() instead of an explicit deref(). Let's pick one style and use it everywhere. > Source/WebKit2/Shared/ShareableResource.cpp:67 > + resource.leakRef(), For your timer, you chose ref() instead of leakRef(). Let's pick one style and use it everywhere.

(In reply to comment #9) > View in context: https://bugs.webkit.org/attachment.cgi?id=194310&action=review > > > Source/WebCore/loader/cache/CachedResource.cpp:979 > > + // Verify the buffers are the same as if the encoded data is different than what we're replacing, > > + // we don't want to replace it. > > I think it's almost self-evident that we wouldn't want to replace our content with something that didn't match -- what's not self-evident is how that could ever happen. I think that's a better thing to comment about. Something like: > > This function supports replacing an item with its value in the disk cache. Since the disk cache is race-y, we can't assume that 'newBuffer' actually matches m_data. Good suggestion. > > Source/WebCore/loader/cache/CachedResource.cpp:981 > > + if (m_data->size() != newBuffer->size() || !memcmp(m_data->data(), newBuffer->data(), m_data->size())) > > + return; > > This is wrong. memcmp returns 0 when the two buffer are equal. Yikes > > Source/WebCore/loader/cache/CachedResource.h:268 > > + void tryReplacingEncodedData(PassRefPtr<SharedBuffer>); > > Your other function is named "tryWrap", in the imperative form, so I think we should do the same here: "tryReplace". Done > > Source/WebCore/platform/cf/SharedBufferCF.cpp:103 > > + else > > + append(newContents); > > I don't think this else case makes sense. We don't have a use case where we would want to append 'newContents', since the optimization will fail if we do. I think it would be better to rename this function to 'tryReplaceContents', and move the buffer comparison into this function as well. Then, we can just return early if 'newContents' lacks an optimized form. Since SharedBuffer is completely divorced from the mechanism we're adding, I was attempting to keep it general purpose. I sympathize to your suggestion but think it is flawed itself - "we can just return early if 'newContents' lacks an optimized form" is impossible to implement as we know there's a cfData but we don't know it's optimized. Maybe "tryReplaceContentsWithPlatformBuffer" is both a sufficiently general and sufficiently specific name to make us both happy? > >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262 > >> + ref(); // Balanced by a deref() in diskCacheTimerFired(). > Nit: It's adoptRef(), not deref(). Fixed > > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:44 > > +static bool CFURLCacheIsMemMappedData(CFURLCacheRef cache, CFDataRef data) > > Can we use the SOFT_LINK_OPTIONAL macro here instead of rolling our own? That was the first thing I tried, at which point I discovered: A - It doesn't do what you think it does and B - It doesn't work at all on Mac, despite the comment above it claiming it might be used on Mac. > > Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:110 > > + sharedMemory->m_shouldVMDeallocateData = true; > > What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true. I think you've misread - When an MMAPed buffer is passed in to ::createFromVMBuffer() directly, SharedMemory didn't allocate it with vm_allocate and therefore should've deallocate it with vm_deallocate. > > Source/WebKit2/Shared/ShareableResource.cpp:61 > > + resource->deref(); > > For your timer, you chose adoptRef() instead of an explicit deref(). Let's pick one style and use it everywhere. > > > Source/WebKit2/Shared/ShareableResource.cpp:67 > > + resource.leakRef(), > > For your timer, you chose ref() instead of leakRef(). Let's pick one style and use it everywhere. Normally we do this type of thing with ref()/deref(). It's easy to make ShareableResource use ref()/deref() and I've made that chance locally. In the case of the timer, there are multiple code paths to return from the function, so relying on RefPtr<> destruction to do the cleanup is both less code and more foolproof.

Created attachment 194337 [details] Patch v3

> Maybe "tryReplaceContentsWithPlatformBuffer" is both a sufficiently general and sufficiently specific name to make us both happy? Sounds good. (My main goal is just removing the call to append(). Putting "try" in the name is just a way to give us the freedom to return early without calling append().) > > What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true. > I think you've misread - When an MMAPed buffer is passed in to ::createFromVMBuffer() directly, > SharedMemory didn't allocate it with vm_allocate and therefore should've deallocate it with > vm_deallocate. Very possible! :) The call to createFromVMBuffer that I see is: RefPtr<SharedMemory> sharedMemory = createFromVMBuffer(toPointer(address), size); Five lines later I see: sharedMemory->m_shouldVMDeallocateData = true; I don't see any other calls to createFromVMBuffer(). That's what made me think that the flag is always logically true. > Normally we do this type of thing with ref()/deref(). OK.

(In reply to comment #12) > > > What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true. > > > I think you've misread - When an MMAPed buffer is passed in to ::createFromVMBuffer() directly, > > SharedMemory didn't allocate it with vm_allocate and therefore should've deallocate it with > > vm_deallocate. > > Very possible! :) > > The call to createFromVMBuffer that I see is: > > RefPtr<SharedMemory> sharedMemory = createFromVMBuffer(toPointer(address), size); > > Five lines later I see: > > sharedMemory->m_shouldVMDeallocateData = true; > > I don't see any other calls to createFromVMBuffer(). That's what made me think that the flag is always logically true. That caller is NetworkResourceLoader::tryGetShareableHandleForResource

Comment on attachment 194337 [details] Patch v3 r=me

Created attachment 194367 [details] Updated patch for EWS run

Attachment 194367 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/loader/ResourceBuffer.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/loader/mac/ResourceBuffer.mm', u'Source/WebCore/platform/SharedBuffer.h', u'Source/WebCore/platform/cf/SharedBufferCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm', u'Source/WebKit2/Platform/SharedMemory.h', u'Source/WebKit2/Platform/mac/SharedMemoryMac.cpp', u'Source/WebKit2/Shared/ShareableResource.cpp', u'Source/WebKit2/Shared/ShareableResource.h', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.messages.in', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1 Source/WebKit2/Shared/ShareableResource.cpp:69: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:70: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:71: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:72: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:73: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/ShareableResource.cpp:75: Use 0 instead of NULL. [readability/null] [5] Total errors found: 6 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

EWS is running *EXTREMELY* slowly this afternoon. Enough have passed that I'm pulling the trigger on landing.