Bug 230085 - [iOS] Ensure the CA transaction has been synchronously committed before snapshotting it
Summary: [iOS] Ensure the CA transaction has been synchronously committed before snaps...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-08 23:52 PDT by Said Abou-Hallawa
Modified: 2021-10-20 10:30 PDT (History)
8 users (show)

See Also:

Attachments
Patch (1.76 KB, patch)
2021-09-09 00:00 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Patch (6.60 KB, patch)
2021-09-09 23:16 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2021-09-13 18:33 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2021-10-18 18:03 PDT, Said Abou-Hallawa 		thorton: review+
Details | Formatted Diff | Diff
Patch (6.95 KB, patch)
2021-10-19 18:37 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-09-08 23:52:01 PDT 
Committing the CA transaction may be an asynchronous operation.
Comment 1 Said Abou-Hallawa 2021-09-08 23:58:23 PDT 
<rdar://81800118>
Comment 2 Said Abou-Hallawa 2021-09-09 00:00:51 PDT 
Created attachment 437714 [details]
Patch
Comment 3 Tim Horton 2021-09-09 00:11:08 PDT 
Comment on attachment 437714 [details]
Patch

This seems like wild overkill for 99.9% of snapshotting use (which is snapshotting the currently displayed content). Is it really ok to -[CATransaction synchronize] every navigation, for example? (When we take back/forward snapshots)
Comment 4 Tim Horton 2021-09-09 00:19:18 PDT 
Comment on attachment 437714 [details]
Patch

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

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:3208
> +        [CATransaction synchronize];

At a /minimum/, it seems like you need to not do this if WKSnapshotConfiguration.afterScreenUpdates=NO (which currently isn't plumbed in here but could be, and is one way to avoid it in the swipe snapshot case)
Comment 5 Said Abou-Hallawa 2021-09-09 23:16:50 PDT 
Created attachment 437838 [details]
Patch
Comment 6 Alexey Proskuryakov 2021-09-12 14:07:26 PDT 
Is this likely to fix any flaky tests? :)
Comment 7 Wenson Hsieh 2021-09-13 13:22:42 PDT 
Comment on attachment 437838 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:493
> +- (void)_snapshotRectWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration rectInViewCoordinates:(CGRect)rectInViewCoordinates intoImageOfWidth:(CGFloat)imageWidth completionHandler:(void(^)(CGImageRef))completionHandler;

Note that Mail on iOS still uses this method, so we'll need to keep the old SPI around for source/binary compatibility.

(The new method should include a `WK_API_AVAILABLE(…)`, as well)
Comment 8 Simon Fraser (smfr) 2021-09-13 13:28:56 PDT 
Comment on attachment 437838 [details]
Patch

r- because it changes SPI
Comment 9 Said Abou-Hallawa 2021-09-13 18:33:33 PDT 
Created attachment 438096 [details]
Patch
Comment 10 Said Abou-Hallawa 2021-10-18 18:03:48 PDT 
Created attachment 441666 [details]
Patch
Comment 11 Tim Horton 2021-10-19 15:23:03 PDT 
Comment on attachment 441666 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:493
> +- (void)_snapshotRectWithAfterScreenUpdates:(BOOL)afterScreenUpdates rectInViewCoordinates:(CGRect)rectInViewCoordinates intoImageOfWidth:(CGFloat)imageWidth completionHandler:(void(^)(CGImageRef))completionHandler WK_API_AVAILABLE(ios(WK_IOS_TBA));

drop the "with"
Comment 12 Said Abou-Hallawa 2021-10-19 18:37:06 PDT 
Created attachment 441832 [details]
Patch
Comment 13 EWS 2021-10-20 10:30:38 PDT 
Committed r284547 (243289@main): <https://commits.webkit.org/243289@main>

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