Bug 141345 – [WK2] Add logging to validate the network cache efficacy (Part 2)

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

[Chris Dumez]

Add diagnostic logging support from the NetworkProcess and use it to log information we need to validate the network cache efficacy.

[Chris Dumez]

Created attachment 246187 [details]
WIP Patch

[Chris Dumez]

Created attachment 246279 [details]
Patch

[WebKit Commit Bot]

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


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


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

[Antti Koivisto]

Comment on attachment 246279 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.h:67
> -    void retrieve(const WebCore::ResourceRequest&, std::function<void (std::unique_ptr<Entry>)>);
> +    void retrieve(uint64_t webPageID, const WebCore::ResourceRequest&, std::function<void (std::unique_ptr<Entry>)>);

Please make webPageID the seconds argument since ResourceRequest is the important one.

[Chris Dumez]

Created attachment 246294 [details]
Patch

[Chris Dumez]

Sam, can you please confirm you're OK with this? It should more or less match the feedback you gave me.

[WebKit Commit Bot]

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


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


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

[WebKit Commit Bot]

Comment on attachment 246294 [details]
Patch

Rejecting attachment 246294 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
leCachedEntryFailureKeyEv", referenced from:
      __ZN6WebKit22NetworkCacheStatistics26recordRetrievedCachedEntryEyRKNS_15NetworkCacheKeyERKN7WebCore15ResourceRequestEb in NetworkCacheStatisticsCocoa.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

** BUILD FAILED **


The following build commands failed:
	Ld /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebKit.framework/Versions/A/WebKit normal x86_64
(1 failure)

Full output: http://webkit-queues.appspot.com/results/5455962949287936

[Chris Dumez]

Created attachment 246358 [details]
Patch

[WebKit Commit Bot]

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


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


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

[WebKit Commit Bot]

Comment on attachment 246358 [details]
Patch

Clearing flags on attachment: 246358

Committed r179910: <http://trac.webkit.org/changeset/179910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused a number of assertion failures on debug bots, I'm going to roll out.

https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r179910%20(2024)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141464

[Chris Dumez]

Comment on attachment 246358 [details]
Patch

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

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:236
> +    MESSAGE_CHECK(page);

So this is the assertion we are sometimes hitting on debug bots. This is likely because by the time we make the logging decision in our background thread, the page is gone.

[Antti Koivisto]

> So this is the assertion we are sometimes hitting on debug bots. This is
> likely because by the time we make the logging decision in our background
> thread, the page is gone.

Moving page related code to NetworkResourceLoader level may be a good idea. Those get canceled when the page dies.

It is somewhat unfortunate that logging facility has to know about pages at all. Cache is not associated with any page.

[Chris Dumez]

(In reply to comment #16)
> > So this is the assertion we are sometimes hitting on debug bots. This is
> > likely because by the time we make the logging decision in our background
> > thread, the page is gone.
> 
> Moving page related code to NetworkResourceLoader level may be a good idea.
> Those get canceled when the page dies.
> 
> It is somewhat unfortunate that logging facility has to know about pages at
> all. Cache is not associated with any page.

Agreed. I have just discussed this with Sam and he'll think about how we should refactor this. In the mean time though, I'll re-upload a patch that drops IPC messages for pages that are no longer in the map. We lose some logging for some rare cases but this is OK for now.

[Chris Dumez]

Created attachment 246403 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:221:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:54:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 16 files


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

[Sam Weinig]

Comment on attachment 246403 [details]
Patch

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

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:58
> +#define MESSAGE_CHECK_NO_ASSERT(condition) do \
> +    if (!(condition)) { \
> +        connection()->markCurrentlyDispatchedMessageAsInvalid(); \
> +        return; \
> +    } \
> +while (0)

I don't think you want this.  These messages are not invalid, it's just that the page is closed.  I would instead null-check the page, and put this comment near it.

[Chris Dumez]

Committed r179972: <http://trac.webkit.org/changeset/179972>

[Chris Dumez]

(In reply to comment #20)
> Comment on attachment 246403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246403&action=review
> 
> > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:58
> > +#define MESSAGE_CHECK_NO_ASSERT(condition) do \
> > +    if (!(condition)) { \
> > +        connection()->markCurrentlyDispatchedMessageAsInvalid(); \
> > +        return; \
> > +    } \
> > +while (0)
> 
> I don't think you want this.  These messages are not invalid, it's just that
> the page is closed.  I would instead null-check the page, and put this
> comment near it.

Ok, replaced with a simple null-check before landing.