Bug 208365 – UIProcess crash after using _prepareForMoveToWindow, then deallocating the WKWebView before moving to the window

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

UIProcess crash after using _prepareForMoveToWindow, then deallocating the WKWebView before moving to the window

[Tim Horton]

Created attachment 391954 [details]
Patch

[Alex Christensen]

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?

[Tim Horton]

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).

[WebKit Commit Bot]

Comment on attachment 391954 [details]
Patch

Clearing flags on attachment: 391954

Committed r257618: <https://trac.webkit.org/changeset/257618>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59877296>

[Wenson Hsieh]

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>

[Sihui Liu]

Created attachment 392281 [details]
Patch

[Sihui Liu]

Created attachment 392362 [details]
Patch

[Tim Horton]

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?

[Sihui Liu]

(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?

[Tim Horton]

(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.

[Tim Horton]

Also, did you want to set r??

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 392426 [details]
Patch

[Sihui Liu]

(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?

[Geoffrey Garen]

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

[Sihui Liu]

Created attachment 392675 [details]
Patch

[Geoffrey Garen]

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?

[Tim Horton]

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

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 392739 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 392739 [details]
Patch for landing

Clearing flags on attachment: 392739

Committed r258009: <https://trac.webkit.org/changeset/258009>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.