[webkit-reviews] review granted: [Bug 181483] Poison small JSObject derivatives which only contain pointers : [Attachment 330932] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 181483: Poison small JSObject derivatives which only contain pointers
https://bugs.webkit.org/show_bug.cgi?id=181483

Attachment 330932: patch

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




--- Comment #13 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 330932
  --> https://bugs.webkit.org/attachment.cgi?id=330932
patch

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

r=me with feedback addressed.

Please remember to address comments in
https://bugs.webkit.org/show_bug.cgi?id=181483#c7.
Please also do a quick benchmark run (sunspider, octane, kraken) to make sure
that there are no perf surprises from this patch.  I typically attach the
benchmark results from the run for future reference.  I suggest that you do the
same.

> Source/JavaScriptCore/ChangeLog:20
> +	   I wrote a script that finds interesting things to poison or
> +	   generally harden. These stood out because they derive from
> +	   JSObject and only contain a few pointer or pointer-like fields,
> +	   and could therefore just be poisoned. This also requires some
> +	   template "improvements" to our poisoning machinery. Worth noting
> +	   is that I'm making PoisonedUniquePtr move-assignable and
> +	   move-constructible from unique_ptr, which makes it a better
> +	   drop-in replacement because we don't need to use
> +	   makePoisonedUniquePtr. This means function-locals can be
> +	   unique_ptr and get the nice RAII pattern, and once the function is
> +	   done you can just move to the class' PoisonedUniquePtr without
> +	   worrying.

Maybe you should consider removing makePoisonedUniquePtr and its uses, and make
it so that PoisonedUniquePtr is initialized in a consistent way.  You can do
that in a follow up patch.

> Source/WTF/wtf/Poisoned.h:31
> +#include <cstddef>
> +#include <utility>

I think our convention is that these go above the <wtf/...> headers unless
there's a reason that they need to go after.  Please fix.

> Source/WTF/wtf/PoisonedUniquePtr.h:32
> +#include <cstddef>
> +#include <memory>

Ditto.