Summary: | [WK2] Add logging to validate the network cache efficacy (Part 2) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, commit-queue, koivisto, sam | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 141269, 141464 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Chris Dumez
2015-02-06 16:34:16 PST
Created attachment 246187 [details]
WIP Patch
Created attachment 246279 [details]
Patch
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.
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. Created attachment 246294 [details]
Patch
Sam, can you please confirm you're OK with this? It should more or less match the feedback you gave me. 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.
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 Created attachment 246358 [details]
Patch
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.
Comment on attachment 246358 [details] Patch Clearing flags on attachment: 246358 Committed r179910: <http://trac.webkit.org/changeset/179910> All reviewed patches have been landed. Closing bug. 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 Re-opened since this is blocked by bug 141464 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. > 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.
(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. Created attachment 246403 [details]
Patch
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.
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. Committed r179972: <http://trac.webkit.org/changeset/179972> (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. |