Bug 224194 – Add additional page load diagnostic logging

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

Add additional logging to help identify places where page loads fail to complete.

[Radar WebKit Bug Importer]

<rdar://problem/76227175>

[Brent Fulgham]

Created attachment 425182 [details]
Patch

[Kate Cheney]

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.

[Brent Fulgham]

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).

[Brent Fulgham]

Created attachment 425217 [details]
Patch

[Kate Cheney]

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!

[Brent Fulgham]

Created attachment 425222 [details]
Patch

[Brent Fulgham]

The test failure is not related to my patch.

[Alex Christensen]

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?

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 425329 [details]
Patch for landing

[EWS]

Committed r275563: <https://commits.webkit.org/r275563>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425329 [details].