Bug 184261 – [GTK][WPE] Memory pressure monitor doesn't reliable notify all the subprocesses

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

The current implementation of the memory pressure monitor ( non-cgroup memory monitor added in bug 155255 ) on the UIProcess is racy and fails to reliably notify all the webkit-related subprocess that it should.

When a memory notification event should happen, the UIProcess writes to a shared eventfd. Then the webkit subprocess (webprocess, networkprocess, etc) should receive the event and act according.

The issue is that they race between them to read this notification on the eventfd: Sometimes they all get the notification, but many other times only one of them get it.

[Carlos Alberto Lopez Perez]

Created attachment 337074 [details]
Test case to demonstrate the issue

The following patch demonstrates the issue. It patches WebKit to simulate a memory event notification each 5 seconds and then prints which process have received it.
In theory all process always should receive the notification, in practice many are missed. Example:


[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/MiniBrowser(7969)]TRIGGER MEMORY PRESSURE EVENT and sleep 5
[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/WebKitNetworkProcess(8004)]Received event to trigger a memory pressure event


[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/MiniBrowser(7969)]TRIGGER MEMORY PRESSURE EVENT and sleep 5
[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/WebKitWebProcess(8002)]Received event to trigger a memory pressure event


[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/MiniBrowser(7969)]TRIGGER MEMORY PRESSURE EVENT and sleep 5
[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/WebKitNetworkProcess(8004)]Received event to trigger a memory pressure event
[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/WebKitWebProcess(8002)]Received event to trigger a memory pressure event


[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/MiniBrowser(7969)]TRIGGER MEMORY PRESSURE EVENT and sleep 5
[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/WebKitWebProcess(8002)]Received event to trigger a memory pressure event


[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/MiniBrowser(7969)]TRIGGER MEMORY PRESSURE EVENT and sleep 5
[/home/igalia/clopez/webkit/webkit/WebKitBuild/Release/bin/WebKitWebProcess(8002)]Received event to trigger a memory pressure event


As you can see only once the network and web process received it at the same time, the other times only one of this two got it.
And this is without opening multiple tabs (just only one webprocess and one network process)

[Carlos Alberto Lopez Perez]

I think this problem is unavoidable with the current implementation (with eventfd). My understanding is that when one of the subprocess reads the value of the eventfd, it destroys (consumes) it.

But since the code does't read the value of the eventfd immediately after receiving the notification that there is something to read on the descriptor, there is a chance that more than one subprocess gets the notification. But its not something that happens reliably.


Perhaps an idea is using a signalfd descriptor instead, and sending a determined signal to all subprocess ? https://gabrbedd.wordpress.com/2013/07/29/handling-signals-with-signalfd/

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #2)
> I think this problem is unavoidable with the current implementation (with
> eventfd). My understanding is that when one of the subprocess reads the
> value of the eventfd, it destroys (consumes) it.

That's correct.

> But since the code does't read the value of the eventfd immediately after
> receiving the notification that there is something to read on the
> descriptor, there is a chance that more than one subprocess gets the
> notification. But its not something that happens reliably.

Ouch. That means that the process which gets scheduled first after
sending data to the eventfd is the one getting notified, and all the
others will miss the event.

> Perhaps an idea is using a signalfd descriptor instead, and sending a
> determined signal to all subprocess ?
> https://gabrbedd.wordpress.com/2013/07/29/handling-signals-with-signalfd/

Or, better than signals (which are always a bit clunky): the UI process
creates one eventfd which gets passed to each of the spawned processes,
and writes the event to _all_ of them (it's a broadcast). This would have
the downside of having one more additional file descriptor open for each
WebProcess, but would work.

There may be other options building upon cross-process IPC facilities but
in my head they sound like wouldn't be particularly nicer to integrate than
having an eventfd per process (semaphones, anyone? :D)


BTW, is there any reason why WebKit IPC cannot be used in this case?

[Carlos Alberto Lopez Perez]

(In reply to Adrian Perez from comment #3)
> (In reply to Carlos Alberto Lopez Perez from comment #2)
> > I think this problem is unavoidable with the current implementation (with
> > eventfd). My understanding is that when one of the subprocess reads the
> > value of the eventfd, it destroys (consumes) it.
> 
> That's correct.
> 
> > But since the code does't read the value of the eventfd immediately after
> > receiving the notification that there is something to read on the
> > descriptor, there is a chance that more than one subprocess gets the
> > notification. But its not something that happens reliably.
> 
> Ouch. That means that the process which gets scheduled first after
> sending data to the eventfd is the one getting notified, and all the
> others will miss the event.
> 

Not exactly.
The current code triggers the event if it gets the notification that there is something to read. This happens:

1. UIProcess writes 1 to the eventfd
2. WebProcess receives the notification that there is something to read
3. NetWorkProcess receives the notification that there is something to read
4. WebProcess try to read the data on the eventfd (ok)
5. NetWorkProcess try to read the data on the eventfd (fail with EAGAIN, but thats ok in the current implementation)
6. WebProcess triggers the memory event
7. NetworkProcess triggers the memory event.


That is what happens when it works fine (the two process receive the event).

But it is racy, it all depends on the notification (on point 3) happening before the first read (on point 4). Otherwise there won't be notification for the other process. And when more than one webprocess is in use it becomes even more racy because there are more subprocess trying to get the notification in time.


> > Perhaps an idea is using a signalfd descriptor instead, and sending a
> > determined signal to all subprocess ?
> > https://gabrbedd.wordpress.com/2013/07/29/handling-signals-with-signalfd/
> 
> Or, better than signals (which are always a bit clunky): the UI process
> creates one eventfd which gets passed to each of the spawned processes,
> and writes the event to _all_ of them (it's a broadcast). This would have
> the downside of having one more additional file descriptor open for each
> WebProcess, but would work.
> 

I think that with signalfd all the usual problems of using signals can be avoided easily, can't be?

Using multiple eventfd file descriptors complicates even more the implementation, which is already fairly complex.

> There may be other options building upon cross-process IPC facilities but
> in my head they sound like wouldn't be particularly nicer to integrate than
> having an eventfd per process (semaphones, anyone? :D)
> 
> 
> BTW, is there any reason why WebKit IPC cannot be used in this case?

Not sure. This can be a good idea, indeed.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #3)
> BTW, is there any reason why WebKit IPC cannot be used in this case?

Good call. I think there should be a really good reason (which I doubt exists) if not using WebKit IPC.

[Michael Catanzaro]

BTW if considering signalfd, you probably actually want g_unix_signal_add() for easy RunLoop integration.

[Carlos Garcia Campos]

I think we wanted a very efficient way of communication, assuming that if the system memory is growing too fast we need to react as fast as possible. Also to ensure that other IPC messages don't affect this one, if we had a way to set priorities. But we could try, we never measured anything after all.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #7)
> I think we wanted a very efficient way of communication, assuming that if
> the system memory is growing too fast we need to react as fast as possible.
> Also to ensure that other IPC messages don't affect this one, if we had a
> way to set priorities. But we could try, we never measured anything after
> all.

So I have started to look at this and I have a WIP implementation of this.

But before continuing I wanted to be sure that delivering the events via IPC would not be a regression in terms of time to deliver the event.

So I benchmarked this.

I took a screencast of two webkitgtks running at the same time a stressful benchmark (JetStream) meanwhile the UIProcess delivers each 10 seconds a memory pressure event with both methods (current eventfd and WebKit's IPC).

Then I parsed the output generated, calculated time deltas, and got this numbers:

Current EventFD method (JetStream):

	Total seconds to deliver the 18 events: 30.873742
	Biggest deliver time : 5.472223
	Average of deliver time per event: 1.715208


WebKit IPC method (JetStream):

	Total seconds to deliver the 18 events: 33.386153
	Biggest deliver time : 4.851518
	Average of deliver time per event: 1.854786

If you are further interested, I shared all this data (including the screencast) at https://people.igalia.com/clopez/wkbug/184261/
I repeated this a number of times and the numbers vary a bit, but the difference between methods is more or less the same.

My conclusion is that it seems there is a small difference, and the eventFD method is a bit faster to be delivered. I guess that was already expected.

But the difference is really small, is not something that is going to make a difference in the real world I think.
Likely we can compensate this by tuning further the thresholds on the UIProcess WebPressure Monitor so it starts delivering events sooner.


So, I would continue with this patch to move the UIProcess WebPressure Monitor to use WebKit's IPC to deliver the events unless someone have a different opinion in the light of this data.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #8)
> So, I would continue with this patch to move the UIProcess WebPressure
> Monitor to use WebKit's IPC to deliver the events unless someone have a
> different opinion in the light of this data.

Sounds good!

[Adrian Perez]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Carlos Alberto Lopez Perez from comment #8)
> > So, I would continue with this patch to move the UIProcess WebPressure
> > Monitor to use WebKit's IPC to deliver the events unless someone have a
> > different opinion in the light of this data.
> 
> Sounds good!

Indeed, using WebKit IPC sounds like the right call here. The difference
between both methods is quite small, and that way we avoid using many
additional file descriptors for eventfd (they are a kind-of-scarce
resource after all, and anything we can do to use less of them is a
good move, IMO).

[Carlos Alberto Lopez Perez]

Created attachment 340167 [details]
Patch

[EWS Watchlist]

Attachment 340167 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:245:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:278:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 340167 [details]
Patch

Attachment 340167 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7647517

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 340174 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Carlos Garcia Campos]

Comment on attachment 340167 [details]
Patch

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

I wonder whether it's worth it keeping the systemd implementation. I think it's actually untested, because it requires to set it up in the system, and I bet most of us don't do it. We could simplify everything by using the IPC message unconditionally and take advantage to also send whether the memory pressure situation is critical or not.

> Source/WebKit/UIProcess/WebProcessPool.cpp:274
> +#if OS(LINUX)
> +    if (MemoryPressureMonitor::isEnabled())
> +        MemoryPressureMonitor::singleton().setProcessPool(this);
> +#endif

We don't need this, we always want to notify all process pools, here we are always setting the last process pool, created in the monitor, that is a singleton.

> Source/WebKit/UIProcess/WebProcessPool.cpp:423
> +    for (auto* processPool : allProcessPools()) {

allProcessPools() is static, so we don't need a member method, the caller could call sendMemoryPressureEvent() to every process pool instead without having to keep a pointer to a particular process pool.

> Source/WebKit/UIProcess/WebProcessPool.cpp:426
> +        if (processPool->m_networkProcess)
> +            processPool->m_networkProcess->send(Messages::ChildProcess::DidReceiveMemoryPressureEvent(), 0);

You could use sendToNetworkingProcess() instead, that already does the check.

> Source/WebKit/UIProcess/WebProcessPool.cpp:427
> +    }

What about the other processes? I guess we should notify storage and plugin processes, no?

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:259
> +                if (m_processPool)
> +                    m_processPool->sendMemoryPressureEvent();

Now that we are using an IPC message we could also send whether it's critical or not as parameter of the message.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #15)
> Comment on attachment 340167 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340167&action=review
> 
> I wonder whether it's worth it keeping the systemd implementation. I think
> it's actually untested, because it requires to set it up in the system, and
> I bet most of us don't do it. We could simplify everything by using the IPC
> message unconditionally and take advantage to also send whether the memory
> pressure situation is critical or not.
> 

I tested it and works fine.

And its surprisingly amazing how easy is to enable it. Just run:
sudo chmod 666 /sys/fs/cgroup/memory/memory.pressure_level /sys/fs/cgroup/memory/cgroup.event_control
(or similar, the thing it to just have rw access to those files)

Of course this will be lost after a reboot. So some udev rule would have to be deployed to make it stick.

I wonder if other distros than Debian have better defaults for the permissions  of this sysfs files.

> > Source/WebKit/UIProcess/WebProcessPool.cpp:274
> > +#if OS(LINUX)
> > +    if (MemoryPressureMonitor::isEnabled())
> > +        MemoryPressureMonitor::singleton().setProcessPool(this);
> > +#endif
> 
> We don't need this, we always want to notify all process pools, here we are
> always setting the last process pool, created in the monitor, that is a
> singleton.
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:423
> > +    for (auto* processPool : allProcessPools()) {
> 
> allProcessPools() is static, so we don't need a member method, the caller
> could call sendMemoryPressureEvent() to every process pool instead without
> having to keep a pointer to a particular process pool.
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:426
> > +        if (processPool->m_networkProcess)
> > +            processPool->m_networkProcess->send(Messages::ChildProcess::DidReceiveMemoryPressureEvent(), 0);
> 
> You could use sendToNetworkingProcess() instead, that already does the check.
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:427
> > +    }
> 
> What about the other processes? I guess we should notify storage and plugin
> processes, no?
> 

Notifications to storage process can be delivered but do nothing. The previous/current implementation doesn't install a memory pressure handler on storage process. I was planning to leave that for other patch.

And for plugin process I tried to do it, but I have no idea how to send the message there from WebProcesPool. So I ended leaving this out as a minor issue (plugin process is something less and less useful as time goes on)
But I will be happy to enable it if its easy. Do you know how this can be done?

> > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:259
> > +                if (m_processPool)
> > +                    m_processPool->sendMemoryPressureEvent();
> 
> Now that we are using an IPC message we could also send whether it's
> critical or not as parameter of the message.

Indeed.

[Carlos Garcia Campos]

(In reply to Carlos Alberto Lopez Perez from comment #16)
> (In reply to Carlos Garcia Campos from comment #15)
> > Comment on attachment 340167 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=340167&action=review
> > 
> > I wonder whether it's worth it keeping the systemd implementation. I think
> > it's actually untested, because it requires to set it up in the system, and
> > I bet most of us don't do it. We could simplify everything by using the IPC
> > message unconditionally and take advantage to also send whether the memory
> > pressure situation is critical or not.
> > 
> 
> I tested it and works fine.
> 
> And its surprisingly amazing how easy is to enable it. Just run:
> sudo chmod 666 /sys/fs/cgroup/memory/memory.pressure_level
> /sys/fs/cgroup/memory/cgroup.event_control
> (or similar, the thing it to just have rw access to those files)

It's not a matter of being easy or not, but whether people do actually do that. I guess most of the users don't even know it, so better to have a single implementation that always works for everybody.

> Of course this will be lost after a reboot. So some udev rule would have to
> be deployed to make it stick.
>
> I wonder if other distros than Debian have better defaults for the
> permissions  of this sysfs files.

It makes the code a lot more complex for little (or no) benefit. so unless there's a really good reason to keep it, I would just remove it.

> > > Source/WebKit/UIProcess/WebProcessPool.cpp:274
> > > +#if OS(LINUX)
> > > +    if (MemoryPressureMonitor::isEnabled())
> > > +        MemoryPressureMonitor::singleton().setProcessPool(this);
> > > +#endif
> > 
> > We don't need this, we always want to notify all process pools, here we are
> > always setting the last process pool, created in the monitor, that is a
> > singleton.
> > 
> > > Source/WebKit/UIProcess/WebProcessPool.cpp:423
> > > +    for (auto* processPool : allProcessPools()) {
> > 
> > allProcessPools() is static, so we don't need a member method, the caller
> > could call sendMemoryPressureEvent() to every process pool instead without
> > having to keep a pointer to a particular process pool.
> > 
> > > Source/WebKit/UIProcess/WebProcessPool.cpp:426
> > > +        if (processPool->m_networkProcess)
> > > +            processPool->m_networkProcess->send(Messages::ChildProcess::DidReceiveMemoryPressureEvent(), 0);
> > 
> > You could use sendToNetworkingProcess() instead, that already does the check.
> > 
> > > Source/WebKit/UIProcess/WebProcessPool.cpp:427
> > > +    }
> > 
> > What about the other processes? I guess we should notify storage and plugin
> > processes, no?
> > 
> 
> Notifications to storage process can be delivered but do nothing. The
> previous/current implementation doesn't install a memory pressure handler on
> storage process. I was planning to leave that for other patch.

The process pool shouldn't know whether a child process implements it or not, I think, otherwise I would add the message to every process proxy.

> And for plugin process I tried to do it, but I have no idea how to send the
> message there from WebProcesPool. So I ended leaving this out as a minor
> issue (plugin process is something less and less useful as time goes on)
> But I will be happy to enable it if its easy. Do you know how this can be
> done?

You will have to add methods to PluginProcessManager and PluginProcessProxy I guess.

> > > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:259
> > > +                if (m_processPool)
> > > +                    m_processPool->sendMemoryPressureEvent();
> > 
> > Now that we are using an IPC message we could also send whether it's
> > critical or not as parameter of the message.
> 
> Indeed.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #17)
> (In reply to Carlos Alberto Lopez Perez from comment #16)
> > (In reply to Carlos Garcia Campos from comment #15)
> > > Comment on attachment 340167 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=340167&action=review
> > > 
> > > I wonder whether it's worth it keeping the systemd implementation. I think
> > > it's actually untested, because it requires to set it up in the system, and
> > > I bet most of us don't do it. We could simplify everything by using the IPC
> > > message unconditionally and take advantage to also send whether the memory
> > > pressure situation is critical or not.
> > > 
> > 
> > I tested it and works fine.
> > 
> > And its surprisingly amazing how easy is to enable it. Just run:
> > sudo chmod 666 /sys/fs/cgroup/memory/memory.pressure_level
> > /sys/fs/cgroup/memory/cgroup.event_control
> > (or similar, the thing it to just have rw access to those files)
> 
> It's not a matter of being easy or not, but whether people do actually do
> that. I guess most of the users don't even know it, so better to have a
> single implementation that always works for everybody.
> 
> > Of course this will be lost after a reboot. So some udev rule would have to
> > be deployed to make it stick.
> >
> > I wonder if other distros than Debian have better defaults for the
> > permissions  of this sysfs files.
> 
> It makes the code a lot more complex for little (or no) benefit. so unless
> there's a really good reason to keep it, I would just remove it.
> 

Fair enough. I will remove it.

Better have one implementation well tested (even if its not the most efficient one) than two half tested.

Also that will simplify the code a lot.

[Carlos Alberto Lopez Perez]

Created attachment 341087 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 341087 [details]
Patch

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

This looks good to me, but it requires a WebKit2 owner approval

> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:139
> +        PluginProcessProxy* pluginProxy = pluginProcess.get();
> +        if (pluginProxy)

why do you need another variable?

[Carlos Alberto Lopez Perez]

Comment on attachment 341087 [details]
Patch

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

>> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:139
>> +        if (pluginProxy)
> 
> why do you need another variable?

I'm unsure if pluginProcess can be null, so in order to check for that before trying to de-reference.

[Carlos Garcia Campos]

Comment on attachment 341087 [details]
Patch

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

>>> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:139
>>> +        if (pluginProxy)
>> 
>> why do you need another variable?
> 
> I'm unsure if pluginProcess can be null, so in order to check for that before trying to de-reference.

You already have pluginProcess variable to check it, but anyway, it can't be nullptr, so you can use it directly.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #22)
> Comment on attachment 341087 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341087&action=review
> 
> >>> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:139
> >>> +        if (pluginProxy)
> >> 
> >> why do you need another variable?
> > 
> > I'm unsure if pluginProcess can be null, so in order to check for that before trying to de-reference.
> 
> You already have pluginProcess variable to check it, but anyway, it can't be
> nullptr, so you can use it directly.

Ok. I will update it accordingly and ask for owner approval.

Thanks

[Carlos Alberto Lopez Perez]

Created attachment 341092 [details]
Patch

Patch reviewed by Carlos Garcia. Looking for Webkit2 Owner approval.

[youenn fablet]

Comment on attachment 341092 [details]
Patch

I do not see anything wrong here but I am not very familiar with how memory pressure is done in cocoa ports.
Two nits below.

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

> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:137
> +    for (auto pluginProcess : m_pluginProcesses)

auto&

> Source/WebKit/UIProcess/WebProcessPool.cpp:276
> +    MemoryPressureMonitor::singleton();

Maybe it should be renamed to something like MemoryPressureMonitor::start() even if internally it just calls singleton?

[Carlos Alberto Lopez Perez]

Created attachment 341194 [details]
Patch

Implement changes suggested by youenn. Patch for landing.

[Brady Eidson]

Please conditionalize the new code in PluginProcess::didReceiveMessage, like all the other code here that is OS(LINUX) only.

[Carlos Alberto Lopez Perez]

(In reply to Brady Eidson from comment #27)
> Please conditionalize the new code in PluginProcess::didReceiveMessage, like
> all the other code here that is OS(LINUX) only.

That is needed for the plugin process to receive any message encoded with  Messages::ChildProcess name.

I thought it would be useful anyway even for not OS(LINUX).

But I will implement the requested change now.

[Carlos Alberto Lopez Perez]

Created attachment 341208 [details]
Patch

Implement change requested by Brady. Patch for landing.

[Brady Eidson]

(In reply to Carlos Alberto Lopez Perez from comment #28)
> (In reply to Brady Eidson from comment #27)
> > Please conditionalize the new code in PluginProcess::didReceiveMessage, like
> > all the other code here that is OS(LINUX) only.
> 
> That is needed for the plugin process to receive any message encoded with 
> Messages::ChildProcess name.
> 
> I thought it would be useful anyway even for not OS(LINUX).

But since no such messages are necessary right now (as evidenced by the fact that we think it works fine without it before this patch)...

Patch looks fine otherwise (though it's depressing how much #if OS(...) code is being shuffled around in cross platform cpp files - definitely indicative of a design problem. Outside the scope of this patch though.

[Carlos Alberto Lopez Perez]

(In reply to Brady Eidson from comment #30)
> (In reply to Carlos Alberto Lopez Perez from comment #28)
> > (In reply to Brady Eidson from comment #27)
> > > Please conditionalize the new code in PluginProcess::didReceiveMessage, like
> > > all the other code here that is OS(LINUX) only.
> > 
> > That is needed for the plugin process to receive any message encoded with 
> > Messages::ChildProcess name.
> > 
> > I thought it would be useful anyway even for not OS(LINUX).
> 
> But since no such messages are necessary right now (as evidenced by the fact
> that we think it works fine without it before this patch)...
> 
> Patch looks fine otherwise (though it's depressing how much #if OS(...) code
> is being shuffled around in cross platform cpp files - definitely indicative
> of a design problem. Outside the scope of this patch though.

All that OS(LINUX) ifdefs can in theory be removed. The code is pretty much cross-platform.

The only part that is really linux-specific is the way to get the system memory information in Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp by reading /proc linux specific files.


I prefixed it all with OS(LINUX) because it is currently only useful for Linux, due to the UIProcess memory monitor only being implemented for it


But perhaps tomorrow other non-Linux port decide to implement also its own UIProcess memory monitor? Then all those OS(LINUX) can be removed or prefixed with the required or.

[Carlos Alberto Lopez Perez]

So.. can I commit this? Can any WebKit2 owner confirm that the patch is ok for commit please? thanks in advance

[WebKit Commit Bot]

Comment on attachment 341208 [details]
Patch

Clearing flags on attachment: 341208

Committed r232176: <https://trac.webkit.org/changeset/232176>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

(In reply to Carlos Alberto Lopez Perez from comment #32)
> So.. can I commit this? Can any WebKit2 owner confirm that the patch is ok
> for commit please? thanks in advance

I didn't r+ because it had already been reviewed by a reviewer and it wasn't even r?

In https://bugs.webkit.org/show_bug.cgi?id=184261#c30 I said "Patch looks fine"

I see y'all landed it last night anyways :)