Skip to content
Permalink
Browse files
REGRESSION (r290875): [iOS] Safari renders many blank tiles after nav…
…igating back to webpage from PDF

https://bugs.webkit.org/show_bug.cgi?id=241536
rdar://94637323

Reviewed by Tim Horton.

When using a standard content view, the logic in `WKApplicationStateTrackingView` creates a
`WebKit::ApplicationStateTracker` after the view becomes parented in the view hierarchy (in
`-didMoveToWindow`), and holds onto it until it is about to be unparented from the view hierarchy
(in `-willMoveToWindow:`).

However, in the case where `WKApplicationStateTrackingView` is a `WKPDFView` (i.e. we're hosting a
remote PDFKit view controller), the behavior is much more intricate; this is because the `WKPDFView`
is replaced in the view hierarchy by the remote view controller's sizing view once the
`PDFHostViewController` has finished loading its remote content. Once this happens, the
`-_contentView` of the `WKApplicationStateTrackingView` points to this sizing view instead, and the
`WKPDFView` (which subclasses `WKApplicationStateTrackingView`) is no longer in the view hierarchy.

Prior to r290875: when we remove `WKPDFView` from the view hierarchy and replace it with a sizing
view, `self._contentView` in both cases already points to the `_UISizeTrackingView`, even though it
hasn't been added to the view hierarchy yet. Since we bail if this size tracking view is not in the
view hierarchy, we previously ended up never clearing out the application state tracker when loading
a PDF, which means that `-[WKPDFView isBackground]` would always return NO, despite the view not
being in the view hierarchy.

In r290875, I replaced the `!self._contentView.window` check with `!_applicationStateTracker`, which
caused us to now clear out the application state tracker when unparenting `WKPDFView`. This, in
turn, means that `-[WKPDFView isBackground]` now returns YES when the `WKPDFView` is removed and
replaced with the size tracking view. When navigating back to the previous page, this causes us to
end up in a state where the `WindowIsActive` flag in `WebPageProxy::m_activityState` is off until
the next page load, since we only update this flag in `WebPageProxy::finishAttachingToWebProcess`,
which is called when `WKPDFView` is still being used as the custom content view. This ultimately
causes the symptoms observed in this bug, which include blank tiles when scrolling.

To address this bug, we first revert the changes in r290875, which allows `WKPDFView` to behave as
if it were in the foreground, even when it's not in the view hierarchy:

```
- (void)willMoveToWindow:(UIWindow *)newWindow
{
    if (!self._contentView.window || newWindow)
        return;
    …
```

Of course, this (by itself) would bring back <https://webkit.org/b/237505>; to preserve that fix, we
additionally limit the early return to cases where the window is *not* in the process of being
destroyed (that is, `-willMoveToWindow:` is called with both the new `window == nil`, while the
current window `self.window == nil`):

```
- (void)willMoveToWindow:(UIWindow *)newWindow
{
    BOOL windowIsBeingDeallocated = !self.window && !newWindow;
    if (!windowIsBeingDeallocated) {
        if (!self._contentView.window || newWindow)
            return;
    }
    …
```

Note that this is logically equivalent to:

```
- (void)willMoveToWindow:(UIWindow *)newWindow
{
    if ((self.window || newWindow) && (!self._contentView.window || newWindow))
        return;
    …
```

...which can also be rewritten a bit more succinctly as:

```
- (void)willMoveToWindow:(UIWindow *)newWindow
{
    if ((self.window && !self._contentView.window) || newWindow)
        return;
    …
```

In other words:

- In the case where a standard content view is used (i.e. `self == self._contentView`), we always
clear out `_applicationStateTracker` if the window we're moving to is `nil`. This takes care of the
normal scenario of unparenting a web view, as well as the corner case where the web view is
unparented in the middle of `-[UIWindow dealloc]`.

- In the case of `PDFHostViewController` where a custom content view is used, we'll return early
in `-willMoveToWindow:` and keep the existing `_applicationStateTracker`, because the size tracking
view has not been parented yet.

Test: ApplicationStateTracking.NavigatingFromPDFDoesNotLeaveWebViewInactive

* Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:
(-[WKApplicationStateTrackingView willMoveToWindow:]):
* Tools/TestWebKitAPI/Configurations/Base.xcconfig:

Add `Source/WebKit/Platform/spi/ios` as a relative header include path for iOS family, such that we
can import the internal `PDFKitSPI.h` in API tests.

* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/ios/ApplicationStateTracking.mm:
(TestWebKitAPI::TEST):

Introduce a new API test that loads a simple webpage, navigates to a PDF document (and uses a remote
PDFKit view controller to render it), and then navigate back to the original webpage.

* Tools/TestWebKitAPI/cocoa/TestWKWebView.mm:
(-[WKWebView _isBackground]): Deleted.

Remove a workaround for API tests that's no longer necessary, so that the new API test fails without
this fix. This workaround was added in r231663, but became unnecessary after r236989, which removed
the dependency on `-_isBackground`.

* Tools/TestWebKitAPI/ios/TestPDFHostViewController.h:
* Tools/TestWebKitAPI/ios/TestPDFHostViewController.mm: Added.

Add a helper class that allows us to swizzle out `+createHostView:forExtensionIdentifier:`, and
return a `PDFHostViewController` subclass, with a subset of method stubs required to avoid crashes
when running API tests. This allows us to simulate PDFKit remote view controller presentation in API
tests.

(-[TestPDFHostViewController setDelegate:]):
(-[TestPDFHostViewController setDocumentData:withScrollView:]):
(-[TestPDFHostViewController currentPageIndex]):
(-[TestPDFHostViewController pageCount]):
(-[TestPDFHostViewController minimumZoomScale]):
(-[TestPDFHostViewController maximumZoomScale]):
(-[TestPDFHostViewController findString:withOptions:]):
(-[TestPDFHostViewController cancelFindString]):
(-[TestPDFHostViewController cancelFindStringWithHighlightsCleared:]):
(-[TestPDFHostViewController focusOnSearchResultAtIndex:]):
(-[TestPDFHostViewController clearSearchHighlights]):
(-[TestPDFHostViewController goToPageIndex:]):
(-[TestPDFHostViewController updatePDFViewLayout]):
(-[TestPDFHostViewController gestureRecognizerShouldBegin:]):
(-[TestPDFHostViewController pageNumberIndicator]):
(-[TestPDFHostViewController snapshotViewRect:snapshotWidth:afterScreenUpdates:withResult:]):
(-[TestPDFHostViewController beginPDFViewRotation]):
(-[TestPDFHostViewController endPDFViewRotation]):
(TestWebKitAPI::swizzledCreateHostViewForExtensionIdentifier):
(TestWebKitAPI::createPDFHostViewControllerSwizzler):

Canonical link: https://commits.webkit.org/251484@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295479 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
whsieh committed Jun 13, 2022
1 parent 79ad9ed commit 11993dcb5c28f2984ff70aa8241dbce528d60c5e
Show file tree
Hide file tree
Showing 7 changed files with 217 additions and 14 deletions.
@@ -51,7 +51,7 @@ - (instancetype)initWithFrame:(CGRect)frame webView:(WKWebView *)webView

- (void)willMoveToWindow:(UIWindow *)newWindow
{
if (!_applicationStateTracker || newWindow)
if ((self.window && !self._contentView.window) || newWindow)
return;

auto page = [_webViewToTrack _page];
@@ -38,6 +38,7 @@ ALTERNATE_HEADER_SEARCH_PATHS = $(ALTERNATE_HEADER_SEARCH_PATHS_$(SDK_VARIANT));
ALTERNATE_HEADER_SEARCH_PATHS_iosmac = $(BUILT_PRODUCTS_DIR)$(WK_ALTERNATE_FRAMEWORKS_DIR)/usr/local/include

HEADER_SEARCH_PATHS = $(ALTERNATE_HEADER_SEARCH_PATHS) ${BUILT_PRODUCTS_DIR}/usr/local/include $(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders $(BUILT_PRODUCTS_DIR)/WebCoreTestSupport ${SRCROOT} $(SRCROOT)/../../Source/WebKit/Platform/cocoa;
HEADER_SEARCH_PATHS[sdk=embedded*] = $(inherited) $(SRCROOT)/../../Source/WebKit/Platform/spi/ios

GCC_NO_COMMON_BLOCKS = YES;
GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES) U_DISABLE_RENAMING=1 U_SHOW_CPLUSPLUS_API=0 $(GCC_PREPROCESSOR_DEFINITIONS_$(PLATFORM_NAME));
@@ -1129,6 +1129,7 @@
F4BFA68E1E4AD08000154298 /* LegacyDragAndDropTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = F4BFA68C1E4AD08000154298 /* LegacyDragAndDropTests.mm */; };
F4C2AB221DD6D95E00E06D5B /* enormous-video-with-sound.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4C2AB211DD6D94100E06D5B /* enormous-video-with-sound.html */; };
F4C8797F2059D8D3009CD00B /* ScrollViewInsetTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = F4C8797E2059D8D3009CD00B /* ScrollViewInsetTests.mm */; };
F4CB8A7D2856462B0017ECD3 /* TestPDFHostViewController.mm in Sources */ = {isa = PBXBuildFile; fileRef = F4CB8A7C285644FB0017ECD3 /* TestPDFHostViewController.mm */; };
F4CD74C620FDACFA00DE3794 /* text-with-async-script.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4CD74C520FDACF500DE3794 /* text-with-async-script.html */; };
F4CDF3D227E97C7E00191928 /* SpellCheckerDocumentTag.mm in Sources */ = {isa = PBXBuildFile; fileRef = F4CDF3D127E97C7E00191928 /* SpellCheckerDocumentTag.mm */; };
F4CF32802366552200D3AD07 /* EnterKeyHintTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = F4CF327F2366552200D3AD07 /* EnterKeyHintTests.mm */; };
@@ -3186,6 +3187,8 @@
F4BFA68C1E4AD08000154298 /* LegacyDragAndDropTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = LegacyDragAndDropTests.mm; sourceTree = "<group>"; };
F4C2AB211DD6D94100E06D5B /* enormous-video-with-sound.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "enormous-video-with-sound.html"; sourceTree = "<group>"; };
F4C8797E2059D8D3009CD00B /* ScrollViewInsetTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = ScrollViewInsetTests.mm; sourceTree = "<group>"; };
F4CB8A7B2856447F0017ECD3 /* TestPDFHostViewController.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = TestPDFHostViewController.h; sourceTree = "<group>"; };
F4CB8A7C285644FB0017ECD3 /* TestPDFHostViewController.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = TestPDFHostViewController.mm; sourceTree = "<group>"; };
F4CD74C520FDACF500DE3794 /* text-with-async-script.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "text-with-async-script.html"; sourceTree = "<group>"; };
F4CD74C720FDB49600DE3794 /* TestURLSchemeHandler.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = TestURLSchemeHandler.h; sourceTree = "<group>"; };
F4CD74C820FDB49600DE3794 /* TestURLSchemeHandler.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = TestURLSchemeHandler.mm; sourceTree = "<group>"; };
@@ -3814,6 +3817,8 @@
F4D4F3B41E4E2BCB00BB2767 /* DragAndDropSimulatorIOS.mm */,
2E7765CC16C4D80A00BA2BB1 /* mainIOS.mm */,
F48D6C0F224B377000E3E2FB /* PreferredContentMode.mm */,
F4CB8A7B2856447F0017ECD3 /* TestPDFHostViewController.h */,
F4CB8A7C285644FB0017ECD3 /* TestPDFHostViewController.mm */,
F4D5C55627C6EBDC00ED1C23 /* TestUIMenuBuilder.h */,
F4D5C55727C6EBDC00ED1C23 /* TestUIMenuBuilder.mm */,
F4517B652054C49500C26721 /* TestWKWebViewController.h */,
@@ -5914,6 +5919,7 @@
F45E15762112CE6200307E82 /* TestInputDelegate.mm in Sources */,
F45D3891215A7B4B002A2979 /* TestInspectorBar.mm in Sources */,
516281252325C18000BB7E42 /* TestPDFDocument.mm in Sources */,
F4CB8A7D2856462B0017ECD3 /* TestPDFHostViewController.mm in Sources */,
7B774906267CCE72009873B4 /* TestRunnerTests.cpp in Sources */,
F4D5C55827C6FF4800ED1C23 /* TestUIMenuBuilder.mm in Sources */,
F4517B672054C49500C26721 /* TestWKWebViewController.mm in Sources */,
@@ -27,7 +27,13 @@

#if PLATFORM(IOS_FAMILY)

#import "ClassMethodSwizzler.h"
#import "PlatformUtilities.h"
#import "TestNavigationDelegate.h"
#import "TestPDFHostViewController.h"
#import "TestProtocol.h"
#import "TestWKWebView.h"
#import "WKWebViewConfigurationExtras.h"

namespace TestWebKitAPI {

@@ -59,6 +65,36 @@
[webView waitForNextPresentationUpdate];
}

#if HAVE(PDFKIT)

TEST(ApplicationStateTracking, NavigatingFromPDFDoesNotLeaveWebViewInactive)
{
RetainPtr configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];

auto pdfHostViewControllerSwizzler = createPDFHostViewControllerSwizzler();
auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 568) configuration:configuration.get()]);
auto delegate = adoptNS([TestNavigationDelegate new]);
[webView setNavigationDelegate:delegate.get()];

[webView loadSimulatedRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"https://foo.com"]] responseHTMLString:@"<body>Hello world</body>"];
[delegate waitForDidFinishNavigation];
[webView waitForNextPresentationUpdate];

RetainPtr fakeURL = [NSURL URLWithString:@"https://bar.com"];
RetainPtr pdfData = [NSData dataWithContentsOfURL:[[NSBundle mainBundle] URLForResource:@"test" withExtension:@"pdf" subdirectory:@"TestWebKitAPI.resources"]];
auto response = adoptNS([[NSURLResponse alloc] initWithURL:fakeURL.get() MIMEType:@"application/pdf" expectedContentLength:[pdfData length] textEncodingName:nil]);
[webView loadSimulatedRequest:[NSURLRequest requestWithURL:fakeURL.get()] response:response.get() responseData:pdfData.get()];
[delegate waitForDidFinishNavigation];

[webView goBack];
[delegate waitForDidFinishNavigation];
[webView waitForNextPresentationUpdate];

EXPECT_TRUE([[webView objectByEvaluatingJavaScript:@"internals.isPageActive()"] boolValue]);
}

#endif // HAVE(PDFKIT)

} // namespace TestWebKitAPI

#endif // PLATFORM(IOS_FAMILY)
@@ -58,19 +58,7 @@
{
return @"com.apple.TestWebKitAPI";
}

@implementation WKWebView (WKWebViewTestingQuirks)

// TestWebKitAPI is currently not a UIApplication so we are unable to track if it is in
// the background or not (https://bugs.webkit.org/show_bug.cgi?id=175204). This can
// cause our processes to get suspended on iOS. We work around this by having
// WKWebView._isBackground always return NO in the context of API tests.
- (BOOL)_isBackground
{
return NO;
}
@end
#endif
#endif // PLATFORM(IOS_FAMILY)

@implementation WKWebView (TestWebKitAPI)

@@ -0,0 +1,38 @@
/*
* Copyright (C) 2022 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
* THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
* PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
* BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
* THE POSSIBILITY OF SUCH DAMAGE.
*/

#pragma once

#if HAVE(PDFKIT) && PLATFORM(IOS_FAMILY)

class ClassMethodSwizzler;

namespace TestWebKitAPI {

std::unique_ptr<ClassMethodSwizzler> createPDFHostViewControllerSwizzler();

} // namespace TestWebKitAPI

#endif // HAVE(PDFKIT) && PLATFORM(IOS_FAMILY)
@@ -0,0 +1,134 @@
/*
* Copyright (C) 2022 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
* THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
* PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
* BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
* THE POSSIBILITY OF SUCH DAMAGE.
*/

#import "config.h"
#import "TestPDFHostViewController.h"

#if HAVE(PDFKIT) && PLATFORM(IOS_FAMILY)

#import "ClassMethodSwizzler.h"
#import "PDFKitSPI.h"

@interface TestPDFHostViewController : PDFHostViewController

@end

@implementation TestPDFHostViewController

- (void)setDelegate:(id)delegate
{
}

- (void)setDocumentData:(NSData *)data withScrollView:(UIScrollView *)scrollView
{
}

- (NSInteger)currentPageIndex
{
return 0;
}

- (NSInteger)pageCount
{
return 1;
}

- (CGFloat)minimumZoomScale
{
return 1;
}

- (CGFloat)maximumZoomScale
{
return 1;
}

- (void)findString:(NSString *)string withOptions:(NSStringCompareOptions)options
{
}

- (void)cancelFindString
{
}

- (void)cancelFindStringWithHighlightsCleared:(BOOL)clearHighlights
{
}

- (void)focusOnSearchResultAtIndex:(NSUInteger)searchIndex
{
}

- (void)clearSearchHighlights
{
}

- (void)goToPageIndex:(NSInteger)pageIndex
{
}

- (void)updatePDFViewLayout
{
}

- (BOOL)gestureRecognizerShouldBegin:(UIGestureRecognizer *)gestureRecognizer
{
return NO;
}

- (UIView *)pageNumberIndicator
{
return [[UIView new] autorelease];
}

- (void)snapshotViewRect:(CGRect)rect snapshotWidth:(NSNumber *)width afterScreenUpdates:(BOOL)afterScreenUpdates withResult:(void (^)(UIImage *image))completion
{
}

- (void)beginPDFViewRotation
{
}

- (void)endPDFViewRotation
{
}

@end

namespace TestWebKitAPI {

static void swizzledCreateHostViewForExtensionIdentifier(id, SEL, void (^callback)(PDFHostViewController *), NSString *)
{
callback([[TestPDFHostViewController new] autorelease]);
}

std::unique_ptr<ClassMethodSwizzler> createPDFHostViewControllerSwizzler()
{
return WTF::makeUnique<ClassMethodSwizzler>([PDFHostViewController class], @selector(createHostView:forExtensionIdentifier:), reinterpret_cast<IMP>(swizzledCreateHostViewForExtensionIdentifier));
}

} // namespace TestWebKitAPI

#endif // HAVE(PDFKIT) && PLATFORM(IOS_FAMILY)

0 comments on commit 11993dc

Please sign in to comment.