Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure we don't leak API::Navigation objects #14023

Merged
merged 1 commit into from
May 24, 2023

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented May 18, 2023

9ae6f74

Make sure we don't leak API::Navigation objects
https://bugs.webkit.org/show_bug.cgi?id=256900
rdar://109430054

Reviewed by Ben Nham.

The lifetime of the API::Navigation objects in the UIProcess is tied to the one
of the WebDocumentLoaders in the WebProcess. The API::Navigation object get
stored on the WebNavigationState::m_navigations HashMap upon creation, keeping
them alive. When the WebDocumentLoader gets detached from its frame, we send a
`DidDestroyNavigation()` IPC to the UIProcess so that we remove the
corresponding navigation from WebNavigationState::m_navigations and stop
extending the API::Navigation lifetime.

The reason the lifetime of navigations is tied to WebDocumentLoaders is that
the original API::Navigation object is reused for some follow-up navigations,
such as same-document navigations.

Since we've added support for process-swapping, this has become leak-prone.
The reason is that a WebPageProxy (and thus a WebNavigationState) is no longer
tied to a single WebProcess. Also, A navigation may start in process A and
then finish in process B.

To avoid leaking, I am adding a ProcessIdentifier data member to API navigation
which is kept up-to-date so that we know which WebProcess currently owns this
object. When a process gets disassociated with a page, we also clear all the
page's navigations that are owned by this process.

* Source/WebKit/UIProcess/API/APINavigation.cpp:
(API::Navigation::Navigation):
* Source/WebKit/UIProcess/API/APINavigation.h:
(API::Navigation::create):
(API::Navigation::processID const):
(API::Navigation::setProcessID):
* Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
(WebKit::ProvisionalPageProxy::didDestroyNavigation):
(WebKit::ProvisionalPageProxy::didReceiveMessage):
* Source/WebKit/UIProcess/ProvisionalPageProxy.h:
* Source/WebKit/UIProcess/SuspendedPageProxy.cpp:
(WebKit::messageNamesToIgnoreWhileSuspended):
(WebKit::SuspendedPageProxy::SuspendedPageProxy):
(WebKit::SuspendedPageProxy::~SuspendedPageProxy):
(WebKit::SuspendedPageProxy::didDestroyNavigation):
(WebKit::SuspendedPageProxy::didReceiveMessage):
* Source/WebKit/UIProcess/SuspendedPageProxy.h:
* Source/WebKit/UIProcess/WebNavigationState.cpp:
(WebKit::WebNavigationState::createLoadRequestNavigation):
(WebKit::WebNavigationState::createBackForwardNavigation):
(WebKit::WebNavigationState::createReloadNavigation):
(WebKit::WebNavigationState::createLoadDataNavigation):
(WebKit::WebNavigationState::createSimulatedLoadWithDataNavigation):
(WebKit::WebNavigationState::didDestroyNavigation):
(WebKit::WebNavigationState::clearNavigationsFromProcess):
* Source/WebKit/UIProcess/WebNavigationState.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::launchProcessForReload):
(WebKit::WebPageProxy::loadRequest):
(WebKit::WebPageProxy::loadFile):
(WebKit::WebPageProxy::loadData):
(WebKit::WebPageProxy::loadSimulatedRequest):
(WebKit::WebPageProxy::reload):
(WebKit::WebPageProxy::goToBackForwardItem):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
(WebKit::WebPageProxy::didDestroyNavigation):
(WebKit::WebPageProxy::didDestroyNavigationShared):
(WebKit::WebPageProxy::didSameDocumentNavigationForFrameViaJSHistoryAPI):
(WebKit::WebPageProxy::processIsNoLongerAssociatedWithPage):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::setIsInProcessCache):
(WebKit::WebProcessProxy::removeProvisionalPageProxy):
(WebKit::WebProcessProxy::removeWebPage):
(WebKit::WebProcessProxy::canTerminateAuxiliaryProcess):
(WebKit::WebProcessProxy::addSuspendedPageProxy):
(WebKit::WebProcessProxy::removeSuspendedPageProxy):
(WebKit::WebProcessProxy::reportProcessDisassociatedWithPageIfNecessary):
(WebKit::WebProcessProxy::isAssociatedWithPage const):
(WebKit::WebProcessProxy::incrementSuspendedPageCount): Deleted.
(WebKit::WebProcessProxy::decrementSuspendedPageCount): Deleted.
* Source/WebKit/UIProcess/WebProcessProxy.h:
(WebKit::WebProcessProxy::suspendedPageCount const):
* Source/WebKit/WebProcess/WebPage/WebDocumentLoader.cpp:
(WebKit::WebDocumentLoader::WebDocumentLoader):
(WebKit::WebDocumentLoader::~WebDocumentLoader):
* Source/WebKit/WebProcess/WebPage/WebDocumentLoader.h:

Canonical link: https://commits.webkit.org/264449@main

440e7cf

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@cdumez cdumez self-assigned this May 18, 2023
@cdumez cdumez added the WebKit2 Bugs relating to the WebKit2 API layer label May 18, 2023
@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from a6af749 to 83fb25f Compare May 18, 2023 18:34
@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from 83fb25f to 267f9e8 Compare May 18, 2023 19:25
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 19, 2023
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label May 19, 2023
@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from 267f9e8 to e3b2632 Compare May 19, 2023 02:17
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 19, 2023
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label May 19, 2023
@cdumez cdumez marked this pull request as ready for review May 19, 2023 14:31
@cdumez cdumez requested a review from bnham May 19, 2023 14:31
Copy link
Contributor

@achristensen07 achristensen07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test. An autoreleasepool and a weak objc pointer should be able to show the change.


// On process-swap, the previous process tries to destroy the navigation but the provisional process is actually taking over the navigation.
if (m_provisionalPage && m_provisionalPage->navigationID() == navigationID)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the previous hack we use to have here to ignore the DidDestroyNavigation IPC from the old process when we swap.

Copy link
Contributor

@bnham bnham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, approved pending the test case being written.

@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from e3b2632 to b950e09 Compare May 19, 2023 19:14
@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from b950e09 to b638c48 Compare May 19, 2023 19:15
@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from b638c48 to e438fc7 Compare May 22, 2023 17:09
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 22, 2023
@cdumez
Copy link
Contributor Author

cdumez commented May 22, 2023

The API tests passes locally but fails on EWS, I am not sure why. This is my second version of the API test and it still fails :/
At this point, I may need to introduce an SPI that exposes the number of Navigation objects in the WebNavigationState HashMap instead of relying on the WKNavigation object getting destroyed.

@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label May 23, 2023
@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from e438fc7 to f82e6d2 Compare May 23, 2023 02:27
@cdumez
Copy link
Contributor Author

cdumez commented May 23, 2023

I think I finally found why the API test is failing on EWS. I'll get it fixed shortly.

@cdumez cdumez force-pushed the 256900_API_Navigation_leak branch from f82e6d2 to 440e7cf Compare May 23, 2023 20:18
@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label May 24, 2023
https://bugs.webkit.org/show_bug.cgi?id=256900
rdar://109430054

Reviewed by Ben Nham.

The lifetime of the API::Navigation objects in the UIProcess is tied to the one
of the WebDocumentLoaders in the WebProcess. The API::Navigation object get
stored on the WebNavigationState::m_navigations HashMap upon creation, keeping
them alive. When the WebDocumentLoader gets detached from its frame, we send a
`DidDestroyNavigation()` IPC to the UIProcess so that we remove the
corresponding navigation from WebNavigationState::m_navigations and stop
extending the API::Navigation lifetime.

The reason the lifetime of navigations is tied to WebDocumentLoaders is that
the original API::Navigation object is reused for some follow-up navigations,
such as same-document navigations.

Since we've added support for process-swapping, this has become leak-prone.
The reason is that a WebPageProxy (and thus a WebNavigationState) is no longer
tied to a single WebProcess. Also, A navigation may start in process A and
then finish in process B.

To avoid leaking, I am adding a ProcessIdentifier data member to API navigation
which is kept up-to-date so that we know which WebProcess currently owns this
object. When a process gets disassociated with a page, we also clear all the
page's navigations that are owned by this process.

* Source/WebKit/UIProcess/API/APINavigation.cpp:
(API::Navigation::Navigation):
* Source/WebKit/UIProcess/API/APINavigation.h:
(API::Navigation::create):
(API::Navigation::processID const):
(API::Navigation::setProcessID):
* Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
(WebKit::ProvisionalPageProxy::didDestroyNavigation):
(WebKit::ProvisionalPageProxy::didReceiveMessage):
* Source/WebKit/UIProcess/ProvisionalPageProxy.h:
* Source/WebKit/UIProcess/SuspendedPageProxy.cpp:
(WebKit::messageNamesToIgnoreWhileSuspended):
(WebKit::SuspendedPageProxy::SuspendedPageProxy):
(WebKit::SuspendedPageProxy::~SuspendedPageProxy):
(WebKit::SuspendedPageProxy::didDestroyNavigation):
(WebKit::SuspendedPageProxy::didReceiveMessage):
* Source/WebKit/UIProcess/SuspendedPageProxy.h:
* Source/WebKit/UIProcess/WebNavigationState.cpp:
(WebKit::WebNavigationState::createLoadRequestNavigation):
(WebKit::WebNavigationState::createBackForwardNavigation):
(WebKit::WebNavigationState::createReloadNavigation):
(WebKit::WebNavigationState::createLoadDataNavigation):
(WebKit::WebNavigationState::createSimulatedLoadWithDataNavigation):
(WebKit::WebNavigationState::didDestroyNavigation):
(WebKit::WebNavigationState::clearNavigationsFromProcess):
* Source/WebKit/UIProcess/WebNavigationState.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::launchProcessForReload):
(WebKit::WebPageProxy::loadRequest):
(WebKit::WebPageProxy::loadFile):
(WebKit::WebPageProxy::loadData):
(WebKit::WebPageProxy::loadSimulatedRequest):
(WebKit::WebPageProxy::reload):
(WebKit::WebPageProxy::goToBackForwardItem):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
(WebKit::WebPageProxy::didDestroyNavigation):
(WebKit::WebPageProxy::didDestroyNavigationShared):
(WebKit::WebPageProxy::didSameDocumentNavigationForFrameViaJSHistoryAPI):
(WebKit::WebPageProxy::processIsNoLongerAssociatedWithPage):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::setIsInProcessCache):
(WebKit::WebProcessProxy::removeProvisionalPageProxy):
(WebKit::WebProcessProxy::removeWebPage):
(WebKit::WebProcessProxy::canTerminateAuxiliaryProcess):
(WebKit::WebProcessProxy::addSuspendedPageProxy):
(WebKit::WebProcessProxy::removeSuspendedPageProxy):
(WebKit::WebProcessProxy::reportProcessDisassociatedWithPageIfNecessary):
(WebKit::WebProcessProxy::isAssociatedWithPage const):
(WebKit::WebProcessProxy::incrementSuspendedPageCount): Deleted.
(WebKit::WebProcessProxy::decrementSuspendedPageCount): Deleted.
* Source/WebKit/UIProcess/WebProcessProxy.h:
(WebKit::WebProcessProxy::suspendedPageCount const):
* Source/WebKit/WebProcess/WebPage/WebDocumentLoader.cpp:
(WebKit::WebDocumentLoader::WebDocumentLoader):
(WebKit::WebDocumentLoader::~WebDocumentLoader):
* Source/WebKit/WebProcess/WebPage/WebDocumentLoader.h:

Canonical link: https://commits.webkit.org/264449@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264449@main (9ae6f74): https://commits.webkit.org/264449@main

Reviewed commits have been landed. Closing PR #14023 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 9ae6f74 into WebKit:main May 24, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit2 Bugs relating to the WebKit2 API layer
Projects
None yet
6 participants