Bug 158862 – No need to ref connection in lambda inside NetworkResourceLoader::tryStoreAsCacheEntry()

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

[Chris Dumez]

No need to ref connection in lambda inside NetworkResourceLoader::tryStoreAsCacheEntry(). We already ref the NetworkResourceLoader which hold a ref to the connection.

[Chris Dumez]

Created attachment 281521 [details]
Patch

[WebKit Commit Bot]

Attachment 281521 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:385:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:109:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


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

[Darin Adler]

Comment on attachment 281521 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:518
> -    // Keep the connection alive.
> -    RefPtr<NetworkConnectionToWebProcess> connection(&connectionToWebProcess());
> -    RefPtr<NetworkResourceLoader> loader(this);
> -    NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [loader, connection](auto& mappedBody) {
> +    NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [this, loader = Ref<NetworkResourceLoader>(*this)](auto& mappedBody) {

I have mixed feelings about this pattern with a lambda that captures both "this" and a named strong reference to this that refs/derefs. I would be tempted to just not capture this, the way the old code did. I think it makes the lifetime clearer.

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 281521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281521&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:518
> > -    // Keep the connection alive.
> > -    RefPtr<NetworkConnectionToWebProcess> connection(&connectionToWebProcess());
> > -    RefPtr<NetworkResourceLoader> loader(this);
> > -    NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [loader, connection](auto& mappedBody) {
> > +    NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [this, loader = Ref<NetworkResourceLoader>(*this)](auto& mappedBody) {
> 
> I have mixed feelings about this pattern with a lambda that captures both
> "this" and a named strong reference to this that refs/derefs. I would be
> tempted to just not capture this, the way the old code did. I think it makes
> the lifetime clearer.

Ok, I can keep this part of the change out. I think I have seen Brady do it so I was trying to match.

[Chris Dumez]

Created attachment 281523 [details]
Patch

[WebKit Commit Bot]

Attachment 281523 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:385:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:109:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


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

[Chris Dumez]

Created attachment 281524 [details]
Patch

[WebKit Commit Bot]

Attachment 281524 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:385:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:109:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


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

[Chris Dumez]

Comment on attachment 281524 [details]
Patch

Clearing flags on attachment: 281524

Committed r202154: <http://trac.webkit.org/changeset/202154>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 281521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281521&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:518
> > -    // Keep the connection alive.
> > -    RefPtr<NetworkConnectionToWebProcess> connection(&connectionToWebProcess());
> > -    RefPtr<NetworkResourceLoader> loader(this);
> > -    NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [loader, connection](auto& mappedBody) {
> > +    NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [this, loader = Ref<NetworkResourceLoader>(*this)](auto& mappedBody) {
> 
> I have mixed feelings about this pattern with a lambda that captures both
> "this" and a named strong reference to this that refs/derefs. I would be
> tempted to just not capture this, the way the old code did. I think it makes
> the lifetime clearer.

I think capturing both is a good idea when the body of the lambda is not just a few lines, the code looks cleaner to me than using protectedThis. all the time. Maybe if we document it, and enforce that the pattern should always be [this, protectedThis = Ref<Foo>(*this)...] to make it more obvious that we are capturing both just to use this implicitly and ensure its lifetime.

[Antti Koivisto]

If the lambda outlives the method scope I think it significantly clearer to access the object via informatively named strong reference (not called protectedThis!) rather than use the double capture trick.

[Brady Eidson]

(In reply to comment #12)
> If the lambda outlives the method scope I think it significantly clearer to
> access the object via informatively named strong reference (not called
> protectedThis!) rather than use the double capture trick.

Every lambda I've written I've double captured.

Whether or not the lambda outlives method scope, the style consideration is about how the lambda reads to a human, and it is always obvious when you're reading what "this" is.

Indeed, one main reason in my push for "protectedThis" was based on the notion that you never actually use that variable in a lambda because you should be capturing this.

So, while this is obviously subjective, I disagree completely.

[Brady Eidson]

(In reply to comment #13)
> Whether or not the lambda outlives method scope, the style consideration is
> about how the lambda reads to a human, and it is always obvious when you're
> reading what "this" is.

To be explicit, as I left it unsaid - When you're reading a lambda inside a method scope, you're automatically reading that code as if it were in that method scope.

In fact, when you have an explicit non-this variable inside the lambda, I think it reads *worse*, because you're making a jump from "I'm reading a standard a member function" to "I'm reading something else"