[webkit-reviews] review denied: [Bug 209942] [GTK][WPE] Replace fopen/fclose by fopen/fseek functions in MemoryPressureMonitor : [Attachment 395329] patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Pablo Saavedra
<psaavedra at igalia.com>'s request for review:
Bug 209942: [GTK][WPE] Replace fopen/fclose by fopen/fseek functions in
MemoryPressureMonitor
https://bugs.webkit.org/show_bug.cgi?id=209942

Attachment 395329: patch

https://bugs.webkit.org/attachment.cgi?id=395329&action=review




--- Comment #11 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 395329
  --> https://bugs.webkit.org/attachment.cgi?id=395329
patch

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

I think this is a nice incremental improvement to the existing
code, but it needs a bit of work. I am particularly concerned
about the missing error checking when opening files. Could you
please look into the suggestions below?

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:67
> +    fflush(zoneInfoFile);

Why do you need to use fflush() here? The fseek() right above is
responsible to arrange whatever is needed internally inside the
libc to make the next read return data starting at the first byte
of the file.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:180
> +    fflush(cgroupControllerFile);

Same here, flushing after seeking shouldn't be needed.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:212
> +    fflush(memInfoFile);

…and the same here.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:327
> +	   FILE* cgroupControllerFile = fopen(s_procSelfCgroup, "r");

This is missing error checking. If the files cannot be opened
the FILE* variables will be null and trying to read from them
will be undefined behaviour. We need checks here, and do one of:

 1. Disable the memory pressure monitor. Probably not ideal.
 2. Retry opening the files periodically, optionally up to a
    certain maximum amount of attempts.
 3. Retry opening the files with exponential backoff.

For simplicity, I think we could go with option (2.) and retrying
every 5s, which is the same interval at which memory status reports
are gathered.