Bug 187739

Summary: -_beginAnimatedResizeWithUpdates: can leave view in bad state if called during an existing animation
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews201 for win-future none

Description Jer Noble 2018-07-17 14:44:35 PDT
-_beginAnimatedResizeWithUpdates: can leave view in bad state if called during an existing animation
Comment 1 Jer Noble 2018-07-17 14:58:09 PDT
Created attachment 345194 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2018-07-17 14:59:18 PDT
<rdar://problem/42304518>
Comment 3 Tim Horton 2018-07-17 15:27:04 PDT
Comment on attachment 345194 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2906
> +    [_resizeAnimationView removeFromSuperview];

resizeAnimationView has the WKContentView as its subview so you just tossed it.

Look at the existing code that does this to see how to clean up correctly
Comment 4 Jer Noble 2018-07-17 16:07:17 PDT
Created attachment 345204 [details]
Patch
Comment 5 Tim Horton 2018-07-17 16:17:34 PDT
Comment on attachment 345204 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2925
> -        _dynamicViewportUpdateMode = WebKit::DynamicViewportUpdateMode::NotResizing;
> +        [self _cancelAnimatedResize];

IIIIII don't know about this one. Does your code do anything even remotely acceptable if _resizeAnimationView is nil?
Comment 6 Jer Noble 2018-07-17 16:24:50 PDT
Comment on attachment 345204 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2925
>> +        [self _cancelAnimatedResize];
> 
> IIIIII don't know about this one. Does your code do anything even remotely acceptable if _resizeAnimationView is nil?

Hmm, -indexOfObject: will fail, and we may move _scrollView to the end. But yeah, that should probably be protected by: "if (!_customContentView && _resizeAnimationView)".
Comment 7 Tim Horton 2018-07-17 16:26:01 PDT
Other than that, it seems OK, but I am afraid. Also, this should be trivially API testable.
Comment 8 Tim Horton 2018-07-17 16:26:10 PDT
There's already a file full of them.
Comment 9 Jer Noble 2018-07-17 17:28:40 PDT
Created attachment 345212 [details]
Patch
Comment 10 EWS Watchlist 2018-07-18 01:41:27 PDT
Comment on attachment 345212 [details]
Patch

Attachment 345212 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8572226

New failing tests:
http/tests/preload/onload_event.html
Comment 11 EWS Watchlist 2018-07-18 01:41:38 PDT
Created attachment 345233 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 12 WebKit Commit Bot 2018-07-18 12:52:01 PDT
Comment on attachment 345212 [details]
Patch

Clearing flags on attachment: 345212

Committed r233927: <https://trac.webkit.org/changeset/233927>
Comment 13 WebKit Commit Bot 2018-07-18 12:52:02 PDT
All reviewed patches have been landed.  Closing bug.