Bug 190098 - WebDriver: thrown ObjC exception under -[WKFullScreenWindowController windowDidFailToEnterFullScreen:] when session is terminated
Summary: WebDriver: thrown ObjC exception under -[WKFullScreenWindowController windowD...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-28 16:23 PDT by BJ Burg
Modified: 2018-10-09 14:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.76 KB, patch)
2018-10-09 10:24 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (10.76 KB, patch)
2018-10-09 10:39 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (10.77 KB, patch)
2018-10-09 14:07 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2018-09-28 16:23:48 PDT
This seems to happen when running the full Selenium suite and it tries to tear down the session.

It finally falls apart when cleaning up after window_tests.py::test_should_minimize_the_current_window.
Comment 1 BJ Burg 2018-09-28 16:24:34 PDT
<rdar://problem/42822671>
Comment 2 Jer Noble 2018-10-09 10:24:44 PDT
Created attachment 351887 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-10-09 10:33:44 PDT
Comment on attachment 351887 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Forcibly exit fullscreen in resetState(), while the VideoFullscreenManagerProxy is still valid.

Should we be catching Objective-C exceptions in more places?
Comment 4 Jer Noble 2018-10-09 10:39:55 PDT
Created attachment 351889 [details]
Patch
Comment 5 Jer Noble 2018-10-09 10:43:11 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 351887 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351887&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        Forcibly exit fullscreen in resetState(), while the VideoFullscreenManagerProxy is still valid.
> 
> Should we be catching Objective-C exceptions in more places?

This is a C++ problem; we're calling a null pointer (because the VideoFullscreenManager has been cleared). So in addition to this patch, we should probably add a bunch of (self._manager) null checks in functions which use it.
Comment 6 BJ Burg 2018-10-09 11:59:28 PDT
Comment on attachment 351887 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CloseWebViewAfterEnterFullscreen.mm:79
> +

Nit: After
Comment 7 BJ Burg 2018-10-09 13:31:01 PDT
Comment on attachment 351889 [details]
Patch

r=me
Comment 8 Jer Noble 2018-10-09 14:02:05 PDT
Comment on attachment 351889 [details]
Patch

Just realized that this test will likely not succeed on iOS, since the fullscreen delegate is different there. I'll put up a new patch which runs the test only on Mac.
Comment 9 Jer Noble 2018-10-09 14:07:23 PDT
Created attachment 351911 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-10-09 14:45:23 PDT
Comment on attachment 351911 [details]
Patch for landing

Clearing flags on attachment: 351911

Committed r236984: <https://trac.webkit.org/changeset/236984>
Comment 11 WebKit Commit Bot 2018-10-09 14:45:25 PDT
All reviewed patches have been landed.  Closing bug.