Bug 184142 – Process swapping on navigation needs to handle server redirects

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

[Brady Eidson]

Process swapping on navigation needs to handle server redirects

<rdar://problem/38690465>

[Brady Eidson]

Created attachment 336912 [details]
Patch

[Brady Eidson]

Created attachment 336927 [details]
Patch

[Alex Christensen]

Comment on attachment 336927 [details]
Patch

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

> Source/WebKit/ChangeLog:14
> +        3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com.

Will future work will make it so a redirect from a to b then back to a will reuse the same process?

> Source/WebKit/UIProcess/WebPageProxy.cpp:2397
> +        m_mainFrameSetHandler = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), request = ResourceRequest { navigation.currentRequest() }]() mutable {

I think the explicit call to the constructor might be unnecessary.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3212
> +    if (m_mainFrameSetHandler) {

m_mainFrameCreationHandler?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3855
> +    Ref<WebFramePolicyListenerProxy> listener = frame->setUpPolicyListenerProxy(listenerID, PolicyListenerType::NavigationAction);

auto?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:162
> +        [(id<WKURLSchemeTaskPrivate>)task _didPerformRedirection:redirectResponse.get() newRequest:request.get()];

Wow, this SPI seems really useful!  I'm glad we have it.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:568
> +    RetainPtr<PSONScheme> handler1 = adoptNS([[PSONScheme alloc] init]);

You have inconsistent use of auto in your tests.

[youenn fablet]

> > Source/WebKit/ChangeLog:14
> > +        3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com.
> 
> Will future work will make it so a redirect from a to b then back to a will
> reuse the same process?

Ideally, future work will enable us to do full loading in NetworkProcess without going through WebProcess, including navigations. That would handle this point automatically, although we might want to get to it before having the full loading stuff.

[Brady Eidson]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 336927 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336927&action=review
> 
> > Source/WebKit/ChangeLog:14
> > +        3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com.
> 
> Will future work will make it so a redirect from a to b then back to a will
> reuse the same process?

Maybe. 
Not necessarily worth it though - At the a.com to b.com redirect we have to fire up a second process, but then reverting back to the a.com process is more work...
CPU/Battery tradeoff...
I'll file a bug tracking the idea, definitely not convinced it's a slam dunk.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:2397
> > +        m_mainFrameSetHandler = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), request = ResourceRequest { navigation.currentRequest() }]() mutable {
> 
> I think the explicit call to the constructor might be unnecessary.

Yah it was in some places, maybe not here, will 2x check.

> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3212
> > +    if (m_mainFrameSetHandler) {
> 
> m_mainFrameCreationHandler?
> 

Sure.


> > Source/WebKit/UIProcess/WebPageProxy.cpp:3855
> > +    Ref<WebFramePolicyListenerProxy> listener = frame->setUpPolicyListenerProxy(listenerID, PolicyListenerType::NavigationAction);
> 
> auto?

Sure.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:162
> > +        [(id<WKURLSchemeTaskPrivate>)task _didPerformRedirection:redirectResponse.get() newRequest:request.get()];
> 
> Wow, this SPI seems really useful!  I'm glad we have it.

Nice! I'll CC you on the bugs I file for how it does weird things... :)

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:568
> > +    RetainPtr<PSONScheme> handler1 = adoptNS([[PSONScheme alloc] init]);
> 
> You have inconsistent use of auto in your tests.

Tests tests shmests

[Brady Eidson]

(In reply to youenn fablet from comment #4)
> > > Source/WebKit/ChangeLog:14
> > > +        3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com.
> > 
> > Will future work will make it so a redirect from a to b then back to a will
> > reuse the same process?
> 
> Ideally, future work will enable us to do full loading in NetworkProcess
> without going through WebProcess, including navigations. That would handle
> this point automatically, although we might want to get to it before having
> the full loading stuff.

^^^^ 👍

[Brady Eidson]

Created attachment 336997 [details]
Patch

[Brady Eidson]

Created attachment 337002 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337002 [details]
Patch

Clearing flags on attachment: 337002

Committed r230174: <https://trac.webkit.org/changeset/230174>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

iOS and macOS Debug WK2 tests are exiting early due to crashes after this change.

ASSERTION FAILED: navigation->originalRequest().url() == originalRequest.url()
/Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/WebPageProxy.cpp(3845) : void WebKit::WebPageProxy::decidePolicyForNavigationAction(uint64_t, const WebCore::SecurityOriginData &, uint64_t, WebKit::NavigationActionData &&, const WebKit::FrameInfoData &, uint64_t, const WebCore::ResourceRequest &, WebCore::ResourceRequest &&, uint64_t, const WebKit::UserData &)

See:https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r230176%20(2692)/results.html

[Ryan Haddad]

Reverted r230174 for reason:

Caused LayoutTests to exit early with assertion failures.

Committed r230191: <https://trac.webkit.org/changeset/230191>

[Brady Eidson]

(In reply to Ryan Haddad from comment #11)
> iOS and macOS Debug WK2 tests are exiting early due to crashes after this
> change.

Annnnnnnnnnnd we don't have EWS for either of them.

Still.

To this day.

[Brady Eidson]

Created attachment 337046 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337046 [details]
Patch

Clearing flags on attachment: 337046

Committed r230195: <https://trac.webkit.org/changeset/230195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.