Bug 209464 – Several refactorings done in the MemoryPressureMonitor.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Pablo Saavedra]

strtoull is a feature added in C++ since C11. This parses the C-string str interpreting its content as an `unsigned long long int` which is more appropriate  for the size_t (unsigned integer type) variables used by the `MemoryPressureMonitor.cpp` functions in counterpoint of atoll() what returns a `long long int`.

[Pablo Saavedra]

Created attachment 394354 [details]
patch

[Adrian Perez]

Comment on attachment 394354 [details]
patch

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

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:87
> +            low += strtoull(token, nullptr, 10);

This is not doing any error checking to ensure that the parsing was
successful. There are also a generic string-to-integer conversion
function in WTF which you could use here:

  bool ok = true;
  low += WTF::toIntegralType<size_t, UChar>(token, strlen(token), &ok, 10);
  if (!ok) return 0;  // Return early on error.

…and so on for the rest of the conversions.

[Pablo Saavedra]

Created attachment 394598 [details]
patch

[Pablo Saavedra]

Created attachment 394599 [details]
patch

[Pablo Saavedra]

Created attachment 394600 [details]
patch

[Pablo Saavedra]

Created attachment 394601 [details]
patch

[Pablo Saavedra]

(In reply to Adrian Perez from comment #2)
> Comment on attachment 394354 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394354&action=review
> 
> > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:87
> > +            low += strtoull(token, nullptr, 10);
> 
> This is not doing any error checking to ensure that the parsing was
> successful. There are also a generic string-to-integer conversion
> function in WTF which you could use here:
> 
>   bool ok = true;
>   low += WTF::toIntegralType<size_t, UChar>(token, strlen(token), &ok, 10);
>   if (!ok) return 0;  // Return early on error.
> 
> …and so on for the rest of the conversions.

Thanks for your hints. I've used those in my new patch. In addition, I decided to clean up some repetitive code and fix some issues that I found running WPE in a cgroup-v2 with unified hierarchy.

[Adrian Perez]

Comment on attachment 394601 [details]
patch

Thanks for the patch, having the checks for parsing numeric values in
places and some of the duplicated code factored out makes me more
confident in this being robust 👍️. Please check the comments before
landing.

It would be still nice to get rid of “strtok()” because often it is a
source of footguns; but I do not have any good suggestion that would
avoid writing a more formal parser… so let's just leave things as-is.

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

> Source/WebKit/ChangeLog:34
> +        getMemoryUsageWithCgroup() sighly. The name of the controller

Typo: sighly → slightly

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:232
> +    CString memoryControllerPath = CString();

You can remove the “= CString()” assignment. The default constructor
will initialize the value to an empty string anyway.

[Pablo Saavedra]

Created attachment 395030 [details]
patch

[Pablo Saavedra]

Created attachment 395031 [details]
patch

[EWS]

Committed r259287: <https://trac.webkit.org/changeset/259287>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395031 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61105994>