⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 244819 in webkit


Ignore:
Timestamp:
Apr 30, 2019, 7:45:10 PM (7 years ago)
Author:
Chris Dumez
Message:

Regression(PSON) URL scheme handlers can no longer respond asynchronously
​https://bugs.webkit.org/show_bug.cgi?id=197426
<rdar://problem/50256169>

Reviewed by Brady Eidson.

Source/WebKit:

The issue was that when committing the provisional process, we would call WebPageProxy::processDidTerminate()
which would call WebPageProxy::stopAllURLSchemeTasks(). This would terminate all URL scheme tasks associated
with the page, including the one associated with the provisisional page / process.

To address the issue, pass m_process to stopAllURLSchemeTasks() in processDidTerminate() and only stop the
tasks associated with the m_process (which is the process we're about to swap away from).

  • UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::processDidTerminate):
(WebKit::WebPageProxy::stopAllURLSchemeTasks):

  • UIProcess/WebPageProxy.h:
  • UIProcess/WebURLSchemeHandler.cpp:

(WebKit::WebURLSchemeHandler::processForTaskIdentifier):
(WebKit::WebURLSchemeHandler::stopAllTasksForPage):

  • UIProcess/WebURLSchemeHandler.h:
  • UIProcess/WebURLSchemeTask.h:

(WebKit::WebURLSchemeTask::process const):

Tools:

Add API test coverage.

  • TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

(-[PSONScheme setShouldRespondAsynchronously:]):
(-[PSONScheme webView:startURLSchemeTask:]):
(-[PSONScheme webView:stopURLSchemeTask:]):

Location:
trunk
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r244818 r244819  
     12019-04-30  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression(PSON) URL scheme handlers can no longer respond asynchronously
     4        https://bugs.webkit.org/show_bug.cgi?id=197426
     5        <rdar://problem/50256169>
     6
     7        Reviewed by Brady Eidson.
     8
     9        The issue was that when committing the provisional process, we would call WebPageProxy::processDidTerminate()
     10        which would call WebPageProxy::stopAllURLSchemeTasks(). This would terminate all URL scheme tasks associated
     11        with the page, including the one associated with the provisisional page / process.
     12
     13        To address the issue, pass m_process to stopAllURLSchemeTasks() in processDidTerminate() and only stop the
     14        tasks associated with the m_process (which is the process we're about to swap away from).
     15
     16        * UIProcess/WebPageProxy.cpp:
     17        (WebKit::WebPageProxy::processDidTerminate):
     18        (WebKit::WebPageProxy::stopAllURLSchemeTasks):
     19        * UIProcess/WebPageProxy.h:
     20        * UIProcess/WebURLSchemeHandler.cpp:
     21        (WebKit::WebURLSchemeHandler::processForTaskIdentifier):
     22        (WebKit::WebURLSchemeHandler::stopAllTasksForPage):
     23        * UIProcess/WebURLSchemeHandler.h:
     24        * UIProcess/WebURLSchemeTask.h:
     25        (WebKit::WebURLSchemeTask::process const):
     26
    1272019-04-30  John Wilander  <wilander@apple.com>
    228

  • trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

    r244812 r244819  
    67266726
    67276727    resetStateAfterProcessExited(reason);
     6728    stopAllURLSchemeTasks(m_process.ptr());
    67286729
    67296730    // For bringup of process swapping, NavigationSwap termination will not go out to clients.
    … …  
    67416742            automationSession->terminate();
    67426743    }
    6743 
    6744     stopAllURLSchemeTasks();
    67456744}
    67466745
    … …  
    68026801}
    68036802
    6804 void WebPageProxy::stopAllURLSchemeTasks()
     6803void WebPageProxy::stopAllURLSchemeTasks(WebProcessProxy* process)
    68056804{
    68066805    HashSet<WebURLSchemeHandler*> handlers;
    … …  
    68096808
    68106809    for (auto* handler : handlers)
    6811         handler->stopAllTasksForPage(*this);
     6810        handler->stopAllTasksForPage(*this, process);
    68126811}
    68136812

  • trunk/Source/WebKit/UIProcess/WebPageProxy.h

    r244812 r244819  
    20022002    void viewIsBecomingVisible();
    20032003
    2004     void stopAllURLSchemeTasks();
     2004    void stopAllURLSchemeTasks(WebProcessProxy* = nullptr);
    20052005
    20062006    void clearInspectorTargets();

  • trunk/Source/WebKit/UIProcess/WebURLSchemeHandler.cpp

    r244812 r244819  
    6161}
    6262
    63 void WebURLSchemeHandler::stopAllTasksForPage(WebPageProxy& page)
     63WebProcessProxy* WebURLSchemeHandler::processForTaskIdentifier(uint64_t taskIdentifier) const
     64{
     65    auto iterator = m_tasks.find(taskIdentifier);
     66    if (iterator == m_tasks.end())
     67        return nullptr;
     68    return iterator->value->process();
     69}
     70
     71void WebURLSchemeHandler::stopAllTasksForPage(WebPageProxy& page, WebProcessProxy* process)
    6472{
    6573    auto iterator = m_tasksByPageIdentifier.find(page.pageID());
    … …  
    6876
    6977    auto& tasksByPage = iterator->value;
    70     while (!tasksByPage.isEmpty())
    71         stopTask(page, *tasksByPage.begin());
     78    Vector<uint64_t> taskIdentifiersToStop;
     79    taskIdentifiersToStop.reserveInitialCapacity(tasksByPage.size());
     80    for (auto taskIdentifier : tasksByPage) {
     81        if (!process || processForTaskIdentifier(taskIdentifier) == process)
     82            taskIdentifiersToStop.uncheckedAppend(taskIdentifier);
     83    }
    7284
    73     ASSERT(m_tasksByPageIdentifier.find(page.pageID()) == m_tasksByPageIdentifier.end());
     85    for (auto& taskIdentifier : taskIdentifiersToStop)
     86        stopTask(page, taskIdentifier);
     87
    7488}
    7589

  • trunk/Source/WebKit/UIProcess/WebURLSchemeHandler.h

    r244812 r244819  
    5656    void startTask(WebPageProxy&, WebProcessProxy&, uint64_t taskIdentifier, WebCore::ResourceRequest&&, SyncLoadCompletionHandler&&);
    5757    void stopTask(WebPageProxy&, uint64_t taskIdentifier);
    58     void stopAllTasksForPage(WebPageProxy&);
     58    void stopAllTasksForPage(WebPageProxy&, WebProcessProxy*);
    5959    void taskCompleted(WebURLSchemeTask&);
    6060
    … …  
    6868
    6969    void removeTaskFromPageMap(uint64_t pageID, uint64_t taskID);
     70    WebProcessProxy* processForTaskIdentifier(uint64_t) const;
    7071
    7172    uint64_t m_identifier;

  • trunk/Source/WebKit/UIProcess/WebURLSchemeTask.h

    r244812 r244819  
    5959    uint64_t identifier() const { return m_identifier; }
    6060    uint64_t pageID() const { return m_pageIdentifier; }
     61    WebProcessProxy* process() const { return m_process.get(); }
    6162
    6263    const WebCore::ResourceRequest& request() const { return m_request; }

  • trunk/Tools/ChangeLog

    r244812 r244819  
     12019-04-30  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression(PSON) URL scheme handlers can no longer respond asynchronously
     4        https://bugs.webkit.org/show_bug.cgi?id=197426
     5        <rdar://problem/50256169>
     6
     7        Reviewed by Brady Eidson.
     8
     9        Add API test coverage.
     10
     11        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
     12        (-[PSONScheme setShouldRespondAsynchronously:]):
     13        (-[PSONScheme webView:startURLSchemeTask:]):
     14        (-[PSONScheme webView:stopURLSchemeTask:]):
     15
    1162019-04-30  Chris Dumez  <cdumez@apple.com>
    217

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

    r244812 r244819  
    4949#import <WebKit/_WKWebsiteDataStoreConfiguration.h>
    5050#import <WebKit/_WKWebsitePolicies.h>
     51#import <wtf/BlockPtr.h>
    5152#import <wtf/Deque.h>
    5253#import <wtf/HashMap.h>
    … …  
    239240    HashMap<String, String> _redirects;
    240241    HashMap<String, RetainPtr<NSData>> _dataMappings;
     242    HashSet<id <WKURLSchemeTask>> _runningTasks;
     243    bool _shouldRespondAsynchronously;
    241244}
    242245- (instancetype)initWithBytes:(const char*)bytes;
    … …  
    264267}
    265268
     269- (void)setShouldRespondAsynchronously:(BOOL)value
     270{
     271    _shouldRespondAsynchronously = value;
     272}
     273
    266274- (void)webView:(WKWebView *)webView startURLSchemeTask:(id <WKURLSchemeTask>)task
    267275{
     276    if ([(id<WKURLSchemeTaskPrivate>)task _requestOnlyIfCached]) {
     277        [task didFailWithError:[NSError errorWithDomain:@"TestWebKitAPI" code:1 userInfo:nil]];
     278        return;
     279    }
     280
     281    _runningTasks.add(task);
     282
     283    auto doAsynchronouslyIfNecessary = [self, strongSelf = retainPtr(self), task = retainPtr(task)](Function<void(id <WKURLSchemeTask>)>&& f, double delay) {
     284        if (!_shouldRespondAsynchronously)
     285            return f(task.get());
     286        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, delay * NSEC_PER_SEC), dispatch_get_main_queue(), makeBlockPtr([self, strongSelf, task, f = WTFMove(f)] {
     287            if (_runningTasks.contains(task.get()))
     288                f(task.get());
     289        }).get());
     290    };
     291
    268292    NSURL *finalURL = task.request.URL;
    269293    auto target = _redirects.get(task.request.URL.absoluteString);
    … …  
    277301    }
    278302
    279     if ([(id<WKURLSchemeTaskPrivate>)task _requestOnlyIfCached]) {
    280         [task didFailWithError:[NSError errorWithDomain:@"TestWebKitAPI" code:1 userInfo:nil]];
    281         return;
    282     }
    283 
    284     RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:finalURL MIMEType:@"text/html" expectedContentLength:1 textEncodingName:nil]);
    285     [task didReceiveResponse:response.get()];
    286 
    287     if (auto data = _dataMappings.get([finalURL absoluteString]))
    288         [task didReceiveData:data.get()];
    289     else if (_bytes) {
    290         RetainPtr<NSData> data = adoptNS([[NSData alloc] initWithBytesNoCopy:(void *)_bytes length:strlen(_bytes) freeWhenDone:NO]);
    291         [task didReceiveData:data.get()];
    292     } else
    293         [task didReceiveData:[@"Hello" dataUsingEncoding:NSUTF8StringEncoding]];
    294 
    295     [task didFinish];
     303    doAsynchronouslyIfNecessary([finalURL = retainPtr(finalURL)](id <WKURLSchemeTask> task) {
     304        RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:finalURL.get() MIMEType:@"text/html" expectedContentLength:1 textEncodingName:nil]);
     305        [task didReceiveResponse:response.get()];
     306    }, 0.1);
     307
     308    doAsynchronouslyIfNecessary([self, finalURL = retainPtr(finalURL)](id <WKURLSchemeTask> task) {
     309        if (auto data = _dataMappings.get([finalURL absoluteString]))
     310            [task didReceiveData:data.get()];
     311        else if (_bytes) {
     312            RetainPtr<NSData> data = adoptNS([[NSData alloc] initWithBytesNoCopy:(void *)_bytes length:strlen(_bytes) freeWhenDone:NO]);
     313            [task didReceiveData:data.get()];
     314        } else
     315            [task didReceiveData:[@"Hello" dataUsingEncoding:NSUTF8StringEncoding]];
     316    }, 0.2);
     317
     318    doAsynchronouslyIfNecessary([self](id <WKURLSchemeTask> task) {
     319        [task didFinish];
     320        _runningTasks.remove(task);
     321    }, 0.3);
    296322}
    297323
    298324- (void)webView:(WKWebView *)webView stopURLSchemeTask:(id <WKURLSchemeTask>)task
    299325{
     326    _runningTasks.remove(task);
    300327}
    301328
    … …  
    472499}
    473500
    474 TEST(ProcessSwap, Basic)
    475 {
    476     auto processPoolConfiguration = psonProcessPoolConfiguration();
    477     auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
    478 
    479     auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
    480     [webViewConfiguration setProcessPool:processPool.get()];
    481     auto handler = adoptNS([[PSONScheme alloc] init]);
     501enum class SchemeHandlerShouldBeAsync { No, Yes };
     502static void runBasicTest(SchemeHandlerShouldBeAsync schemeHandlerShouldBeAsync)
     503{
     504    auto processPoolConfiguration = psonProcessPoolConfiguration();
     505    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
     506
     507    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     508    [webViewConfiguration setProcessPool:processPool.get()];
     509    auto handler = adoptNS([[PSONScheme alloc] init]);
     510    [handler setShouldRespondAsynchronously:(schemeHandlerShouldBeAsync == SchemeHandlerShouldBeAsync::Yes)];
    482511    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
    483512
    … …  
    515544    // 3 loads, 3 decidePolicy calls (e.g. the load that did perform a process swap should not have generated an additional decidePolicy call)
    516545    EXPECT_EQ(numberOfDecidePolicyCalls, 3);
     546}
     547
     548TEST(ProcessSwap, Basic)
     549{
     550    runBasicTest(SchemeHandlerShouldBeAsync::No);
     551}
     552
     553TEST(ProcessSwap, BasicWithAsyncSchemeHandler)
     554{
     555    runBasicTest(SchemeHandlerShouldBeAsync::Yes);
    517556}
    518557
Note: See TracChangeset for help on using the changeset viewer.