Bug 171090 - WebContent process becomes unresponsive after returning nil from async version of -webView:createWebViewWithConfiguration:...
Summary: WebContent process becomes unresponsive after returning nil from async versio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-20 16:54 PDT by Brady Eidson
Modified: 2017-04-20 18:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.82 KB, patch)
2017-04-20 17:00 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2017-04-20 17:20 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-04-20 16:54:55 PDT
WebContent process becomes unresponsive after returning nil from async version of -webView:createWebViewWithConfiguration:...

<rdar://problem/31739023>
Comment 1 Brady Eidson 2017-04-20 17:00:10 PDT
Created attachment 307664 [details]
Patch
Comment 2 Sam Weinig 2017-04-20 17:03:04 PDT
Comment on attachment 307664 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:198
> +            if (webView && [webView->_configuration _relatedWebView] != relatedWebView.get())
>                  [NSException raise:NSInternalInconsistencyException format:@"Returned WKWebView was not created with the given configuration."];

If this exception is caught, does it leave the WebProcess in an unresponsive state as well?
Comment 3 Andy Estes 2017-04-20 17:03:45 PDT
Comment on attachment 307664 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:-198
> -            if (!webView)
> -                return;

Can you call completionHandler with nullptr here and then not bother with the null checks below?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/OpenAndCloseWindow.mm:180
>  // https://bugs.webkit.org/show_bug.cgi?id=171083 - Try to figure out why this fails for some configs but not others, and resolve.
>  //TEST(WebKit2, OpenAndCloseWindowAsyncCallbackException)
>  //{
> -//    openedWebView = nullptr;
> -//    isDone = false;
> +//    resetToConsistentState();
>  //
>  //    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
>  //

Commented out code!
Comment 4 Brady Eidson 2017-04-20 17:05:53 PDT
Comment on attachment 307664 [details]
Patch

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

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:198
>>                  [NSException raise:NSInternalInconsistencyException format:@"Returned WKWebView was not created with the given configuration."];
> 
> If this exception is caught, does it leave the WebProcess in an unresponsive state as well?

No.

Though it's a good point here...  should we allow a bogusly created WebView through just because the app caught the exception?

Same possibility has always existed for the synchronous form of this API, so it's not a new issue.
Comment 5 Brady Eidson 2017-04-20 17:06:22 PDT
(In reply to Andy Estes from comment #3)
> Comment on attachment 307664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307664&action=review
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:-198
> > -            if (!webView)
> > -                return;
> 
> Can you call completionHandler with nullptr here and then not bother with
> the null checks below?

Yup!
Comment 6 Brady Eidson 2017-04-20 17:18:33 PDT
(In reply to Brady Eidson from comment #4)
> Comment on attachment 307664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307664&action=review
> 
> >> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:198
> >>                  [NSException raise:NSInternalInconsistencyException format:@"Returned WKWebView was not created with the given configuration."];
> > 
> > If this exception is caught, does it leave the WebProcess in an unresponsive state as well?
> 
> No.
> 
> Though it's a good point here...  should we allow a bogusly created WebView
> through just because the app caught the exception?
> 
> Same possibility has always existed for the synchronous form of this API, so
> it's not a new issue.


Nevermind, it would totally hang and you'd be left in a completely inconsistent state...

But same thing for the sync version of this API, as well.
Comment 7 Brady Eidson 2017-04-20 17:20:56 PDT
Created attachment 307667 [details]
Patch
Comment 8 WebKit Commit Bot 2017-04-20 18:02:51 PDT
Comment on attachment 307667 [details]
Patch

Clearing flags on attachment: 307667

Committed r215598: <http://trac.webkit.org/changeset/215598>
Comment 9 WebKit Commit Bot 2017-04-20 18:02:52 PDT
All reviewed patches have been landed.  Closing bug.