Bug 184838 – Add facility for tracking times and results of page and resource loading

• Component: Page Loading
• Status: RESOLVED FIXED

[Keith Rollin]

Add a facility for tracking and reporting page and resource loads.

<rdar://problem/36548974>

[Keith Rollin]

Created attachment 338673 [details]
Patch

[Keith Rollin]

Created attachment 338681 [details]
Patch

[Keith Rollin]

Created attachment 338768 [details]
Patch

[Keith Rollin]

Created attachment 338814 [details]
Rebase to fix merge conflicts.

[Brent Fulgham]

Comment on attachment 338814 [details]
Rebase to fix merge conflicts.

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

Looks good! r- to fix the iterator stuff and clean up the struct a little. Nice job!

> Source/WebKit/Configurations/WebKit.xcconfig:115
> +FRAMEWORK_AND_LIBRARY_LDFLAGS = -lobjc -framework CFNetwork -framework CoreAudio -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework IOKit -framework WebKitLegacy -lnetwork $(WK_ACCESSIBILITY_LDFLAGS) $(WK_APPKIT_LDFLAGS) $(WK_ASSERTION_SERVICES_LDFLAGS) $(WK_CARBON_LDFLAGS) $(WK_CORE_PDF_LDFLAGS) $(WK_CORE_PREDICTION_LDFLAGS) $(WK_CORE_SERVICES_LDFLAGS) $(WK_GRAPHICS_SERVICES_LDFLAGS) $(WK_IOSURFACE_LDFLAGS) $(WK_LIBWEBRTC_LDFLAGS) $(WK_MOBILE_CORE_SERVICES_LDFLAGS) $(WK_MOBILE_GESTALT_LDFLAGS) $(WK_OPENGL_LDFLAGS) $(WK_PDFKIT_LDFLAGS) $(WK_SAFE_BROWSING_LDFLAGS) $(WK_UIKIT_LDFLAGS);

Is it safe to add "-lnetwork" unconditionally? Based on EWS I assume it's fine.

> Source/WebKit/NetworkProcess/NetworkActivityTracker.h:73
> +    nw_activity_t get() { return m_networkActivity.get(); }

I wonder if Soup or Curl has any similar network health features? If so, we could make this a 'PlatformActivity' type that would vary based on platform. But that's not something we need for this patch.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:586
> +            return std::nullopt;

If the network process crashed, shouldn't we create a new activity tracker so that we can at least capture whatever data comes in for the remaining loads?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:630
> +        m_networkActivityTrackers[itemIndex].m_networkActivity.complete(NetworkActivityTracker::CompletionCode::None);

This would be better as:

for (auto& activityTracker : m_networkActivityTrackers)
    activityTracker.m_networkActivity.complete(...)

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:644
> +            m_networkActivityTrackers.remove(itemIndex);

I really hate this kind of iterating over a container while removing elements. We have had so many security and stability bugs due to this kind of stuff.

Instead, I propose:

for (auto& activityTracker : m_networkActivityTrackers) {
    if (activityTracker.m_pageID == pageID)
        acitivityTracker.m_networkActivity.complete(NetworkActivityTracker::CompletionCode::None);
}

m_networkActivityTrackers.removeAllMatching([&](const auto& activityTracker) { return activityTracker.pageID == pageID; });

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:654
> +    }

return m_networkActivityTrackers.findMatching([&](const auto& item) { return item.m_isRootActivity && item.m_pageID == pageID; });

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:664
> +    }

return m_networkActivityTrackers.findMatching([&](const auto& item) { return item.m_resourceID == resourceID; });

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:211
> +        NetworkActivityTracker m_networkActivity;

Since this is a struct and doesn't encapsulate these members behind accessors, they should be renamed without the "m_*" prefix.

> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:28
> +#include "NetworkActivityTracker.h"

Could this just be a forward declaration?

[Alex Christensen]

Comment on attachment 338814 [details]
Rebase to fix merge conflicts.

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

> Source/WTF/wtf/Platform.h:1274
> +#if USE(APPLE_INTERNAL_SDK) && __has_include(<nw/activity.h>)

Will this make aI think this will make each file check for the existence of <nw/activity.h> which could slow down the build a bit.  Is there a better way to describe when we expect it to be available?

> Source/WebCore/loader/FrameLoader.cpp:263
>      Frame& m_frame;
> +    FrameLoaderClient& m_client;

We shouldn't need to keep both of these.  We can access the FrameLoaderClient through m_frame.loader().client()

> Source/WebKit/NetworkProcess/NetworkActivityTracker.cpp:2
> + * Copyright (C) 22018 Apple Inc. All rights reserved.

ditto

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:194
> +            , m_resourceID(0)

Please use c++11-style "resourceID { 0 };" declaration instead of this.

> Source/WebKit/NetworkProcess/cocoa/NetworkActivityTrackerCocoa.mm:2
> + * Copyright (C) 22018 Apple Inc. All rights reserved.

Wow, that's planning ahead!

[Keith Rollin]

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 338814 [details]
> Rebase to fix merge conflicts.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338814&action=review
> 
> Looks good! r- to fix the iterator stuff and clean up the struct a little.
> Nice job!
> 
> > Source/WebKit/Configurations/WebKit.xcconfig:115
> > +FRAMEWORK_AND_LIBRARY_LDFLAGS = -lobjc -framework CFNetwork -framework CoreAudio -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework IOKit -framework WebKitLegacy -lnetwork $(WK_ACCESSIBILITY_LDFLAGS) $(WK_APPKIT_LDFLAGS) $(WK_ASSERTION_SERVICES_LDFLAGS) $(WK_CARBON_LDFLAGS) $(WK_CORE_PDF_LDFLAGS) $(WK_CORE_PREDICTION_LDFLAGS) $(WK_CORE_SERVICES_LDFLAGS) $(WK_GRAPHICS_SERVICES_LDFLAGS) $(WK_IOSURFACE_LDFLAGS) $(WK_LIBWEBRTC_LDFLAGS) $(WK_MOBILE_CORE_SERVICES_LDFLAGS) $(WK_MOBILE_GESTALT_LDFLAGS) $(WK_OPENGL_LDFLAGS) $(WK_PDFKIT_LDFLAGS) $(WK_SAFE_BROWSING_LDFLAGS) $(WK_UIKIT_LDFLAGS);
> 
> Is it safe to add "-lnetwork" unconditionally? Based on EWS I assume it's
> fine.

I've built locally under multiple versions of macOS and iOS in both debug and release modes, and it succeeds.

> > Source/WebKit/NetworkProcess/NetworkActivityTracker.h:73
> > +    nw_activity_t get() { return m_networkActivity.get(); }
> 
> I wonder if Soup or Curl has any similar network health features? If so, we
> could make this a 'PlatformActivity' type that would vary based on platform.
> But that's not something we need for this patch.

I strove to structure the code to support this mechanism on other platforms if people wanted to figure out how such support should be implemented. There's a NetworkActivityTracker object which comes with a dummy/empty implementation for non-Cocoa and a concrete implementation for Cocoa. The non-Cocoa implementation could be replaced with a platform-specific implementation for Windows or Linux if wanted. Did I not express my intent the "WebKit Way"?

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:586
> > +            return std::nullopt;
> 
> If the network process crashed, shouldn't we create a new activity tracker
> so that we can at least capture whatever data comes in for the remaining
> loads?

The problem with trying to do that would be with re-establishing the parent NetworkActivityTracer that represents the page. The cost/benefit ratio seems high to me. How strongly do you want this?

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:630
> > +        m_networkActivityTrackers[itemIndex].m_networkActivity.complete(NetworkActivityTracker::CompletionCode::None);
> 
> This would be better as:
> 
> for (auto& activityTracker : m_networkActivityTrackers)
>     activityTracker.m_networkActivity.complete(...)

You're right. I'm not sure why I did it that way.

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:644
> > +            m_networkActivityTrackers.remove(itemIndex);
> 
> I really hate this kind of iterating over a container while removing
> elements. We have had so many security and stability bugs due to this kind
> of stuff.
> 
> Instead, I propose:
> 
> for (auto& activityTracker : m_networkActivityTrackers) {
>     if (activityTracker.m_pageID == pageID)
>        
> acitivityTracker.m_networkActivity.complete(NetworkActivityTracker::
> CompletionCode::None);
> }
> 
> m_networkActivityTrackers.removeAllMatching([&](const auto& activityTracker)
> { return activityTracker.pageID == pageID; });

OK. But I'd like to later talk with you about what kinds of problems occur, so that I can be better aware of why to avoid this anti-pattern.

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:654
> > +    }
> 
> return m_networkActivityTrackers.findMatching([&](const auto& item) { return
> item.m_isRootActivity && item.m_pageID == pageID; });

OK. I didn't realize we had a Vector method that returned an index as opposed to an iterator.

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:664
> > +    }
> 
> return m_networkActivityTrackers.findMatching([&](const auto& item) { return
> item.m_resourceID == resourceID; });

Ditto.

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:211
> > +        NetworkActivityTracker m_networkActivity;
> 
> Since this is a struct and doesn't encapsulate these members behind
> accessors, they should be renamed without the "m_*" prefix.

Very well. I'd checked the WebKit Code Style Guidelines when working on this, and it didn't say anything about structs being treated differently from class's in this respect. But I can go along with this unwritten rule.

> > Source/WebKit/NetworkProcess/NetworkLoadParameters.h:28
> > +#include "NetworkActivityTracker.h"
> 
> Could this just be a forward declaration?

Compiler says no. In the bowels of the error stream is the message "incomplete type 'WebKit::NetworkActivityTracker' used in type trait expression", related to a file that pulls in NetworkLoadParameters.h but that has nothing to do with the "std::optional<NetworkActivityTracker> networkActivityTracker" member. So if we were to forward-declare NetworkActivityTracker, we'd have to #include "NetworkActivityTracker.h in any file that instantiated NetworkLoadParameters.

[Keith Rollin]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 338814 [details]
> Rebase to fix merge conflicts.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338814&action=review
> 
> > Source/WTF/wtf/Platform.h:1274
> > +#if USE(APPLE_INTERNAL_SDK) && __has_include(<nw/activity.h>)
> 
> ...I think this will make each file check for the existence of
> <nw/activity.h> which could slow down the build a bit.  Is there a better
> way to describe when we expect it to be available?

In response to your comment, I timed some builds with and without that __has_include() check. There was no difference in build times (42 min, 1 sec +/- 1 sec).

That said, I reconsidered the placement of those #defines. They have pretty limited scope and don't really benefit from being in such a core #include file. I therefore looked into putting them into NetworkActivityTracker.h. I checked elsewhere in WebKit to see if there are precedents for HAVE_foo and USE_foo variables being defined outside of Platform.h, Feature.h, or config.h files, and there were a small number of them. I took them as examples to follow and went ahead and moved the variables out of wtf/Platform.h.

> > Source/WebCore/loader/FrameLoader.cpp:263
> >      Frame& m_frame;
> > +    FrameLoaderClient& m_client;
> 
> We shouldn't need to keep both of these.  We can access the
> FrameLoaderClient through m_frame.loader().client()

Thanks.

> > Source/WebKit/NetworkProcess/NetworkActivityTracker.cpp:2
> > + * Copyright (C) 22018 Apple Inc. All rights reserved.
> 
> ditto

ditto

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:194
> > +            , m_resourceID(0)
> 
> Please use c++11-style "resourceID { 0 };" declaration instead of this.

OK.

> > Source/WebKit/NetworkProcess/cocoa/NetworkActivityTrackerCocoa.mm:2
> > + * Copyright (C) 22018 Apple Inc. All rights reserved.
> 
> Wow, that's planning ahead!

Oops.

[Keith Rollin]

> #if HAVE(NW_ACTIVITY)
>    nw_activity_t get() { return m_networkActivity.get(); }
> #endif

I found that get() was too generic and confusing -- it wasn't clear what I was getting. I first tried changing it to get_nw_activity(), but that didn't match WebKit naming guidelines. I thought about getNWActivity(), but that seemed to be perverting the underlying type name. So I changed it to getPlatformObject().

[Keith Rollin]

Created attachment 339164 [details]
Patch

[Brent Fulgham]

Comment on attachment 339164 [details]
Patch

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

Looks great! I had a few things I'd like to see changed slightly, but overall this is ready to go. r- to make the m_parameters member const again.

> Source/WebCore/ChangeLog:5
> +        rdar://problem/36548974

<rdar://problem/36548974>

> Source/WebKit/ChangeLog:5
> +        rdar://problem/36548974

<rdar://problem/36548974>

> Source/WebKitLegacy/ChangeLog:5
> +        rdar://problem/36548974

<rdar://problem/36548974>

> Source/WebCore/loader/FrameLoader.cpp:257
> +            platformStrategies()->loaderStrategy()->pageLoadCompleted(m_frame.loader().client().pageID().value());

if (auto pageID = m_frame.loader().client().pageID())
    platformStrategies()->loaderStrategy()->pageLoadCompleted(pageID.value());

> Source/WebKit/NetworkProcess/NetworkActivityTracker.h:79
> +#if HAVE(NW_ACTIVITY)

We could probably just use "#if HAVE(NW_ACTIVITY)" to protect m_networkActivity, since the other items are likely useful for other ports (if they wanted to implement some kind of network activity tracker for their network backends). But this is fine as-is.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:551
> +#if HAVE(NW_ACTIVITY)

I don't think you need this. All other ports will return 'false' from NetworkProcess::trackNetworkActivity(), so just make this call on all ports.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:168
> +    m_parameters.networkActivityTracker = m_connection->startTrackingResourceLoad(m_parameters.webPageID, m_parameters.identifier, isMainResource());

I don't think we should change m_parameters from const. Instead, I think you should track m_hasNetworkActivityTracker (or something) as a separate member (like we do for m_defersLoading), and set 'networkActivityTracker' in the copy of m_parameters we send to the NetworkLoad object in NetworkResourceLoader::startNetworkLoad.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:162
> +    NetworkResourceLoadParameters m_parameters;

I don't think we should change this.

[Keith Rollin]

(In reply to Brent Fulgham from comment #11)
> Comment on attachment 339164 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339164&action=review
> 
> Looks great! I had a few things I'd like to see changed slightly, but
> overall this is ready to go. r- to make the m_parameters member const again.
> 
> > Source/WebCore/ChangeLog:5
> > +        rdar://problem/36548974
> 
> <rdar://problem/36548974>
> 
> > Source/WebKit/ChangeLog:5
> > +        rdar://problem/36548974
> 
> <rdar://problem/36548974>
> 
> > Source/WebKitLegacy/ChangeLog:5
> > +        rdar://problem/36548974
> 
> <rdar://problem/36548974>

OK. But I see them both ways in the ChangeLog files.

> > Source/WebCore/loader/FrameLoader.cpp:257
> > +            platformStrategies()->loaderStrategy()->pageLoadCompleted(m_frame.loader().client().pageID().value());
> 
> if (auto pageID = m_frame.loader().client().pageID())
>    
> platformStrategies()->loaderStrategy()->pageLoadCompleted(pageID.value());

OK.

> > Source/WebKit/NetworkProcess/NetworkActivityTracker.h:79
> > +#if HAVE(NW_ACTIVITY)
> 
> We could probably just use "#if HAVE(NW_ACTIVITY)" to protect
> m_networkActivity, since the other items are likely useful for other ports
> (if they wanted to implement some kind of network activity tracker for their
> network backends). But this is fine as-is.

I agree that they are likely useful for other ports, but we get "unused data members" errors if we don't elide them.

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:551
> > +#if HAVE(NW_ACTIVITY)
> 
> I don't think you need this. All other ports will return 'false' from
> NetworkProcess::trackNetworkActivity(), so just make this call on all ports.

OK.

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:168
> > +    m_parameters.networkActivityTracker = m_connection->startTrackingResourceLoad(m_parameters.webPageID, m_parameters.identifier, isMainResource());
> 
> I don't think we should change m_parameters from const. Instead, I think you
> should track m_hasNetworkActivityTracker (or something) as a separate member
> (like we do for m_defersLoading), and set 'networkActivityTracker' in the
> copy of m_parameters we send to the NetworkLoad object in
> NetworkResourceLoader::startNetworkLoad.

Good point.

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.h:162
> > +    NetworkResourceLoadParameters m_parameters;
> 
> I don't think we should change this.

OK.

[Keith Rollin]

Created attachment 339351 [details]
Patch

[Brent Fulgham]

Comment on attachment 339351 [details]
Patch

Looks great! Thanks for finishing this up.

[WebKit Commit Bot]

Comment on attachment 339351 [details]
Patch

Clearing flags on attachment: 339351

Committed r231282: <https://trac.webkit.org/changeset/231282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 339351 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:301
> +    m_trackNetworkActivity = parameters.trackNetworkActivity;

For these things, I believe the WebKit coding style mandates a prefix since those are booleans. In this case, 'should' would look much better.

> Source/WebKit/NetworkProcess/NetworkProcess.h:165
> +    bool trackNetworkActivity() const { return m_trackNetworkActivity; }

ditto.

> Source/WebKit/NetworkProcess/NetworkProcess.h:306
> +    bool m_trackNetworkActivity { false };

ditto.

> Source/WebKit/NetworkProcess/NetworkProcessCreationParameters.cpp:119
> +    encoder << trackNetworkActivity;

ditto.

> Source/WebKit/NetworkProcess/NetworkProcessCreationParameters.h:136
> +    bool trackNetworkActivity { false };

ditto.

> Source/WebKit/UIProcess/API/C/WKContextConfigurationRef.h:80
> +WK_EXPORT bool WKContextConfigurationTrackNetworkActivity(WKContextConfigurationRef configuration);

Ditto here about API naming, this seems weird to me. WKContextConfigurationTrackNetworkActivity() does not look like a better at all.

> Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:63
> +@property (nonatomic) BOOL trackNetworkActivity WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Other people I added in CC are more familiar with API naming than I am but this seems weird to me. I personally would have used a prefix like shouldTrackNetworkActivity (or alternatively tracksNetworkActivity which I like less but seems consistent with 'processSwapsOnNavigation' which Brady just added).