Bug 225008

Summary: Handle warning-level memory notifications more aggressively
Product: WebKit Reporter: Ben Nham <nham>
Component: New BugsAssignee: Ben Nham <nham>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, nham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
patch for landing none

Description Ben Nham 2021-04-23 16:17:22 PDT
Handle warning-level memory notifications more aggressively
Comment 1 Ben Nham 2021-04-23 16:22:52 PDT
Created attachment 426963 [details]
Patch
Comment 2 Ben Nham 2021-04-23 16:23:27 PDT
Comment on attachment 426963 [details]
Patch

initial pass for testing
Comment 3 Radar WebKit Bug Importer 2021-04-23 16:24:07 PDT
<rdar://problem/77091307>
Comment 4 Ben Nham 2021-04-26 13:30:34 PDT
Created attachment 427086 [details]
Patch
Comment 5 Chris Dumez 2021-04-26 14:04:28 PDT
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.
Comment 6 Ben Nham 2021-04-26 14:47:19 PDT
(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?
Comment 7 Chris Dumez 2021-04-26 14:50:55 PDT
(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.
Comment 8 Chris Dumez 2021-04-26 14:51:18 PDT
Comment on attachment 427086 [details]
Patch

r=me but ideally without ifdef.
Comment 9 Ben Nham 2021-04-26 14:56:02 PDT
Created attachment 427095 [details]
patch for landing
Comment 10 EWS 2021-04-26 16:04:25 PDT
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].