Bug 219632 - REGRESSION(r261238): WKWebView crashes on launch inside a quicklook preview
Summary: REGRESSION(r261238): WKWebView crashes on launch inside a quicklook preview
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Safari 14
Hardware: Mac Other
: P2 Critical
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-08 01:52 PST by sbarex
Modified: 2021-01-27 11:33 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.23 KB, patch)
2021-01-25 19:06 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sbarex 2020-12-08 01:52:29 PST
I have a Quicklook appex that show the file contents by a WKWebView. The application and the extension are sandboxed. When try to use the quickllok preview I see only the spinning icon.
On XCode this is the log:

[Process] 0x7f8cbc029620 - [pageProxyID=5, webPageID=6, PID=0] WebPageProxy::processDidTerminate: (pid 0), reason 3
[Loading] 0x7f8cbc029620 - [pageProxyID=5, webPageID=6, PID=0] WebPageProxy::dispatchProcessDidTerminate: reason = 3
[Process] 0x7f8cbc029620 - [pageProxyID=5, webPageID=6, PID=0] WebPageProxy::tryReloadAfterProcessTermination: process crashed and the client did not handle it, not reloading the page because we reached the maximum number of attempts

Using the webkit source code I see this other output log:
com.apple.WebKit.Networking.Development[96084:1414561] Application does not have permission to communicate with network resources. rc=1 : errno=22

Che entitlements check fail on `XPCServiceInitializerDelegate::checkEntitlements` in XPCServiceEntryPoint.mm. If I hack this function returning true the preview works well.
I see that the function was changed on revision 261238 of the 2020-05-06 19:32. From this revision the `checkEntitlements` uses the SandboxSPI to check the entitlements with `sandbox_check_by_audit_token`. Before it uses `hasEntitlement` (that uses `SecTaskCopyValueForEntitlement` to perform the check).

Both the application and the extension contain the entitlement `com.apple.security.network.client`.

Please note that the application works on Catalina. 
If I use the deprecated WebView insted of WKWebView the preview works also on Big Sur.
Comment 1 Radar WebKit Bug Importer 2020-12-09 14:59:15 PST
<rdar://problem/72154830>
Comment 2 Geoffrey Garen 2020-12-09 15:41:34 PST
Seems like something about the quicklook preview setup does not carry the entitlement through the audit token.

I wonder if you can hack most of the code out of your project and turn it into a test app demonstrating this problem.
Comment 3 Alexey Proskuryakov 2020-12-09 16:17:38 PST
> Both the application and the extension contain the entitlement `com.apple.security.network.client`.

I don't think that it does anything for QuickLook extensions.

Can you try using a com.apple.security.temporary-exception.mach-lookup.global-name exception entitlement for com.apple.nsurlsessiond instead? That would be for diagnosis only, I'm not suggesting to do this in production at this point.
Comment 4 sbarex 2020-12-10 12:05:28 PST
(In reply to Geoffrey Garen from comment #2)
> Seems like something about the quicklook preview setup does not carry the
> entitlement through the audit token.
> 
> I wonder if you can hack most of the code out of your project and turn it
> into a test app demonstrating this problem.

Hi,
I have created a demo code here:
https://github.com/sbarex/QLTest

The quicklook appex handle files with extension .sbarex_test 
The PreviewController create a WKWebView and show a "hello world" message with loadHTMLString (so for this example do not shows the file contents).

The completion handler to show the ql preview is called when the html rendering is complete.

On Big Sur webkit fail immediately and the handle in never called (So the spinning icon rest visibile).

Inside the PreviewController.swift I have inserted but commented the code that use the old WebView. Using this code instead of the wkwebview the preview works.
Comment 5 sbarex 2020-12-10 12:11:57 PST
(In reply to Alexey Proskuryakov from comment #3)
> > Both the application and the extension contain the entitlement `com.apple.security.network.client`.
> 
> I don't think that it does anything for QuickLook extensions.
> 
> Can you try using a
> com.apple.security.temporary-exception.mach-lookup.global-name exception
> entitlement for com.apple.nsurlsessiond instead? That would be for diagnosis
> only, I'm not suggesting to do this in production at this point.

I do not understand which entitlement I must try to add...
com.apple.security.temporary-exception.mach-lookup.global-name does nothing for the bug.

If I add com.apple.nsurlsessiond I need to sign the app with a certificate. Then the appex fail to load and Xcode cannot bind to the process to debug (failure Reason: “org.sbarex.QLTest.QLExt” failed to launch or exited before the debugger could attach to it. Please verify that “org.sbarex.QLTest.QLExt” has a valid code signature that permits it to be launched on “My Mac”.)
Comment 6 Alexey Proskuryakov 2020-12-10 13:02:31 PST
Please see documentation for com.apple.security.temporary-exception.mach-lookup.global-name at https://developer.apple.com/library/archive/documentation/Miscellaneous/Reference/EntitlementKeyReference/Chapters/AppSandboxTemporaryExceptionEntitlements.html.

The idea is that WebContent process confirms that the UI process can look up the com.apple.nsurlsessiond name. For applications, com.apple.security.network.client includes that, but as far as I know, QuickLook extensions don't honor com.apple.security.network.client.
Comment 7 sbarex 2020-12-10 14:34:41 PST
(In reply to Alexey Proskuryakov from comment #6)
> Please see documentation for
> com.apple.security.temporary-exception.mach-lookup.global-name at
> https://developer.apple.com/library/archive/documentation/Miscellaneous/
> Reference/EntitlementKeyReference/Chapters/
> AppSandboxTemporaryExceptionEntitlements.html.
> 
> The idea is that WebContent process confirms that the UI process can look up
> the com.apple.nsurlsessiond name. For applications,
> com.apple.security.network.client includes that, but as far as I know,
> QuickLook extensions don't honor com.apple.security.network.client.

Thanks, now I understand.
By setting the entitlement com.apple.security.temporary-exception.mach-lookup.global-name as an array and inserting the value com.apple.nsurlsessiond  finally the extension works! Many thanks!

However with Catalina it was not necessary to insert an exception in the entitlements. I don't know if for a different management of the quicklook framework or for the different version of webkit.

From what I understand most of the formats supported by the system are rendered in quicklook via a webkit view (although apple still uses the deperecated qlgenerator api), so it seems strange to me that it does not allow its use in the new framework.

There is still one last bug but I'm afraid it doesn't depend on webkit. When scroll bars appear, it is not possible to drag the scroller with the mouse because the action causes the entire window to be dragged (but you can scroll with trackpad gesture).
Comment 8 Alexey Proskuryakov 2020-12-10 15:22:23 PST
This was a WebKit change, you correctly identified it to be http://trac.webkit.org/r261238.

Thank you for testing! Just to reiterate, I'm not saying that adding a temporary entitlement is a permanent or even recommended solution. We'll need to look into this more.

> There is still one last bug but I'm afraid it doesn't depend on webkit.

https://feedbackassistant.apple.com is always a good place to report bugs that may or may not be in WebKit.
Comment 9 sbarex 2020-12-10 15:31:30 PST
(In reply to Alexey Proskuryakov from comment #8)
> This was a WebKit change, you correctly identified it to be
> http://trac.webkit.org/r261238.
> 
> Thank you for testing! Just to reiterate, I'm not saying that adding a
> temporary entitlement is a permanent or even recommended solution. We'll
> need to look into this more.
> 
> > There is still one last bug but I'm afraid it doesn't depend on webkit.
> 
> https://feedbackassistant.apple.com is always a good place to report bugs
> that may or may not be in WebKit.

Thanks. Yes I know it's not a permanent solution but at least it makes the code usable. In the absence of a temporary solution I was thinking of suspending some my quicklook extension projects while now I can take them back in hand.

For the feedback to Apple I have already sent it (both for this bug, for the scroll bar bug and also for other things in the past) but unfortunately I have never received feedback until now.
Comment 10 Brent Fulgham 2021-01-25 19:06:19 PST
Created attachment 418364 [details]
Patch
Comment 11 EWS 2021-01-26 12:40:24 PST
Committed r271895: <https://trac.webkit.org/changeset/271895>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418364 [details].
Comment 12 sbarex 2021-01-27 11:33:48 PST
(In reply to Brent Fulgham from comment #10)
> Created attachment 418364 [details]
> Patch

Thank you very much