Add a facility for tracking and reporting page and resource loads. <rdar://problem/36548974>
Created attachment 338673 [details] Patch
Created attachment 338681 [details] Patch
Created attachment 338768 [details] Patch
Created attachment 338814 [details] Rebase to fix merge conflicts.
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?
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!
(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.
(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.
> #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().
Created attachment 339164 [details] Patch
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.
(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.
Created attachment 339351 [details] Patch
Comment on attachment 339351 [details] Patch Looks great! Thanks for finishing this up.
Comment on attachment 339351 [details] Patch Clearing flags on attachment: 339351 Committed r231282: <https://trac.webkit.org/changeset/231282>
All reviewed patches have been landed. Closing bug.
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).