Bug 208365 - UIProcess crash after using _prepareForMoveToWindow, then deallocating the WKWebView before moving to the window
Summary: UIProcess crash after using _prepareForMoveToWindow, then deallocating the WK...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-27 18:39 PST by Tim Horton
Modified: 2020-03-06 10:39 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.86 KB, patch)
2020-02-27 18:39 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (6.46 KB, patch)
2020-03-03 09:05 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (8.74 KB, patch)
2020-03-03 19:21 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (9.81 KB, patch)
2020-03-04 09:33 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (10.54 KB, patch)
2020-03-05 21:43 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (10.82 KB, patch)
2020-03-06 10:19 PST, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2020-02-27 18:39:16 PST
UIProcess crash after using _prepareForMoveToWindow, then deallocating the WKWebView before moving to the window
Comment 1 Tim Horton 2020-02-27 18:39:35 PST
Created attachment 391954 [details]
Patch
Comment 2 Alex Christensen 2020-02-27 21:04:17 PST
Comment on attachment 391954 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:204
> +    __weak NSWindow *_observingWindow;

Is this ARC code?  Does __weak do anything here?
Could you use WeakObjCPtr?
Comment 3 Tim Horton 2020-02-27 21:31:54 PST
Comment on attachment 391954 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:204
>> +    __weak NSWindow *_observingWindow;
> 
> Is this ARC code?  Does __weak do anything here?
> Could you use WeakObjCPtr?

__weak works AS IF BY MAGIC. (Or really because we have CLANG_ENABLE_OBJC_WEAK on).
Comment 4 WebKit Commit Bot 2020-02-27 22:21:26 PST
Comment on attachment 391954 [details]
Patch

Clearing flags on attachment: 391954

Committed r257618: <https://trac.webkit.org/changeset/257618>
Comment 5 WebKit Commit Bot 2020-02-27 22:21:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2020-02-27 22:22:24 PST
<rdar://problem/59877296>
Comment 7 Wenson Hsieh 2020-03-01 15:36:09 PST
Reverted r257618 for reason:

This allegedly causes Safari to crash when closing a window (see rdar://problem/59922725).

Committed r257694: <https://trac.webkit.org/changeset/257694>
Comment 8 Sihui Liu 2020-03-03 09:05:59 PST
Created attachment 392281 [details]
Patch
Comment 9 Sihui Liu 2020-03-03 19:21:36 PST
Created attachment 392362 [details]
Patch
Comment 10 Tim Horton 2020-03-03 19:30:14 PST
Comment on attachment 392362 [details]
Patch

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

> Tools/ChangeLog:9
> +        * TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm:

Is it possible to write a test for the window-close case?
Comment 11 Sihui Liu 2020-03-03 22:24:15 PST
(In reply to Tim Horton from comment #10)
> Comment on attachment 392362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392362&action=review
> 
> > Tools/ChangeLog:9
> > +        * TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm:
> 
> Is it possible to write a test for the window-close case?

By window-close case, you mean WKWebView.PrepareForMoveToWindowThenWindowDeallocBeforeMoving but removing the autoreleasepool? If window is closed but not released, looks like it does not cause trouble to our current code except may receive notifications sent from other windows?
Comment 12 Tim Horton 2020-03-03 22:35:48 PST
(In reply to Sihui Liu from comment #11)
> (In reply to Tim Horton from comment #10)
> > Comment on attachment 392362 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392362&action=review
> > 
> > > Tools/ChangeLog:9
> > > +        * TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm:
> > 
> > Is it possible to write a test for the window-close case?
> 
> By window-close case, you mean
> WKWebView.PrepareForMoveToWindowThenWindowDeallocBeforeMoving but removing
> the autoreleasepool? If window is closed but not released, looks like it
> does not cause trouble to our current code except may receive notifications
> sent from other windows?

I mean "a test that crashes with my patch and not with yours", I'm not sure what exactly is required to make that the case.
Comment 13 Tim Horton 2020-03-03 22:36:04 PST
Also, did you want to set r??
Comment 14 Sihui Liu 2020-03-04 09:03:10 PST
(In reply to Tim Horton from comment #12)
> (In reply to Sihui Liu from comment #11)
> > (In reply to Tim Horton from comment #10)
> > > Comment on attachment 392362 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=392362&action=review
> > > 
> > > > Tools/ChangeLog:9
> > > > +        * TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm:
> > > 
> > > Is it possible to write a test for the window-close case?
> > 
> > By window-close case, you mean
> > WKWebView.PrepareForMoveToWindowThenWindowDeallocBeforeMoving but removing
> > the autoreleasepool? If window is closed but not released, looks like it
> > does not cause trouble to our current code except may receive notifications
> > sent from other windows?
> 
> I mean "a test that crashes with my patch and not with yours", I'm not sure
> what exactly is required to make that the case.

WKWebView.PrepareForMoveToWindowThenViewDeallocBeforeMoving is based on your test. WKWebView.PrepareForMoveToWindowThenWindowDeallocBeforeMoving is the test case that would crash with your patch. 
Seems we need another tweak though. Will add a third test case.
Comment 15 Sihui Liu 2020-03-04 09:33:10 PST
Created attachment 392426 [details]
Patch
Comment 16 Sihui Liu 2020-03-04 15:28:25 PST
(In reply to Sihui Liu from comment #15)
> Created attachment 392426 [details]
> Patch

From the API test failure, looks like there can be a ref cycle if we keep strong ref of window in WKWindowVisibilityObserver. 

The observer does not know state of window being observed, and we have to remove observers before window deallocs.

Either we ask the client of prepareForMoveToWindow to cancel prepareForMoveToWindow/close the target window properly before window deallocs, or we don't set observer in viewWillMoveToWindow for window to move to.

Any idea, Tim?
Comment 17 Geoffrey Garen 2020-03-05 17:03:06 PST
Comment on attachment 392426 [details]
Patch

As Sihui explained in person, the API test failure indicates a retain cycle -- the cycle happens when you move to the window.

Tim and Sihui and I think this solution might work:

WebViewImpl::m_targetWindowForMovePreparation becomes a RetainPtr

	WebViewImpl::prepareForMoveToWindow does not clear m_targetWindowForMovePreparation after calling completion handler

	WebViewImpl::viewWillMoveToWindow: copy to WebViewImpl::viewWillMoveToWindowImpl

	WebViewImpl::viewWillMoveToWindow: calls viewWillMoveToWindowImpl and then clear m_targetWindowForMovePreparation

	WebViewImpl::viewDidMoveToWindow: copy to WebViewImpl::viewDidMoveToWindowImpl

	WebViewImpl::viewDidMoveToWindow: call WebViewImpl::viewDidMoveToWindowImpl

	WebViewImpl::prepareForMoveToWindow: call WebViewImpl::viewWillMoveToWindowImpl & WebViewImpl::viewDidMoveToWindowImpl
Comment 18 Sihui Liu 2020-03-05 21:43:37 PST
Created attachment 392675 [details]
Patch
Comment 19 Geoffrey Garen 2020-03-06 09:56:33 PST
Comment on attachment 392675 [details]
Patch

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

Looks right to me, and tests pass. Yay! I think this could use a round of style fixup before landing.

> Source/WebKit/ChangeLog:10
> +        not actually moves. Make WebView hold strong reference to the target window so it knows to stop observing at

does not actually move.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2043
> +        return m_targetWindowForMovePreparation.get().backingScaleFactor;

m_targetWindowForMovePreparation->backingScaleFactor

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2337
> +            m_colorSpace = m_targetWindowForMovePreparation.get().colorSpace;

m_targetWindowForMovePreparation->colorSpace

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2430
>      viewDidMoveToWindow();

Should this be viewDidMoveToWindowImpl? Or maybe you discovered that we only need that treatment for viewWillMoveToWindow?
Comment 20 Tim Horton 2020-03-06 09:59:59 PST
Comment on attachment 392675 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:301
> +    void viewWillMoveToWindowImpl(NSWindow *);

Can't this hide in private-land?

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2043
>> +        return m_targetWindowForMovePreparation.get().backingScaleFactor;
> 
> m_targetWindowForMovePreparation->backingScaleFactor

Actually [m_targetWindowForMovePreparation backingScaleFactor]

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2337
>> +            m_colorSpace = m_targetWindowForMovePreparation.get().colorSpace;
> 
> m_targetWindowForMovePreparation->colorSpace

see above
Comment 21 Sihui Liu 2020-03-06 10:18:49 PST
Comment on attachment 392675 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:301
>> +    void viewWillMoveToWindowImpl(NSWindow *);
> 
> Can't this hide in private-land?

It can!

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2430
>>      viewDidMoveToWindow();
> 
> Should this be viewDidMoveToWindowImpl? Or maybe you discovered that we only need that treatment for viewWillMoveToWindow?

Yes, there is nothing different about viewDidMoveToWindow in preparation.
Comment 22 Sihui Liu 2020-03-06 10:19:11 PST
Created attachment 392739 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2020-03-06 10:39:20 PST
Comment on attachment 392739 [details]
Patch for landing

Clearing flags on attachment: 392739

Committed r258009: <https://trac.webkit.org/changeset/258009>
Comment 24 WebKit Commit Bot 2020-03-06 10:39:22 PST
All reviewed patches have been landed.  Closing bug.