Bug 216012 – Webpages flash when switching between windows

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

rdar://problem/67097019

[Sihui Liu]

Created attachment 407620 [details]
Patch

[Darin Adler]

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]

Created attachment 407621 [details]
Patch

[Sihui Liu]

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

Oh, I see your response now.

[Sihui Liu]

Created attachment 407624 [details]
Patch

[Darin Adler]

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]

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

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

Should the title be "switching between tabs"?

[Darin Adler]

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]

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

Created attachment 407631 [details]
Patch

[Sihui Liu]

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

Created attachment 407632 [details]
Patch

[Darin Adler]

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

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

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

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

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

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

Created attachment 407648 [details]
Patch for landing

[EWS]

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]

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]

Created attachment 407805 [details]
Patch

[Sihui Liu]

Created attachment 407816 [details]
Patch

[Sihui Liu]

Created attachment 407817 [details]
Patch

[Sihui Liu]

ping for review...

[Tim Horton]

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

Created attachment 408003 [details]
Patch

[EWS]

Committed r266634: <https://trac.webkit.org/changeset/266634>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408003 [details].