Bug 225333 – Add SPI to suspend / resume a WKWebView

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Add SPI to suspend / resume a WKWebView.

[Chris Dumez]

<rdar://77462543>

[Chris Dumez]

Created attachment 427613 [details]
WIP Patch

[Chris Dumez]

Created attachment 427619 [details]
WIP Patch

[Chris Dumez]

Created attachment 427623 [details]
WIP Patch (with test)

[Chris Dumez]

Created attachment 427670 [details]
Patch

[Sam Weinig]

Comment on attachment 427670 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        Add SPI to suspend / resume a WKWebView. This suspends the page as if it was in
> +        the back/forward cache but the suspension happens in-place, no need to navigate.
> +        The suspended page is also not part of the Back/Forward cache so its cap on the
> +        number of suspended pages does not apply here.

What are the rules for what you can do with a suspended WKWebView? Can you call loadRequest:?  Will that resume it?

[Geoffrey Garen]

> What are the rules for what you can do with a suspended WKWebView? Can you
> call loadRequest:?  Will that resume it?

This question makes me wonder if we really want suspension to be a state on the WebView, or if it would be better to model it more explicitly like a navigation to about:blank. (Not that we would require the client to actually navigate to about:blank -- just that the implementation of suspend would navigate to about:blank or about:suspended or whatever, and the implementation of resume would navigate back. Perhaps we would also hide the actual about:blank or about:suspended entry from history and/or back/forward list.)

[Chris Dumez]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 427670 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427670&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        Add SPI to suspend / resume a WKWebView. This suspends the page as if it was in
> > +        the back/forward cache but the suspension happens in-place, no need to navigate.
> > +        The suspended page is also not part of the Back/Forward cache so its cap on the
> > +        number of suspended pages does not apply here.
> 
> What are the rules for what you can do with a suspended WKWebView? Can you
> call loadRequest:?  Will that resume it?

With the current implementation, it isn't safe to do much with a suspended view besides resuming it or destroying it. We could support loadRequest if we wanted. This is not a use case we have currently though.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #7)
> > What are the rules for what you can do with a suspended WKWebView? Can you
> > call loadRequest:?  Will that resume it?
> 
> This question makes me wonder if we really want suspension to be a state on
> the WebView, or if it would be better to model it more explicitly like a
> navigation to about:blank. (Not that we would require the client to actually
> navigate to about:blank -- just that the implementation of suspend would
> navigate to about:blank or about:suspended or whatever, and the
> implementation of resume would navigate back. Perhaps we would also hide the
> actual about:blank or about:suspended entry from history and/or back/forward
> list.)

2 issues:
a. Yes, we'd need logic to hide the about:blank / about:suspended from the back/forward list. This was a major point to introducing this SPI vs having the client navigate.
b. Your statement assumes we have a history item to go back to. Unlike back/forward cache, this new suspension mechanism does not require a HistoryItem. Note that calling loadHTMLString or loadData on a WKWebView does NOT create a HistoryItem. Yet I think we should be able to suspend it.

[Chris Dumez]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 427670 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427670&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        Add SPI to suspend / resume a WKWebView. This suspends the page as if it was in
> > +        the back/forward cache but the suspension happens in-place, no need to navigate.
> > +        The suspended page is also not part of the Back/Forward cache so its cap on the
> > +        number of suspended pages does not apply here.
> 
> What are the rules for what you can do with a suspended WKWebView? Can you
> call loadRequest:?  Will that resume it?

I did not focus on all these cases because this is not API yet, just SPI for now used in a very specific way. If it were API, I agree we'd need to go through each WKWebView API and decide what their behavior should be when suspended.

Since I think the SPI would eventually be useful as API, I do think I should follow-up and figure those cases out though. I was hoping to land this ASAP to unblock the client app that needs it but if people think we need a better story for what each WKWebView API does when suspended before landing, I can prioritize it.

[Chris Dumez]

Created attachment 427775 [details]
Patch

[Sam Weinig]

(In reply to Chris Dumez from comment #10)
> (In reply to Sam Weinig from comment #6)
> > Comment on attachment 427670 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427670&action=review
> > 
> > > Source/WebKit/ChangeLog:12
> > > +        Add SPI to suspend / resume a WKWebView. This suspends the page as if it was in
> > > +        the back/forward cache but the suspension happens in-place, no need to navigate.
> > > +        The suspended page is also not part of the Back/Forward cache so its cap on the
> > > +        number of suspended pages does not apply here.
> > 
> > What are the rules for what you can do with a suspended WKWebView? Can you
> > call loadRequest:?  Will that resume it?
> 
> I did not focus on all these cases because this is not API yet, just SPI for
> now used in a very specific way. If it were API, I agree we'd need to go
> through each WKWebView API and decide what their behavior should be when
> suspended.
> 
> Since I think the SPI would eventually be useful as API, I do think I should
> follow-up and figure those cases out though. I was hoping to land this ASAP
> to unblock the client app that needs it but if people think we need a better
> story for what each WKWebView API does when suspended before landing, I can
> prioritize it.

I generally think we should be very careful with any new SPI as they can be quite hard to remove once adopted.

For something as big as this, I think we need the functionality to be fleshed out and tested before we would want anyone using it.

[Chris Dumez]

(In reply to Sam Weinig from comment #12)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Sam Weinig from comment #6)
> > > Comment on attachment 427670 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=427670&action=review
> > > 
> > > > Source/WebKit/ChangeLog:12
> > > > +        Add SPI to suspend / resume a WKWebView. This suspends the page as if it was in
> > > > +        the back/forward cache but the suspension happens in-place, no need to navigate.
> > > > +        The suspended page is also not part of the Back/Forward cache so its cap on the
> > > > +        number of suspended pages does not apply here.
> > > 
> > > What are the rules for what you can do with a suspended WKWebView? Can you
> > > call loadRequest:?  Will that resume it?
> > 
> > I did not focus on all these cases because this is not API yet, just SPI for
> > now used in a very specific way. If it were API, I agree we'd need to go
> > through each WKWebView API and decide what their behavior should be when
> > suspended.
> > 
> > Since I think the SPI would eventually be useful as API, I do think I should
> > follow-up and figure those cases out though. I was hoping to land this ASAP
> > to unblock the client app that needs it but if people think we need a better
> > story for what each WKWebView API does when suspended before landing, I can
> > prioritize it.
> 
> I generally think we should be very careful with any new SPI as they can be
> quite hard to remove once adopted.
> 
> For something as big as this, I think we need the functionality to be
> fleshed out and tested before we would want anyone using it.

I did flesh it out more in my latest iteration. Requests to navigate or run JS are now handled in particular.

[Chris Dumez]

Created attachment 427810 [details]
Patch

[Chris Dumez]

Created attachment 427815 [details]
Patch

[Sam Weinig]

(In reply to Chris Dumez from comment #13)
> (In reply to Sam Weinig from comment #12)
> > (In reply to Chris Dumez from comment #10)
> > > (In reply to Sam Weinig from comment #6)
> > > > Comment on attachment 427670 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=427670&action=review
> > > > 
> > > > > Source/WebKit/ChangeLog:12
> > > > > +        Add SPI to suspend / resume a WKWebView. This suspends the page as if it was in
> > > > > +        the back/forward cache but the suspension happens in-place, no need to navigate.
> > > > > +        The suspended page is also not part of the Back/Forward cache so its cap on the
> > > > > +        number of suspended pages does not apply here.
> > > > 
> > > > What are the rules for what you can do with a suspended WKWebView? Can you
> > > > call loadRequest:?  Will that resume it?
> > > 
> > > I did not focus on all these cases because this is not API yet, just SPI for
> > > now used in a very specific way. If it were API, I agree we'd need to go
> > > through each WKWebView API and decide what their behavior should be when
> > > suspended.
> > > 
> > > Since I think the SPI would eventually be useful as API, I do think I should
> > > follow-up and figure those cases out though. I was hoping to land this ASAP
> > > to unblock the client app that needs it but if people think we need a better
> > > story for what each WKWebView API does when suspended before landing, I can
> > > prioritize it.
> > 
> > I generally think we should be very careful with any new SPI as they can be
> > quite hard to remove once adopted.
> > 
> > For something as big as this, I think we need the functionality to be
> > fleshed out and tested before we would want anyone using it.
> 
> I did flesh it out more in my latest iteration. Requests to navigate or run
> JS are now handled in particular.

Is the rule then that any use of API that would ordinarily work should work when suspended, resuming the page? Do some functions not resume the webview?

[Sam Weinig]

I should add, my personal opinion would be that all WKWebView API/SPI throw or crash if the page is suspended (except resume of course).

[Chris Dumez]

(In reply to Sam Weinig from comment #17)
> I should add, my personal opinion would be that all WKWebView API/SPI throw
> or crash if the page is suspended (except resume of course).

What I implemented is so far is my personal preference (but I am happy to change if others feel differently):
1. Navigating the views unsuspends the page before navigating
2. Operations that do not require the WebProcess keep working
2. Operations that need the WebProcess (e.g. evaluateJS(), find(), takeSnapshot(), createWebArchive, createPDF, ...) fail (as in I am calling their completion handler with an error). I have no covered everything in this case but the top ones.

Throwing an exception for every SPI/API call besides resume() seems super aggressive but then again, that's pretty much the contract we have with the client. That said, there are a lot of API/SPIs that do not need the WebProcess in any way (e.g. WKWebView.URL, WKWebView.isLoading, WKWebView.title, WKWebView.canGoBack) and could keep working just fine while suspended I think.

[Sam Weinig]

(In reply to Chris Dumez from comment #18)
> (In reply to Sam Weinig from comment #17)
> > I should add, my personal opinion would be that all WKWebView API/SPI throw
> > or crash if the page is suspended (except resume of course).
> 
> What I implemented is so far is my personal preference (but I am happy to
> change if others feel differently):
> 1. Navigating the views unsuspends the page before navigating
> 2. Operations that do not require the WebProcess keep working
> 2. Operations that need the WebProcess (e.g. evaluateJS(), find(),
> takeSnapshot(), createWebArchive, createPDF, ...) fail (as in I am calling
> their completion handler with an error). I have no covered everything in
> this case but the top ones.
> 
> Throwing an exception for every SPI/API call besides resume() seems super
> aggressive but then again, that's pretty much the contract we have with the
> client. That said, there are a lot of API/SPIs that do not need the
> WebProcess in any way (e.g. WKWebView.URL, WKWebView.isLoading,
> WKWebView.title, WKWebView.canGoBack) and could keep working just fine while
> suspended I think.

I realize I didn't really add my reasons for my opinion. There are a few reasons I think being heavy handed makes sense.

1) It is much easier to relax things in the future than it is to make them stricter. For instance, if we want to make it valid to call WKWebView.canGoBack when suspended, we can remove the check and document in the header that WKWebView.canGoBack can be called when suspended.
2) Side effects for methods like loadRequest: resuming the view are hard to teach, especially when it is no 100% clear which methods will have the side effects and which won't.

[Geoffrey Garen]

I think I agree that throwing an exception when an operation obviously can't work because we're suspended is a helpful approach -- and more helpful than auto-unsuspending, since that might put the app in an invalid state.

Why don't we compromise and throw an exception in APIs that (a) can't work while suspended or (b) it's not obvious whether they can work or not while suspended. But for the APIs where it is obvious that they can work while suspended, let's not throw. It just doesn't make sense. And I worry that we'll create a long tail of trivial exception noise that doesn't help anyone -- for example, just because Interface Builder has the WebView's URL or title hooked up to some view, which seems like an obviously valid thing to do even while suspended.

[Geoffrey Garen]

Side note: This discussion reminds me of the setDefersLoading() API, and the numerous costly "blank WebView" / "load failed" bugs it caused. Hopefully throwing an exception in APIs that obviously won't work, including loading APIs, will help avoid that kind of problem this time around.

[Chris Dumez]

Created attachment 428213 [details]
Patch

[Chris Dumez]

(In reply to Geoffrey Garen from comment #20)
> I think I agree that throwing an exception when an operation obviously can't
> work because we're suspended is a helpful approach -- and more helpful than
> auto-unsuspending, since that might put the app in an invalid state.
> 
> Why don't we compromise and throw an exception in APIs that (a) can't work
> while suspended or (b) it's not obvious whether they can work or not while
> suspended. But for the APIs where it is obvious that they can work while
> suspended, let's not throw. It just doesn't make sense. And I worry that
> we'll create a long tail of trivial exception noise that doesn't help anyone
> -- for example, just because Interface Builder has the WebView's URL or
> title hooked up to some view, which seems like an obviously valid thing to
> do even while suspended.

Ok, I have implemented this proposal and extended API test coverage accordingly.

[Chris Dumez]

Created attachment 428218 [details]
Patch

[Geoffrey Garen]

iOS EWS is angry: TestWebKitAPI.ProcessSuspension.WKWebViewSuspendPage

[Chris Dumez]

(In reply to Geoffrey Garen from comment #25)
> iOS EWS is angry: TestWebKitAPI.ProcessSuspension.WKWebViewSuspendPage

It is minor, will fix.

   TestWebKitAPI.ProcessSuspension.WKWebViewSuspendPage
        
        /Volumes/Data/worker/iOS-14-Simulator-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSuspension.mm:192
        Value of: [webView allowsLinkPreview]
          Actual: true
        Expected: false

Looks like this property has a different default value on iOS than macOS is all.

[Chris Dumez]

(In reply to Chris Dumez from comment #26)
> (In reply to Geoffrey Garen from comment #25)
> > iOS EWS is angry: TestWebKitAPI.ProcessSuspension.WKWebViewSuspendPage
> 
> It is minor, will fix.
> 
>    TestWebKitAPI.ProcessSuspension.WKWebViewSuspendPage
>         
>        
> /Volumes/Data/worker/iOS-14-Simulator-Build-EWS/build/Tools/TestWebKitAPI/
> Tests/WebKitCocoa/ProcessSuspension.mm:192
>         Value of: [webView allowsLinkPreview]
>           Actual: true
>         Expected: false
> 
> Looks like this property has a different default value on iOS than macOS is
> all.

```
/*! @abstract A Boolean value indicating whether link preview is allowed for any
 links inside this WKWebView.
 @discussion The default value is YES on Mac and iOS.
 */
@property (nonatomic) BOOL allowsLinkPreview WK_API_AVAILABLE(macos(10.11), ios(9.0));
```

That doc seems to be a lie. Turns out it is true on both platforms.

[Chris Dumez]

Created attachment 428296 [details]
Patch

[Chris Dumez]

Created attachment 428298 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #27)
> (In reply to Chris Dumez from comment #26)
> > (In reply to Geoffrey Garen from comment #25)
> > > iOS EWS is angry: TestWebKitAPI.ProcessSuspension.WKWebViewSuspendPage
> > 
> > It is minor, will fix.
> > 
> >    TestWebKitAPI.ProcessSuspension.WKWebViewSuspendPage
> >         
> >        
> > /Volumes/Data/worker/iOS-14-Simulator-Build-EWS/build/Tools/TestWebKitAPI/
> > Tests/WebKitCocoa/ProcessSuspension.mm:192
> >         Value of: [webView allowsLinkPreview]
> >           Actual: true
> >         Expected: false
> > 
> > Looks like this property has a different default value on iOS than macOS is
> > all.
> 
> ```
> /*! @abstract A Boolean value indicating whether link preview is allowed for
> any
>  links inside this WKWebView.
>  @discussion The default value is YES on Mac and iOS.
>  */
> @property (nonatomic) BOOL allowsLinkPreview WK_API_AVAILABLE(macos(10.11),
> ios(9.0));
> ```
> 
> That doc seems to be a lie. Turns out it is true on both platforms.

Never mind, looks like I misread the doc...

[Chris Dumez]

Created attachment 428303 [details]
Patch

[Geoffrey Garen]

Comment on attachment 428303 [details]
Patch

r=me

[EWS]

Committed r277341 (237600@main): <https://commits.webkit.org/237600@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428303 [details].

[Ryan Haddad]

This change has caused all WK2 layout tests to exit with an exception. It seems that there is a ton of this logging during test runs: 
Error: Attempt to call WKPageRef API/SPI on a suspended page, this is a client bug.

https://build.webkit.org/#/builders/70/builds/2492

[WebKit Commit Bot]

Re-opened since this is blocked by bug 225685

[Chris Dumez]

Created attachment 428341 [details]
Patch

[Chris Dumez]

Created attachment 428342 [details]
Patch

[Chris Dumez]

Created attachment 428344 [details]
Patch

[EWS]

Committed r277356 (237614@main): <https://commits.webkit.org/237614@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428344 [details].