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

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].