Bug 112387 – NetworkProcess should send vm_copied, mmap'ed memory to WebProcesses when a resource is already in the disk cache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

NetworkProcess should send vm_copied, mmap'ed memory to WebProcesses when a resource is already in the disk cache

In radar as <rdar://problem/13414153>

[Brady Eidson]

Created attachment 193197 [details]
Patch v1

[WebKit Review Bot]

Attachment 193197 [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/platform/network/cf/ResourceRequest.h', u'Source/WebCore/platform/network/mac/ResourceRequestMac.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm', 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.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp', u'Source/WebKit2/WebProcess/mac/WebProcessMac.mm']" exit_code: 1
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:119:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:120:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:121:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:122:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:123:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Platform/SharedMemory.h:87:  The parameter name "size_t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #2)
> Attachment 193197 [details] did not pass style-queue:

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:119:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

Fixed this

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:120:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:121:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:122:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:123:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]

That's preferred CF-style.  Without strong objections I'll leave it.

> Source/WebKit2/Platform/SharedMemory.h:87:  The parameter name "size_t" adds no information, so it should be removed.  [readability/parameter_name] [5]

Boneheaded mistake.  Will fix.

[Alexey Proskuryakov]

Comment on attachment 193197 [details]
Patch v1

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

I only had a brief look at the patch. Would be helpful to talk it thorough in person (mostly for my education).

> Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm:45
> +#import <CFNetwork/CFURLCache.h>

Nope.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:32
> +#include <CFNetwork/CFURLCache.h>

Nope.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:50
> +    // We have to ask the WebCore::ResourceRequest for its CFURLRequest on the main thread because
> +    // of AtomicString thread safety issues.
> +    __block CFURLRequestRef cfRequest = 0;
> +    dispatch_sync(dispatch_get_main_queue(), ^{
> +        cfRequest = request().cfURLRequest(DoNotUpdateHTTPBody);
> +    });

I don' think that this is needed. CFURLRequest is going to be in ResourceRequest already, so this is just a plain getter.

[Geoffrey Garen]

Comment on attachment 193197 [details]
Patch v1

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

r=me

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:87
> +    static size_t fileBackedResourceThreshold();

Might be nice to call this "minimum" or "minimumSize" instead of "threshold".

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:65
> +    // We only care about the vm_copy optimization for resources that should be file backed.

This should really be a FIXME about using an API instead of a guess in future.

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:-119
> -        [nsurlCache setDiskCapacity:0];

This is no longer necessary / correct. The WebProcess no longer shares the disk cache with the network process, so it is appropriate for the WebProcess to have a zero-sized disk cache, to save SQL memory.

[Build Bot]

Comment on attachment 193197 [details]
Patch v1

Attachment 193197 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17112224

[Geoffrey Garen]

/Volumes/Data/EWS/WebKit/Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:32:10: fatal error: 'CFNetwork/CFURLCache.h' file not found
#include <CFNetwork/CFURLCache.h>
         ^

Looks like ap is the smart one in this crowd.

[Build Bot]

Comment on attachment 193197 [details]
Patch v1

Attachment 193197 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17132166

[Build Bot]

Comment on attachment 193197 [details]
Patch v1

Attachment 193197 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17126469

[Brady Eidson]

(In reply to comment #4)
> (From update of attachment 193197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193197&action=review
> 
> > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:50
> > +    // We have to ask the WebCore::ResourceRequest for its CFURLRequest on the main thread because
> > +    // of AtomicString thread safety issues.
> > +    __block CFURLRequestRef cfRequest = 0;
> > +    dispatch_sync(dispatch_get_main_queue(), ^{
> > +        cfRequest = request().cfURLRequest(DoNotUpdateHTTPBody);
> > +    });
> 
> I don' think that this is needed. CFURLRequest is going to be in ResourceRequest already, so this is just a plain getter.

Unfortunately I found in testing that getting the cfURLRequest() creates atomic strings on the background loader thread, even though the ResourceRequest is already wrapping an NSURLRequest.

I could add a different mode to ResourceRequest to just fetch the CFURLRequest that exists without "updating platform object" to bypass this, if you think it'd be better.  That would open up the possibility that other people start using that accessor when what *THEY* really want is the traditional accessor that does the platform updating.

[Brady Eidson]

(In reply to comment #7)
> /Volumes/Data/EWS/WebKit/Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:32:10: fatal error: 'CFNetwork/CFURLCache.h' file not found
> #include <CFNetwork/CFURLCache.h>
>          ^
> 
> Looks like ap is the smart one in this crowd.

YUP.

I probably won't get to fixing that up tonight, but will do over the weekend.

I like your naming suggestion, too.

Thanks for the review!

[Alexey Proskuryakov]

> Unfortunately I found in testing that getting the cfURLRequest() creates atomic strings on the background loader thread, even though the ResourceRequest is already wrapping an NSURLRequest.

I'd like to understand the mechanics of this.

It is not necessarily wrong to create AtomicStrings on a non-main thread. It's using them on multiple threads that it wrong.

[Brady Eidson]

(In reply to comment #12)
> > Unfortunately I found in testing that getting the cfURLRequest() creates atomic strings on the background loader thread, even though the ResourceRequest is already wrapping an NSURLRequest.
> 
> I'd like to understand the mechanics of this.
> 
> It is not necessarily wrong to create AtomicStrings on a non-main thread. It's using them on multiple threads that it wrong.

Pretty straight forward:
0 - Be on a background loading thread
1 - Call ResourceHandle::cfURLRequest()
2 - updatePlatformRequest() creates some static atomic strings
3 - Later the load finishes and the thread is destroyed.
4 - WTFThreadData is destroyed which...
5 - Destroys the AtomicStringTable which...
6 - setIsAtomic(false) on each atomic string...
7 - ... Even the static ones....
8 -  ASSERT(!isStatic());

Thread 3 Crashed:
0   com.apple.JavaScriptCore      	0x00000001091938a1 WTF::StringImpl::setIsAtomic(bool) + 97 (StringImpl.h:523)
1   com.apple.JavaScriptCore      	0x0000000109197de9 WTF::AtomicStringTable::destroy(WTF::AtomicStringTable*) + 105 (AtomicString.cpp:103)
2   com.apple.JavaScriptCore      	0x00000001091ea470 WTF::WTFThreadData::~WTFThreadData() + 48 (WTFThreadData.cpp:54)
3   com.apple.JavaScriptCore      	0x00000001091ea435 WTF::WTFThreadData::~WTFThreadData() + 21 (WTFThreadData.cpp:56)
4   com.apple.JavaScriptCore      	0x0000000108cadb87 WTF::ThreadSpecific<WTF::WTFThreadData>::destroy(void*) + 55 (ThreadSpecific.h:231)

Now, we could definitely also solve this by adding a new getExistingCFURLRequestOrNull() accessor instead of doing the thread hopping.  After half a night's sleep, I think with a scary enough name like that people wouldn't ever feel inclined to use it.

[Alexey Proskuryakov]

> 1 - Call ResourceHandle::cfURLRequest()
> 2 - updatePlatformRequest() creates some static atomic strings

Why does it do this? We are handling a response, so request's platform data should have already been created. It should not be needing an update.

I do not think that adding thread hopping per function calls is the right way forward. We either need to make the work ResourceHandle does thread safe, or at least to minimize thread hopping by doing it once, immediately on receiving a delegate call.

[Brady Eidson]

(In reply to comment #14)
> > 1 - Call ResourceHandle::cfURLRequest()
> > 2 - updatePlatformRequest() creates some static atomic strings
> 
> Why does it do this? We are handling a response, so request's platform data should have already been created. It should not be needing an update.
> 
> I do not think that adding thread hopping per function calls is the right way forward. We either need to make the work ResourceHandle does thread safe, or at least to minimize thread hopping by doing it once, immediately on receiving a delegate call.

Needing this was a cross between something boneheaded I was doing before asking for the cfurlresponse.  It's being removed before landing.

[Brady Eidson]

Created attachment 193583 [details]
Patch for landing (EWS check)

[WebKit Review Bot]

Attachment 193583 [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/platform/network/cf/ResourceRequest.h', u'Source/WebCore/platform/network/mac/ResourceRequestMac.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm', 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.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:120:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:121:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:122:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:123:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

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

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> http://trac.webkit.org/changeset/146088

iOS build fix in r146177:  http://trac.webkit.org/r146177