Add additional logging to help identify places where page loads fail to complete.
<rdar://problem/76227175>
Created attachment 425182 [details] Patch
Comment on attachment 425182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425182&action=review Yay, more logging! > Source/WebCore/loader/ResourceLoader.cpp:130 > + RELEASE_LOG_IF_ALLOWED("init: Cancelling because the document loader has no frame."); I think this should be "releaseResources: ..." instead of "init: ... ". > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:562 > + RELEASE_LOG_IF_ALLOWED(Loading, "removeLoadIdentifier %" PRIu64 " and aborting corresponding loader", identifier); This message format diverges from the typical format of <functionName><colon><message>. Maybe it could be refactored to align more? > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6728 > + RELEASE_LOG_IF_ALLOWED(Process, "WebPage: Registered handler %" PRIu64 " for the '%s' scheme", handlerIdentifier, scheme..utf8().data()); I think WebPage: is redundant because RELEASE_LOG_IF_ALLOWED should already include it. Should this be "registerURLSchemeHandler: ..." instead? > Source/WebKit/WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:109 > + WEBURLSCHEMETASKPROXY_RELEASE_LOG_IF_ALLOWED(Loading, "Redirected scheme task would have been sent to a different URL."); Ditto about format, do we want this to say "didPerformRedirection: Redirected scheme task would have been sent to a different URL"? > Source/WebKit/WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:115 > + WEBURLSCHEMETASKPROXY_RELEASE_LOG_IF_ALLOWED(Loading, "Received redirect during previous redirect processing, queuing it."); Ditto re: format. > Source/WebKit/WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:129 > + WEBURLSCHEMETASKPROXY_RELEASE_LOG_IF_ALLOWED(Loading, "Received response during redirect processing, queuing it."); Ditto. > Source/WebKit/WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:152 > + WEBURLSCHEMETASKPROXY_RELEASE_LOG_IF_ALLOWED(Loading, "Received data during response processing, queuing it."); Ditto.
Comment on attachment 425182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425182&action=review >> Source/WebCore/loader/ResourceLoader.cpp:130 >> + RELEASE_LOG_IF_ALLOWED("init: Cancelling because the document loader has no frame."); > > I think this should be "releaseResources: ..." instead of "init: ... ". I'm trying to capture the reason the 'init' method is failing to create a loader, and is triggering a cancel and complete(false) -- so I think this is the right label. >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:562 >> + RELEASE_LOG_IF_ALLOWED(Loading, "removeLoadIdentifier %" PRIu64 " and aborting corresponding loader", identifier); > > This message format diverges from the typical format of <functionName><colon><message>. Maybe it could be refactored to align more? Will do. >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6728 >> + RELEASE_LOG_IF_ALLOWED(Process, "WebPage: Registered handler %" PRIu64 " for the '%s' scheme", handlerIdentifier, scheme..utf8().data()); > > I think WebPage: is redundant because RELEASE_LOG_IF_ALLOWED should already include it. Should this be "registerURLSchemeHandler: ..." instead? Oh, yes! You are right. >> Source/WebKit/WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:109 >> + WEBURLSCHEMETASKPROXY_RELEASE_LOG_IF_ALLOWED(Loading, "Redirected scheme task would have been sent to a different URL."); > > Ditto about format, do we want this to say "didPerformRedirection: Redirected scheme task would have been sent to a different URL"? Yep (and same to below).
Created attachment 425217 [details] Patch
Comment on attachment 425182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425182&action=review >>> Source/WebCore/loader/ResourceLoader.cpp:130 >>> + RELEASE_LOG_IF_ALLOWED("init: Cancelling because the document loader has no frame."); >> >> I think this should be "releaseResources: ..." instead of "init: ... ". > > I'm trying to capture the reason the 'init' method is failing to create a loader, and is triggering a cancel and complete(false) -- so I think this is the right label. Oops, I misread the diff. You're right!
Created attachment 425222 [details] Patch
The test failure is not related to my patch.
Comment on attachment 425222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425222&action=review > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:364 > + RELEASE_LOG_IF_ALLOWED(Loading, "didClose: WebProcess (%d) closed its connection. Aborting related loaders.", connection.remoteProcessID()); Cool! Do we want a log in the web process if the network process closes its connection?
(In reply to Alex Christensen from comment #9) > Comment on attachment 425222 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425222&action=review > > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:364 > > + RELEASE_LOG_IF_ALLOWED(Loading, "didClose: WebProcess (%d) closed its connection. Aborting related loaders.", connection.remoteProcessID()); > > Cool! Do we want a log in the web process if the network process closes its > connection? Yes -- that's a great idea. I'll add that.
Created attachment 425329 [details] Patch for landing
Committed r275563: <https://commits.webkit.org/r275563> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425329 [details].