Bug 183665 – First piece of process swapping on navigation

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

[Brady Eidson]

First piece of process swapping on navigation

[Brady Eidson]

<rdar://problem/37996312>

[Brady Eidson]

Created attachment 335898 [details]
Patch

[Brady Eidson]

Created attachment 335906 [details]
Patch

[Andy Estes]

Comment on attachment 335906 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:1407
> +    SetForScope<bool> currentLoadShouldCheckNavigationPolicyGuard(m_currentLoadShouldCheckNavigationPolicy, request.shouldCheckNavigationPolicy());

Can shouldCheckNavigationPolicy be a property of DocumentLoader to avoid needing a SetForScope thing here?

> Source/WebKit/UIProcess/API/APINavigation.h:40
> +    Navigation(const Navigation&) = delete;

WTF_MAKE_NONCOPYABLE?

> Source/WebKit/UIProcess/WebPageProxy.cpp:2392
> +    if (m_currentNavigationPolicyListenerID && *m_currentNavigationPolicyListenerID == listenerID) {
> +        m_currentNavigationPolicyListenerID = std::nullopt;
> +
> +        if (action == PolicyAction::Use && navigation) {
> +            auto proposedProcess = process().processPool().processForNavigation(*this, navigation->request().url());
> +            if (proposedProcess.ptr() != &process()) {
> +                action = PolicyAction::Suspend;
> +                RunLoop::main().dispatch([this, protectedThis = makeRef(*this), navigation = makeRef(*navigation), proposedProcess = WTFMove(proposedProcess)]() mutable {
> +                    continueNavigationInNewProcess(navigation.get(), WTFMove(proposedProcess));
> +                });
> +            }
> +        }
> +    }

I was confused why we only now needed m_currentNavigationPolicyListenerID, and you explained to me that it was to distinguish decidePolicyForNavigationAction from the other policy decisions (response, new window). A more direct way to do this might be to add a type enum to WebFramePolicyListenerProxy and check that instead. Looks like you can get to the active listener from WebFrameProxy::m_activeListener (and you could always assert or check that it's listener ID equals listenerID).

[Brady Eidson]

(In reply to Andy Estes from comment #4)
> Comment on attachment 335906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335906&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1407
> > +    SetForScope<bool> currentLoadShouldCheckNavigationPolicyGuard(m_currentLoadShouldCheckNavigationPolicy, request.shouldCheckNavigationPolicy());
> 
> Can shouldCheckNavigationPolicy be a property of DocumentLoader to avoid
> needing a SetForScope thing here?

Unique DocumentLoaders can go through multiple rounds of policy checks, as well, so something similar would still be needed.

> 
> > Source/WebKit/UIProcess/API/APINavigation.h:40
> > +    Navigation(const Navigation&) = delete;
> 
> WTF_MAKE_NONCOPYABLE?

Sure

> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:2392
> > +    if (m_currentNavigationPolicyListenerID && *m_currentNavigationPolicyListenerID == listenerID) {
> > +        m_currentNavigationPolicyListenerID = std::nullopt;
> > +
> > +        if (action == PolicyAction::Use && navigation) {
> > +            auto proposedProcess = process().processPool().processForNavigation(*this, navigation->request().url());
> > +            if (proposedProcess.ptr() != &process()) {
> > +                action = PolicyAction::Suspend;
> > +                RunLoop::main().dispatch([this, protectedThis = makeRef(*this), navigation = makeRef(*navigation), proposedProcess = WTFMove(proposedProcess)]() mutable {
> > +                    continueNavigationInNewProcess(navigation.get(), WTFMove(proposedProcess));
> > +                });
> > +            }
> > +        }
> > +    }
> 
> I was confused why we only now needed m_currentNavigationPolicyListenerID,
> and you explained to me that it was to distinguish
> decidePolicyForNavigationAction from the other policy decisions (response,
> new window). A more direct way to do this might be to add a type enum to
> WebFramePolicyListenerProxy and check that instead. Looks like you can get
> to the active listener from WebFrameProxy::m_activeListener (and you could
> always assert or check that it's listener ID equals listenerID).

Will try this.

[Brady Eidson]

Created attachment 336164 [details]
Patch

[Brady Eidson]

(In reply to Brady Eidson from comment #6)
> Created attachment 336164 [details]
> Patch

Will cq+ after EWS

[WebKit Commit Bot]

Comment on attachment 336164 [details]
Patch

Clearing flags on attachment: 336164

Committed r229778: <https://trac.webkit.org/changeset/229778>

[Michael Catanzaro]

Fixed?