WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
225008
Handle warning-level memory notifications more aggressively
https://bugs.webkit.org/show_bug.cgi?id=225008
Summary
Handle warning-level memory notifications more aggressively
Ben Nham
Reported
2021-04-23 16:17:22 PDT
Handle warning-level memory notifications more aggressively
Attachments
Patch
(2.62 KB, patch)
2021-04-23 16:22 PDT
,
Ben Nham
no flags
Details
Formatted Diff
Diff
Patch
(3.49 KB, patch)
2021-04-26 13:30 PDT
,
Ben Nham
no flags
Details
Formatted Diff
Diff
patch for landing
(2.47 KB, patch)
2021-04-26 14:56 PDT
,
Ben Nham
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Ben Nham
Comment 1
2021-04-23 16:22:52 PDT
Created
attachment 426963
[details]
Patch
Ben Nham
Comment 2
2021-04-23 16:23:27 PDT
Comment on
attachment 426963
[details]
Patch initial pass for testing
Radar WebKit Bug Importer
Comment 3
2021-04-23 16:24:07 PDT
<
rdar://problem/77091307
>
Ben Nham
Comment 4
2021-04-26 13:30:34 PDT
Created
attachment 427086
[details]
Patch
Chris Dumez
Comment 5
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.
Ben Nham
Comment 6
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?
Chris Dumez
Comment 7
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.
Chris Dumez
Comment 8
2021-04-26 14:51:18 PDT
Comment on
attachment 427086
[details]
Patch r=me but ideally without ifdef.
Ben Nham
Comment 9
2021-04-26 14:56:02 PDT
Created
attachment 427095
[details]
patch for landing
EWS
Comment 10
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug