[GTK] Web processes should not have unlimited access to memory

• Component: WebKitGTK
• Status: NEW ---

[Michael Catanzaro]

Web processes should not have unlimited access to memory.

A quick example of why not: open a bunch of bugs on Red Hat Bugzilla in new tabs. [1] If you open ~20 bugs, at least one (usually about three for me) should run out of control, allocating memory until your computer begins to swap excessively. Mine hangs for an hour, forcing me to power off. This happens to me multiple times per day. (Curious if any folks from Apple can reproduce this in Safari.) Even though it's a bug that should be fixed, other similar bugs exist, like [2] and [3], or might exist in the future, so we should be robust to this by setting a memory limit for the web process.

On Linux the options for limiting memory are RLIMIT_AS and RLIMIT_DATA. RLIMIT_RSS exists but doesn't do anything; even if it worked, it wouldn't be useful for the problem I want to solve, which is swapping. RLIMIT_DATA is not useful in practice since it doesn't affect mmap(), which is what we use to allocate memory, and also what malloc() uses. So RLIMIT_AS is the only option. I've picked a 5 GB address space limit, which should work for a long time in the future. In practice, this limits web processes to ~1.4 GB of memory on my machine. A well-behaved web process uses 50-100 MiB (although I've seen them go as high as ~250 MiB after a sufficiently long time loading many pages and leaking much memory), so this is so wildly higher than necessary that it shouldn't hurt normal operation. (Of course, we might need to increase it in the distant future.)

[1] https://bugzilla.redhat.com/buglist.cgi?component=epiphany&product=Fedora
[2] https://bugs.webkit.org/show_bug.cgi?id=139847
[3] https://bugs.webkit.org/show_bug.cgi?id=126122

[Michael Catanzaro]

Created attachment 256489 [details]
Patch

[Martin Robinson]

Comment on attachment 256489 [details]
Patch

What happens when a WebProcess hits this limit? Does it crash?

[Michael Catanzaro]

(In reply to comment #2)
> Comment on attachment 256489 [details]
> Patch
> 
> What happens when a WebProcess hits this limit? Does it crash?

It causes mmap() to return ENOMEM. If that happens to WTF::OSAllocator, it will immediately call CRASH(). Other call sites could handle it differently, of course. In practice, with my reproducer, the web process loads for some time (30-45 seconds) and then crashes.

[Carlos Alberto Lopez Perez]

Comment on attachment 256489 [details]
Patch

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

> Source/WebKit2/Shared/unix/ChildProcessMain.cpp:62
> +    // The address space limit is currently set to 5 GB. In practice, this causes a runaway web
> +    // process to cap out at about 1.4 GB of allocated memory.
> +    static const auto addressSpaceLimitBytes = 5000000000l;
> +    rlimit64 rlim = {addressSpaceLimitBytes, addressSpaceLimitBytes};
> +    rlim.rlim_max = rlim.rlim_cur;
> +    if (setrlimit64(RLIMIT_AS, &rlim))

I think that we should allow at runtime to tune this limit as also to disable any limit check. Maybe with an environment variable?

[Michael Catanzaro]

Good idea. An environment variable sounds best. Do you prefer something short like WEBKIT_CHILD_RLIMIT_AS or WEBKIT_CHILD_ASLIMIT, or something more descriptive like WEBKIT_CHILD_ADDRESS_SPACE_LIMIT? If set to 0 the limit would be disabled, otherwise it would be applied in bytes.

[Carlos Garcia Campos]

Comment on attachment 256489 [details]
Patch

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

> Source/WebKit2/Shared/unix/ChildProcessMain.cpp:46
> +void ChildProcessMainBase::setProcessResourceLimits()

Only resource this limits is memory, no? I would call it setMemoryLimit, or even setHeap Limit, don't need to a use Process in the method name either, since it's a method of a ChildProcess class.

> Source/WebKit2/Shared/unix/ChildProcessMain.h:40
> +    virtual void setProcessResourceLimits();

Why is this virtual?

[Michael Catanzaro]

Do you think we should provide a way to override the limit at compile time as well? I imagine the address space required could vary considerably depending on the distribution. Something like:

#ifndef WEBKIT_CHILD_ADDRESS_SPACE_LIMIT
#define WEBKIT_CHILD_ADDRESS_SPACE_LIMIT 5000000000l
#endif

(In reply to comment #6)
> Only resource this limits is memory, no? I would call it setMemoryLimit, or
> even setHeap Limit, don't need to a use Process in the method name either,
> since it's a method of a ChildProcess class.

Let's call it setAddressSpaceLimit. I wish something as simple as a "memory" limit was possible in Linux.

(In reply to comment #6) 
> > Source/WebKit2/Shared/unix/ChildProcessMain.h:40
> > +    virtual void setProcessResourceLimits();
> 
> Why is this virtual?

Since all the other member functions of ChildProcessMain are virtual, to allow child processes to have different resource limits. We could leave it non-virtual until a child process wants to do this; I don't care.

[Carlos Alberto Lopez Perez]

(In reply to comment #5)
> Good idea. An environment variable sounds best. Do you prefer something
> short like WEBKIT_CHILD_RLIMIT_AS or WEBKIT_CHILD_ASLIMIT, or something more
> descriptive like WEBKIT_CHILD_ADDRESS_SPACE_LIMIT? If set to 0 the limit
> would be disabled, otherwise it would be applied in bytes.

I like more the first one, but also the others are fine.

[Michael Catanzaro]

Created attachment 256616 [details]
Patch

[Jérémy Lal]

Hi !
i built this map using javascript, dom, svg and external http resources:
http://assets-carte.lefigaro.fr/cartes/common/france-resultats-2014.jpg
it was a ~4GB memory usage and the only engine that didn't crash or hang or error out or leak like crazy was webkitgtk...
While the rationale is certainly good, this limit sounds a bit like shooting oneself in the foot, it should be documented upfront !

[Michael Catanzaro]

Not sure I understand the problem. My web processes require < 50 MiB to display that image.

[Jérémy Lal]

(In reply to comment #11)
> Not sure I understand the problem. My web processes require < 50 MiB to
> display that image.

The image is a jpg created from a big svg file created by those means.

[Michael Catanzaro]

This broke Java. The plugin process needs to be exempt from the limit.

[Michael Catanzaro]

Created attachment 257645 [details]
Patch

[Michael Catanzaro]

Comment on attachment 257645 [details]
Patch

Perhaps predictably in retrospect, this has caused a very significant increase in OOM, and generally made the web process less reliable. I'm going to keep the patch in Fedora until the bug in comment #0 is resolved, due to the severity of that bug, but I don't recommend it this for upstream and have removed r?