Bug 208365

Created attachment 391954 [details] Patch

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 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 on attachment 391954 [details] Patch Clearing flags on attachment: 391954 Committed r257618: <https://trac.webkit.org/changeset/257618>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59877296>

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>

Created attachment 392281 [details] Patch

Created attachment 392362 [details] Patch

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?

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

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

Also, did you want to set r??

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

Created attachment 392426 [details] Patch

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

Created attachment 392675 [details] Patch

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

Created attachment 392739 [details] Patch for landing

Comment on attachment 392739 [details] Patch for landing Clearing flags on attachment: 392739 Committed r258009: <https://trac.webkit.org/changeset/258009>

All reviewed patches have been landed. Closing bug.