...
rdar://problem/67097019
Created attachment 407620 [details] Patch
Comment on attachment 407620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407620&action=review > Source/WebKit/ChangeLog:14 > + In the swithcing case, view to be unselected will detach from root layer in its web process, which makes its typo: "switching" > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:495 > + RefPtr<WebPageProxy> strongThis(weakThis.get()); Better style: auto strongThis = makeRefPtr(weakThis.get()); Also seems like we should make makeRefPtr smarter so the "get()" is not required. > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:499 > + m_hasScheduledActivityStateUpdate = false; > + strongThis->dispatchActivityStateChange(); Is suggest either using strongThis for both of the above, or for neither. If we use it for both, then we don’t need to capture "this" at all, and that’s probably the most elegant version, and clearest on object lifetime. > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:146 > + m_shouldHandleActivityStateChangeCallbacks = true; I think you mean "false" here?
Created attachment 407621 [details] Patch
(In reply to Darin Adler from comment #3) > Comment on attachment 407620 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407620&action=review > > > Source/WebKit/ChangeLog:14 > > + In the swithcing case, view to be unselected will detach from root layer in its web process, which makes its > > typo: "switching" > Will update. > > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:495 > > + RefPtr<WebPageProxy> strongThis(weakThis.get()); > > Better style: > > auto strongThis = makeRefPtr(weakThis.get()); > Okay. > Also seems like we should make makeRefPtr smarter so the "get()" is not > required. > > > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:499 > > + m_hasScheduledActivityStateUpdate = false; > > + strongThis->dispatchActivityStateChange(); > > Is suggest either using strongThis for both of the above, or for neither. If > we use it for both, then we don’t need to capture "this" at all, and that’s > probably the most elegant version, and clearest on object lifetime. > Sure, will remove "this". > > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:146 > > + m_shouldHandleActivityStateChangeCallbacks = true; > > I think you mean "false" here? Yes! Will change.
Comment on attachment 407621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407621&action=review > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:146 > + if (!m_shouldHandleActivityStateChangeCallbacks) > + return; > + m_shouldHandleActivityStateChangeCallbacks = true; This doesn’t make logical sense. Why do we have to set something to true since we just exited if false.
Oh, I see your response now.
Created attachment 407624 [details] Patch
Comment on attachment 407624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407624&action=review > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:142 > + if (!weakThis) > + return; Why don’t we need the strongThis approach here?
(In reply to Darin Adler from comment #9) > Comment on attachment 407624 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407624&action=review > > > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:142 > > + if (!weakThis) > > + return; > > Why don’t we need the strongThis approach here? Because TiledCoreAnimationDrawingArea is owned by WebPageProxy and is less likely get destroyed in the commit handlers?
(In reply to Sihui Liu from comment #10) > (In reply to Darin Adler from comment #9) > > Comment on attachment 407624 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=407624&action=review > > > > > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:142 > > > + if (!weakThis) > > > + return; > > > > Why don’t we need the strongThis approach here? > > Because TiledCoreAnimationDrawingArea is owned by WebPageProxy and is less > likely get destroyed in the commit handlers? Still no guarantee, though, is there? Probably should just do it, or we'll be here fixing a rare crashtracer in a year or two :)
Should the title be "switching between tabs"?
Comment on attachment 407624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407624&action=review >>>> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:142 >>>> + return; >>> >>> Why don’t we need the strongThis approach here? >> >> Because TiledCoreAnimationDrawingArea is owned by WebPageProxy and is less likely get destroyed in the commit handlers? > > Still no guarantee, though, is there? Probably should just do it, or we'll be here fixing a rare crashtracer in a year or two :) Yes, "less likely" is not how to deal with object lifetime.
(In reply to Darin Adler from comment #13) > Comment on attachment 407624 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407624&action=review > > >>>> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:142 > >>>> + return; > >>> > >>> Why don’t we need the strongThis approach here? > >> > >> Because TiledCoreAnimationDrawingArea is owned by WebPageProxy and is less likely get destroyed in the commit handlers? > > > > Still no guarantee, though, is there? Probably should just do it, or we'll be here fixing a rare crashtracer in a year or two :) > > Yes, "less likely" is not how to deal with object lifetime. Okay, will update this.
Created attachment 407631 [details] Patch
(In reply to Simon Fraser (smfr) from comment #12) > Should the title be "switching between tabs"? I considered about this and found tab is something API clients may use to place web views. WebKit seems to only care about window. What do you think?
Comment on attachment 407631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407631&action=review I can't r+ but it looks reasonable to me :P > Source/WebKit/ChangeLog:14 > + In the swithcing case, view to be unselected will detach from root layer in its web process, which makes its switching is still misspelled
Created attachment 407632 [details] Patch
Comment on attachment 407632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407632&action=review Can we find a way to make a regression test for this? We want this fixed for the long term, not just fixed now and then broken in the future. Tests the main way we do that. We also need a test that would have detected our mistake before where m_shouldHandleActivityStateChangeCallbacks was never set back to false. > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:144 > + auto strongPage = makeRefPtr(weakThis->m_webPage); If we’re going to be dereferencing this without null checking, then I suggest putting this into a Ref rather than a RefPtr. auto strongPage = makeRef(*weakThis->m_webPage); > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:145 > + auto* drawingArea = static_cast<TiledCoreAnimationDrawingArea*>(strongPage->drawingArea()); If we’re going to be dereferencing this without null checking, then I suggest putting this into a reference rather than a pointer: auto& drawingArea = static_cast<TiledCoreAnimationDrawingArea&>(*strongPage->drawingArea()); > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:155 > + for (const auto& callbackID : drawingArea->m_nextActivityStateChangeCallbackIDs) Should just use auto here, not const auto&. Seems fine to copy an ID.
Comment on attachment 407632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407632&action=review > Source/WebKit/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Tim already reviewed this, so it’s strange that I "reviewed" it.
(In reply to Darin Adler from comment #20) > Comment on attachment 407632 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407632&action=review > > > Source/WebKit/ChangeLog:6 > > + Reviewed by NOBODY (OOPS!). > > Tim already reviewed this, so it’s strange that I "reviewed" it. Tim did not r+ because he wrote the original patch, so it's not strange!
(In reply to Darin Adler from comment #19) > Comment on attachment 407632 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407632&action=review > > Can we find a way to make a regression test for this? We want this fixed for > the long term, not just fixed now and then broken in the future. Tests the > main way we do that. +1 since at least part of this is a regression (and I expect all of it is, in the longer term). But it's going to be tricky :P > We also need a test that would have detected our mistake before where > m_shouldHandleActivityStateChangeCallbacks was never set back to false. I'm pretty surprised none of the tests that test activity state changes failed.
Comment on attachment 407632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407632&action=review >> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:144 >> + auto strongPage = makeRefPtr(weakThis->m_webPage); > > If we’re going to be dereferencing this without null checking, then I suggest putting this into a Ref rather than a RefPtr. > > auto strongPage = makeRef(*weakThis->m_webPage); Sure, will use Ref. >> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:145 >> + auto* drawingArea = static_cast<TiledCoreAnimationDrawingArea*>(strongPage->drawingArea()); > > If we’re going to be dereferencing this without null checking, then I suggest putting this into a reference rather than a pointer: > > auto& drawingArea = static_cast<TiledCoreAnimationDrawingArea&>(*strongPage->drawingArea()); Will add a check drawingArea != weakThis.get() here, since m_webPage above is a reference and drawingArea() returns a raw pointer. >> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:155 >> + for (const auto& callbackID : drawingArea->m_nextActivityStateChangeCallbackIDs) > > Should just use auto here, not const auto&. Seems fine to copy an ID. Sure.
(In reply to Tim Horton from comment #22) > (In reply to Darin Adler from comment #19) > > Comment on attachment 407632 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=407632&action=review > > > > Can we find a way to make a regression test for this? We want this fixed for > > the long term, not just fixed now and then broken in the future. Tests the > > main way we do that. > > +1 since at least part of this is a regression (and I expect all of it is, > in the longer term). But it's going to be tricky :P > Do you mean tests for finding the flash, or test to make sure the ordering of events is correct? > > We also need a test that would have detected our mistake before where > > m_shouldHandleActivityStateChangeCallbacks was never set back to false. > > I'm pretty surprised none of the tests that test activity state changes > failed. In that case we just try handleActivityStateChangeCallbacks on every commit, which seems ... not harmful if we don't have callback?
(In reply to Sihui Liu from comment #24) > (In reply to Tim Horton from comment #22) > > (In reply to Darin Adler from comment #19) > > > Comment on attachment 407632 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=407632&action=review > > > > > > Can we find a way to make a regression test for this? We want this fixed for > > > the long term, not just fixed now and then broken in the future. Tests the > > > main way we do that. > > > > +1 since at least part of this is a regression (and I expect all of it is, > > in the longer term). But it's going to be tricky :P > > > > Do you mean tests for finding the flash, or test to make sure the ordering > of events is correct? It might actually not be possible to catch the flash, since the only way the WP update leap-frogs the UI process is via the fact that it's directly connected to the window server and doesn't have to go through the UI process. /Maybe/ you can do it like you say, by following the order of events. > > > We also need a test that would have detected our mistake before where > > > m_shouldHandleActivityStateChangeCallbacks was never set back to false. > > > > I'm pretty surprised none of the tests that test activity state changes > > failed. > > In that case we just try handleActivityStateChangeCallbacks on every commit, > which seems ... not harmful if we don't have callback? Oh! It's the opposite of what I thought. Yeah, OK. And that sounds untestable too.
(In reply to Tim Horton from comment #25) > (In reply to Sihui Liu from comment #24) > > Do you mean tests for finding the flash, or test to make sure the ordering > > of events is correct? > > It might actually not be possible to catch the flash, since the only way the > WP update leap-frogs the UI process is via the fact that it's directly > connected to the window server and doesn't have to go through the UI > process. /Maybe/ you can do it like you say, by following the order of > events. I want to state the obvious: The goal of the test is to fail if we re-introduce the flash. We can be as tricky as we need to be about how we catch the mistake; it doesn’t have to literally prove we have a flash. > > > > We also need a test that would have detected our mistake before where > > > > m_shouldHandleActivityStateChangeCallbacks was never set back to false. > > > > > > I'm pretty surprised none of the tests that test activity state changes > > > failed. > > > > In that case we just try handleActivityStateChangeCallbacks on every commit, > > which seems ... not harmful if we don't have callback? > > Oh! It's the opposite of what I thought. Yeah, OK. And that sounds > untestable too. If doing it every time is not bad performance they why do we bother with the dirty bit?
Created attachment 407648 [details] Patch for landing
Committed r266384: <https://trac.webkit.org/changeset/266384> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407648 [details].
Reverted r266384 for reason: Revision is causing 5 API faliures/timeouts and build failures on macOS Committed r266412: <https://trac.webkit.org/changeset/266412>
Created attachment 407805 [details] Patch
Created attachment 407816 [details] Patch
Created attachment 407817 [details] Patch
ping for review...
Comment on attachment 407817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407817&action=review > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:494 > + bool hasScheduledOnObserver = m_activityStateChangeDispatcher->isScheduled(); "on"? > Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:520 > + if (![CATransaction currentState]) { Both of the places you check this should have comments, since it's very very weird. Maybe we should just adjust the tests instead? Is this a state you can ever get to in "reality"?
Comment on attachment 407817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407817&action=review >> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:494 >> + bool hasScheduledOnObserver = m_activityStateChangeDispatcher->isScheduled(); > > "on"? I was using hasScheduledOnObserver and hasScheduledOnCommitHandler; will remove >> Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:520 >> + if (![CATransaction currentState]) { > > Both of the places you check this should have comments, since it's very very weird. > > Maybe we should just adjust the tests instead? Is this a state you can ever get to in "reality"? According to https://developer.apple.com/documentation/quartzcore/catransaction?language=objc, implicit transactions are created when layer tree is modified without an active transaction, so theoretically this could happen either we already do the change in an explicit transaction, or there is no layer tree change? The tests seem correct, at least for WKWebView.PrepareForMoveToWindow, which just calls _prepareForMoveToWindow and ensures callback is invoked.
Created attachment 408003 [details] Patch
Committed r266634: <https://trac.webkit.org/changeset/266634> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408003 [details].