Bug 190879 – [PSON] Avoid tearing down the drawing area when suspending a WebPage due to process-swap

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Avoid tearing down the drawing area when suspending a WebPage due to process-swap. We really only need to reset the drawing area upon resuming the WebPage. There is no strict need to destroy the drawing area on suspension and this has caused various crashes because code usually assumes we always have a drawing area.

[Chris Dumez]

Created attachment 353039 [details]
Patch

[Chris Dumez]

Comment on attachment 353039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353039&action=review

> Source/WebKit/UIProcess/WebPageProxy.h:2264
>      WeakPtr<SuspendedPageProxy> m_pageSuspendedDueToCurrentNavigation;

I will look into dropping this member in a follow-up now that it is barely used and only on Mac.

[Chris Dumez]

Works on iOS but breaks Mac, trying to figure out why.

[Chris Dumez]

Created attachment 353050 [details]
Patch

[Antti Koivisto]

Comment on attachment 353050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353050&action=review

Which mechanism guarantees that we don't use memory for any tiles in suspended process?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6140
>      if (!m_isSuspended)
> -        return;
> -    m_drawingArea = nullptr;
> +        m_shouldResetDrawingArea = true;
> +
> +    setLayerTreeStateIsFrozen(true);

Why does this now call setLayerTreeStateIsFrozen(true) even when unsuspending?

[Chris Dumez]

Comment on attachment 353050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353050&action=review

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6140
>> +    setLayerTreeStateIsFrozen(true);
> 
> Why does this now call setLayerTreeStateIsFrozen(true) even when unsuspending?

I was wondering the same thing. This is pre-existing code that you added so you tell me :)

[Chris Dumez]

(In reply to Antti Koivisto from comment #5)
> Comment on attachment 353050 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353050&action=review
> 
> Which mechanism guarantees that we don't use memory for any tiles in
> suspended process?

I need to look into this as I do not know. The reason I started tearing down the drawing area was to fix the view freezing when navigating back with PSON on iOS, not for memory savings though.


> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6140
> >      if (!m_isSuspended)
> > -        return;
> > -    m_drawingArea = nullptr;
> > +        m_shouldResetDrawingArea = true;
> > +
> > +    setLayerTreeStateIsFrozen(true);
> 
> Why does this now call setLayerTreeStateIsFrozen(true) even when
> unsuspending?

[Chris Dumez]

Comment on attachment 353050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353050&action=review

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:-6131
> -    setLayerTreeStateIsFrozen(true);

Notice that setLayerTreeStateIsFrozen(true); was already called unconditionally here, whether suspended is true or false. I merely added a 'm_shouldResetDrawingArea = true;' statement to this method, did not change freezing behavior in any way. The diff just looks a bit weird because I dropped the method below and it shared some code with this one.

[Chris Dumez]

Created attachment 353189 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353189 [details]
Patch

Clearing flags on attachment: 353189

Committed r237467: <https://trac.webkit.org/changeset/237467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45593109>

[Geoffrey Garen]

> > Which mechanism guarantees that we don't use memory for any tiles in
> > suspended process?
> 
> I need to look into this as I do not know. The reason I started tearing down
> the drawing area was to fix the view freezing when navigating back with PSON
> on iOS, not for memory savings though.

Did we figure this out?