RESOLVED FIXED 216012
Webpages flash when switching between windows
https://bugs.webkit.org/show_bug.cgi?id=216012
Summary Webpages flash when switching between windows
Sihui Liu
Reported 2020-08-31 13:34:31 PDT
...
Attachments
Patch (11.27 KB, patch)
2020-08-31 13:42 PDT, Sihui Liu
no flags
Patch (11.30 KB, patch)
2020-08-31 13:48 PDT, Sihui Liu
no flags
Patch (11.57 KB, patch)
2020-08-31 14:09 PDT, Sihui Liu
no flags
Patch (11.85 KB, patch)
2020-08-31 15:38 PDT, Sihui Liu
no flags
Patch (11.85 KB, patch)
2020-08-31 15:46 PDT, Sihui Liu
no flags
Patch for landing (11.90 KB, patch)
2020-08-31 19:04 PDT, Sihui Liu
no flags
Patch (12.69 KB, patch)
2020-09-02 14:04 PDT, Sihui Liu
no flags
Patch (13.83 KB, patch)
2020-09-02 14:30 PDT, Sihui Liu
no flags
Patch (13.87 KB, patch)
2020-09-02 14:42 PDT, Sihui Liu
no flags
Patch (14.40 KB, patch)
2020-09-04 11:24 PDT, Sihui Liu
no flags
Sihui Liu
Comment 1 2020-08-31 13:34:55 PDT
Sihui Liu
Comment 2 2020-08-31 13:42:19 PDT
Darin Adler
Comment 3 2020-08-31 13:47:13 PDT
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?
Sihui Liu
Comment 4 2020-08-31 13:48:51 PDT
Sihui Liu
Comment 5 2020-08-31 14:04:34 PDT
(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.
Darin Adler
Comment 6 2020-08-31 14:05:35 PDT
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.
Darin Adler
Comment 7 2020-08-31 14:05:57 PDT
Oh, I see your response now.
Sihui Liu
Comment 8 2020-08-31 14:09:12 PDT
Darin Adler
Comment 9 2020-08-31 14:10:47 PDT
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?
Sihui Liu
Comment 10 2020-08-31 14:37:02 PDT
(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?
Tim Horton
Comment 11 2020-08-31 14:53:50 PDT
(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 :)
Simon Fraser (smfr)
Comment 12 2020-08-31 14:54:24 PDT
Should the title be "switching between tabs"?
Darin Adler
Comment 13 2020-08-31 14:58:17 PDT
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.
Sihui Liu
Comment 14 2020-08-31 14:59:45 PDT
(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.
Sihui Liu
Comment 15 2020-08-31 15:38:15 PDT
Sihui Liu
Comment 16 2020-08-31 15:39:54 PDT
(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?
Tim Horton
Comment 17 2020-08-31 15:43:20 PDT
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
Sihui Liu
Comment 18 2020-08-31 15:46:47 PDT
Darin Adler
Comment 19 2020-08-31 16:04:19 PDT
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.
Darin Adler
Comment 20 2020-08-31 16:04:41 PDT
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 Horton
Comment 21 2020-08-31 16:23:15 PDT
(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!
Tim Horton
Comment 22 2020-08-31 16:24:29 PDT
(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.
Sihui Liu
Comment 23 2020-08-31 16:42:56 PDT
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.
Sihui Liu
Comment 24 2020-08-31 16:49:06 PDT
(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?
Tim Horton
Comment 25 2020-08-31 16:56:53 PDT
(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.
Darin Adler
Comment 26 2020-08-31 17:34:43 PDT
(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?
Sihui Liu
Comment 27 2020-08-31 19:04:26 PDT
Created attachment 407648 [details] Patch for landing
EWS
Comment 28 2020-08-31 21:00:01 PDT
Committed r266384: <https://trac.webkit.org/changeset/266384> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407648 [details].
Hector Lopez
Comment 29 2020-09-01 14:49:18 PDT
Reverted r266384 for reason: Revision is causing 5 API faliures/timeouts and build failures on macOS Committed r266412: <https://trac.webkit.org/changeset/266412>
Sihui Liu
Comment 30 2020-09-02 14:04:55 PDT
Sihui Liu
Comment 31 2020-09-02 14:30:28 PDT
Sihui Liu
Comment 32 2020-09-02 14:42:46 PDT
Sihui Liu
Comment 33 2020-09-04 09:38:23 PDT
ping for review...
Tim Horton
Comment 34 2020-09-04 09:43:42 PDT
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"?
Sihui Liu
Comment 35 2020-09-04 10:24:34 PDT
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.
Sihui Liu
Comment 36 2020-09-04 11:24:10 PDT
EWS
Comment 37 2020-09-04 13:17:25 PDT
Committed r266634: <https://trac.webkit.org/changeset/266634> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408003 [details].
Note You need to log in before you can comment on or make changes to this bug.