Bug 216012

Summary: Webpages flash when switching between windows
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, hector_i_lopez, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=216047
https://bugs.webkit.org/show_bug.cgi?id=216263
https://bugs.webkit.org/show_bug.cgi?id=216275
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch
none
Patch
none
Patch none

Description Sihui Liu 2020-08-31 13:34:31 PDT
...
Comment 1 Sihui Liu 2020-08-31 13:34:55 PDT
rdar://problem/67097019
Comment 2 Sihui Liu 2020-08-31 13:42:19 PDT
Created attachment 407620 [details]
Patch
Comment 3 Darin Adler 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?
Comment 4 Sihui Liu 2020-08-31 13:48:51 PDT
Created attachment 407621 [details]
Patch
Comment 5 Sihui Liu 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.
Comment 6 Darin Adler 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.
Comment 7 Darin Adler 2020-08-31 14:05:57 PDT
Oh, I see your response now.
Comment 8 Sihui Liu 2020-08-31 14:09:12 PDT
Created attachment 407624 [details]
Patch
Comment 9 Darin Adler 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?
Comment 10 Sihui Liu 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?
Comment 11 Tim Horton 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 :)
Comment 12 Simon Fraser (smfr) 2020-08-31 14:54:24 PDT
Should the title be "switching between tabs"?
Comment 13 Darin Adler 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.
Comment 14 Sihui Liu 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.
Comment 15 Sihui Liu 2020-08-31 15:38:15 PDT
Created attachment 407631 [details]
Patch
Comment 16 Sihui Liu 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?
Comment 17 Tim Horton 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
Comment 18 Sihui Liu 2020-08-31 15:46:47 PDT
Created attachment 407632 [details]
Patch
Comment 19 Darin Adler 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.
Comment 20 Darin Adler 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.
Comment 21 Tim Horton 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!
Comment 22 Tim Horton 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.
Comment 23 Sihui Liu 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.
Comment 24 Sihui Liu 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?
Comment 25 Tim Horton 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.
Comment 26 Darin Adler 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?
Comment 27 Sihui Liu 2020-08-31 19:04:26 PDT
Created attachment 407648 [details]
Patch for landing
Comment 28 EWS 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].
Comment 29 Hector Lopez 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>
Comment 30 Sihui Liu 2020-09-02 14:04:55 PDT
Created attachment 407805 [details]
Patch
Comment 31 Sihui Liu 2020-09-02 14:30:28 PDT
Created attachment 407816 [details]
Patch
Comment 32 Sihui Liu 2020-09-02 14:42:46 PDT
Created attachment 407817 [details]
Patch
Comment 33 Sihui Liu 2020-09-04 09:38:23 PDT
ping for review...
Comment 34 Tim Horton 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"?
Comment 35 Sihui Liu 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.
Comment 36 Sihui Liu 2020-09-04 11:24:10 PDT
Created attachment 408003 [details]
Patch
Comment 37 EWS 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].