Bug 225008 – Handle warning-level memory notifications more aggressively

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Nham]

Handle warning-level memory notifications more aggressively

[Ben Nham]

Created attachment 426963 [details]
Patch

[Ben Nham]

Comment on attachment 426963 [details]
Patch

initial pass for testing

[Radar WebKit Bug Importer]

<rdar://problem/77091307>

[Ben Nham]

Created attachment 427086 [details]
Patch

[Chris Dumez]

Comment on attachment 427086 [details]
Patch

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

> Source/WTF/wtf/PlatformEnableCocoa.h:353
> +#define ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING 1

Maybe ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING_IN_BACKGROUND_PROCESSES ?

But overall, I am not sure we really need/want a build flag at all? What's the worry on iOS? Usually background processes on iOS will be suspended. If they are not because of some kind of process assertion, then why not treat these memory warnings as critical?
I think we should try and keep things cross-platform as much as possible.

> Source/WebKit/WebProcess/WebProcess.cpp:389
> +        if (m_pagesInWindows.isEmpty() && critical == Critical::No)

I have some concerns about m_pagesInWindows being possibly out of date. In particular, shouldn't we remove a WebPage from m_pagesInWindows in WebProcess::removeWebPage()? I'd feel a lot more confident stale pages could not be in m_pagesInWindows if we did that. I guess I could convinced myself that pageDidLeaveWindow MUST be called before WebProcess::removeWebPage() but it seems fragile.

[Ben Nham]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 427086 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427086&action=review
> 
> > Source/WTF/wtf/PlatformEnableCocoa.h:353
> > +#define ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING 1
> 
> Maybe ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING_IN_BACKGROUND_PROCESSES ?
> 
> But overall, I am not sure we really need/want a build flag at all? What's
> the worry on iOS? Usually background processes on iOS will be suspended. If
> they are not because of some kind of process assertion, then why not treat
> these memory warnings as critical?
> I think we should try and keep things cross-platform as much as possible.

Good point, I think I'll remove the ifdef.

> > Source/WebKit/WebProcess/WebProcess.cpp:389
> > +        if (m_pagesInWindows.isEmpty() && critical == Critical::No)
> 
> I have some concerns about m_pagesInWindows being possibly out of date. In
> particular, shouldn't we remove a WebPage from m_pagesInWindows in
> WebProcess::removeWebPage()? I'd feel a lot more confident stale pages could
> not be in m_pagesInWindows if we did that. I guess I could convinced myself
> that pageDidLeaveWindow MUST be called before WebProcess::removeWebPage()
> but it seems fragile.

Hmm, I'm not sure how m_pagesInWindows would be out of date. WebProcess::removeWebPage calls WebProcess::pageWillLeaveWindow so that leaves m_pagesInWindows in a consistent state at the end of that run loop iteration. And the memory pressure handler also runs in the main run loop so it will see m_pagesInWindows in a consistent state. Am I missing something here?

[Chris Dumez]

(In reply to Ben Nham from comment #6)
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 427086 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427086&action=review
> > 
> > > Source/WTF/wtf/PlatformEnableCocoa.h:353
> > > +#define ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING 1
> > 
> > Maybe ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING_IN_BACKGROUND_PROCESSES ?
> > 
> > But overall, I am not sure we really need/want a build flag at all? What's
> > the worry on iOS? Usually background processes on iOS will be suspended. If
> > they are not because of some kind of process assertion, then why not treat
> > these memory warnings as critical?
> > I think we should try and keep things cross-platform as much as possible.
> 
> Good point, I think I'll remove the ifdef.
> 
> > > Source/WebKit/WebProcess/WebProcess.cpp:389
> > > +        if (m_pagesInWindows.isEmpty() && critical == Critical::No)
> > 
> > I have some concerns about m_pagesInWindows being possibly out of date. In
> > particular, shouldn't we remove a WebPage from m_pagesInWindows in
> > WebProcess::removeWebPage()? I'd feel a lot more confident stale pages could
> > not be in m_pagesInWindows if we did that. I guess I could convinced myself
> > that pageDidLeaveWindow MUST be called before WebProcess::removeWebPage()
> > but it seems fragile.
> 
> Hmm, I'm not sure how m_pagesInWindows would be out of date.
> WebProcess::removeWebPage calls WebProcess::pageWillLeaveWindow so that
> leaves m_pagesInWindows in a consistent state at the end of that run loop
> iteration. And the memory pressure handler also runs in the main run loop so
> it will see m_pagesInWindows in a consistent state. Am I missing something
> here?

No, you are right. I missed that WebProcess::removeWebPage() was calling pageWillLeaveWindow(). All good.

[Chris Dumez]

Comment on attachment 427086 [details]
Patch

r=me but ideally without ifdef.

[Ben Nham]

Created attachment 427095 [details]
patch for landing

[EWS]

Committed r276618 (237047@main): <https://commits.webkit.org/237047@main>

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