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

Prepare to allow parent frames to navigate site isolated iframes #13785

Conversation

achristensen07
Copy link
Contributor

@achristensen07 achristensen07 commented May 12, 2023

65e5a93

Prepare to allow parent frames to navigate site isolated iframes
https://bugs.webkit.org/show_bug.cgi?id=256678
rdar://109235337

Reviewed by Brady Eidson.

This PR does a few things, all of which will not change behavior with site isolation off.

First, it moves NavigationScheduler from LocalFrame to Frame.  This is needed because
if a parent frame sets iframe.src=newURL then the parent's process needs to schedule the
navigation of the iframe.  This is the first of several large steps to get the navigation
to happen.  The next steps are to make a FrameLoader able to be owned by either a LocalFrame
or a RemoteFrame and able to be moved between the two as the navigation causes processes
to change.  I got the NavigationScheduler to the point where it doesn't assert in the API
test ParentNavigatingCrossOriginIframeToSameOrigin when site isolation is enabled, though
the navigation doesn't proceed yet because there is no FrameLoader on the RemoteFrame.

In order to make that compile, I needed to make some InspectorInstrumentation parameters
be Frame instead of LocalFrame.

I also made the source process in WebPageProxy::decidePolicyForNavigationAction be the
WebFrameProxy's current process instead of the process from which the decidePolicyForNavigationAction
message came.  In all tests that are running for site isolation so far, they are the same.

I add some assertions in the WebFrame constructor and invalidator that helped me figure out what
was going wrong when debugging this.  We had some code that was making 2 WebFrames with the same
FrameIdentifier.  This will help future developers catch problems earlier.

I also change the alert in the API test ParentNavigatingCrossOriginIframeToSameOrigin to be
called in the onload handler.  This will allow my frame tree validation code to have deterministic
state once I implement the frame teardown in the previoiusly-used process.  If it weren't in the
onload handler, then there was a race condition in the test because the alert could come before
the didCommitLoadForFrame was processed.

I also move the boolean values initialized in the FrameLoader constructor to use initializers in the header.

* Source/WebCore/inspector/InspectorInstrumentation.cpp:
(WebCore::InspectorInstrumentation::frameScheduledNavigationImpl):
(WebCore::InspectorInstrumentation::frameClearedScheduledNavigationImpl):
* Source/WebCore/inspector/InspectorInstrumentation.h:
(WebCore::InspectorInstrumentation::frameScheduledNavigation):
(WebCore::InspectorInstrumentation::frameClearedScheduledNavigation):
(WebCore::InspectorInstrumentation::instrumentingAgents):
* Source/WebCore/inspector/agents/InspectorPageAgent.cpp:
(WebCore::InspectorPageAgent::frameDetached):
(WebCore::InspectorPageAgent::frameForId):
(WebCore::InspectorPageAgent::frameId):
(WebCore::InspectorPageAgent::assertFrame):
(WebCore::InspectorPageAgent::frameScheduledNavigation):
(WebCore::InspectorPageAgent::frameClearedScheduledNavigation):
* Source/WebCore/inspector/agents/InspectorPageAgent.h:
* Source/WebCore/loader/NavigationDisabler.h:
(WebCore::NavigationDisabler::isNavigationAllowed):
* Source/WebCore/loader/NavigationScheduler.cpp:
(WebCore::NavigationScheduler::NavigationScheduler):
(WebCore::NavigationScheduler::scheduleRedirect):
(WebCore::NavigationScheduler::mustLockBackForwardList):
(WebCore::NavigationScheduler::scheduleLocationChange):
(WebCore::NavigationScheduler::scheduleFormSubmission):
(WebCore::NavigationScheduler::scheduleRefresh):
(WebCore::NavigationScheduler::timerFired):
(WebCore::NavigationScheduler::schedule):
(WebCore::NavigationScheduler::startTimer):
(WebCore::NavigationScheduler::cancel):
* Source/WebCore/loader/NavigationScheduler.h:
* Source/WebCore/loader/SubframeLoader.cpp:
(WebCore::FrameLoader::SubframeLoader::loadOrRedirectSubframe):
* Source/WebCore/page/Frame.cpp:
(WebCore::Frame::Frame):
(WebCore::Frame::~Frame):
* Source/WebCore/page/Frame.h:
(WebCore::Frame::navigationScheduler const):
* Source/WebCore/page/LocalFrame.cpp:
(WebCore::LocalFrame::LocalFrame):
(WebCore::LocalFrame::~LocalFrame):
* Source/WebCore/page/LocalFrame.h:
(WebCore::LocalFrame::navigationScheduler const): Deleted.
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
* Source/WebKit/WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::WebFrame):
(WebKit::WebFrame::invalidate):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm:
(TestWebKitAPI::TEST):

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

6d6b61e

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
βœ… πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@achristensen07 achristensen07 self-assigned this May 12, 2023
@achristensen07 achristensen07 added the WebKit Process Model Bugs related to WebKit's multi-process architecture label May 12, 2023
@achristensen07 achristensen07 force-pushed the eng/Prepare-to-allow-parent-frames-to-navigate-site-isolated-iframes branch from 46c5890 to bb4960f Compare May 12, 2023 00:20
@achristensen07 achristensen07 force-pushed the eng/Prepare-to-allow-parent-frames-to-navigate-site-isolated-iframes branch from bb4960f to 6d6b61e Compare May 12, 2023 00:30
@achristensen07 achristensen07 added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 12, 2023
https://bugs.webkit.org/show_bug.cgi?id=256678
rdar://109235337

Reviewed by Brady Eidson.

This PR does a few things, all of which will not change behavior with site isolation off.

First, it moves NavigationScheduler from LocalFrame to Frame.  This is needed because
if a parent frame sets iframe.src=newURL then the parent's process needs to schedule the
navigation of the iframe.  This is the first of several large steps to get the navigation
to happen.  The next steps are to make a FrameLoader able to be owned by either a LocalFrame
or a RemoteFrame and able to be moved between the two as the navigation causes processes
to change.  I got the NavigationScheduler to the point where it doesn't assert in the API
test ParentNavigatingCrossOriginIframeToSameOrigin when site isolation is enabled, though
the navigation doesn't proceed yet because there is no FrameLoader on the RemoteFrame.

In order to make that compile, I needed to make some InspectorInstrumentation parameters
be Frame instead of LocalFrame.

I also made the source process in WebPageProxy::decidePolicyForNavigationAction be the
WebFrameProxy's current process instead of the process from which the decidePolicyForNavigationAction
message came.  In all tests that are running for site isolation so far, they are the same.

I add some assertions in the WebFrame constructor and invalidator that helped me figure out what
was going wrong when debugging this.  We had some code that was making 2 WebFrames with the same
FrameIdentifier.  This will help future developers catch problems earlier.

I also change the alert in the API test ParentNavigatingCrossOriginIframeToSameOrigin to be
called in the onload handler.  This will allow my frame tree validation code to have deterministic
state once I implement the frame teardown in the previoiusly-used process.  If it weren't in the
onload handler, then there was a race condition in the test because the alert could come before
the didCommitLoadForFrame was processed.

I also move the boolean values initialized in the FrameLoader constructor to use initializers in the header.

* Source/WebCore/inspector/InspectorInstrumentation.cpp:
(WebCore::InspectorInstrumentation::frameScheduledNavigationImpl):
(WebCore::InspectorInstrumentation::frameClearedScheduledNavigationImpl):
* Source/WebCore/inspector/InspectorInstrumentation.h:
(WebCore::InspectorInstrumentation::frameScheduledNavigation):
(WebCore::InspectorInstrumentation::frameClearedScheduledNavigation):
(WebCore::InspectorInstrumentation::instrumentingAgents):
* Source/WebCore/inspector/agents/InspectorPageAgent.cpp:
(WebCore::InspectorPageAgent::frameDetached):
(WebCore::InspectorPageAgent::frameForId):
(WebCore::InspectorPageAgent::frameId):
(WebCore::InspectorPageAgent::assertFrame):
(WebCore::InspectorPageAgent::frameScheduledNavigation):
(WebCore::InspectorPageAgent::frameClearedScheduledNavigation):
* Source/WebCore/inspector/agents/InspectorPageAgent.h:
* Source/WebCore/loader/NavigationDisabler.h:
(WebCore::NavigationDisabler::isNavigationAllowed):
* Source/WebCore/loader/NavigationScheduler.cpp:
(WebCore::NavigationScheduler::NavigationScheduler):
(WebCore::NavigationScheduler::scheduleRedirect):
(WebCore::NavigationScheduler::mustLockBackForwardList):
(WebCore::NavigationScheduler::scheduleLocationChange):
(WebCore::NavigationScheduler::scheduleFormSubmission):
(WebCore::NavigationScheduler::scheduleRefresh):
(WebCore::NavigationScheduler::timerFired):
(WebCore::NavigationScheduler::schedule):
(WebCore::NavigationScheduler::startTimer):
(WebCore::NavigationScheduler::cancel):
* Source/WebCore/loader/NavigationScheduler.h:
* Source/WebCore/loader/SubframeLoader.cpp:
(WebCore::FrameLoader::SubframeLoader::loadOrRedirectSubframe):
* Source/WebCore/page/Frame.cpp:
(WebCore::Frame::Frame):
(WebCore::Frame::~Frame):
* Source/WebCore/page/Frame.h:
(WebCore::Frame::navigationScheduler const):
* Source/WebCore/page/LocalFrame.cpp:
(WebCore::LocalFrame::LocalFrame):
(WebCore::LocalFrame::~LocalFrame):
* Source/WebCore/page/LocalFrame.h:
(WebCore::LocalFrame::navigationScheduler const): Deleted.
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
* Source/WebKit/WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::WebFrame):
(WebKit::WebFrame::invalidate):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm:
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/263999@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Prepare-to-allow-parent-frames-to-navigate-site-isolated-iframes branch from 6d6b61e to 65e5a93 Compare May 12, 2023 03:22
@webkit-commit-queue
Copy link
Collaborator

Committed 263999@main (65e5a93): https://commits.webkit.org/263999@main

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

@webkit-commit-queue webkit-commit-queue merged commit 65e5a93 into WebKit:main May 12, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Process Model Bugs related to WebKit's multi-process architecture
Projects
None yet
4 participants