[webkit-reviews] review denied: [Bug 113466]=?UTF-8?Q?=20Don=E2=80=99t=20let=20Plug=2Dins=20use=20System=20V=20shared=20memory=20?=: [Attachment 195467] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 27 21:42:42 PDT 2013


Alexey Proskuryakov <ap at webkit.org> has denied Simon Cooper
<scooper at apple.com>'s request for review:
Bug 113466: Don’t let Plug-ins use System V shared memory
https://bugs.webkit.org/show_bug.cgi?id=113466

Attachment 195467: Patch
https://bugs.webkit.org/attachment.cgi?id=195467&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=195467&action=review


Looks pretty cool overall, but needs some style fixing, possibly multiple
rounds.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:41
> +#ifdef _SHIM_LOG_WITH_ASL
> +#include <asl.h>
> +#endif

Is there a specific reason to use asl here? That's not what we normally do.
Would fprintf(stderr, ...) or NSLog work?

The macro needs a more descriptive name (it does not matter that it's WITH_ASL
as far as I can tell). How about LOG_SYSV_SHARED_MEMORY_USAGE?

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:133
> +// This needs to C scoped
> +extern "C" {

This comment is not helpful, as it doesn't explain why. In fact, I'm not even
sure if this is true that we need C linkage.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:135
> +    // Fake System V shared memory. This is not thread safe.

You could add ASSERT(isMainThread()) to all functions to say this a little more
forcefully.

Would be nice to explain what is fake about it. Does it work as shared memory
at all?

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:139
> +#ifdef _SHIM_LOG_WITH_ASL
> +    static const char *lid = "PluginProcessShim:";
> +#endif

I'd just put this into each log call, it doesn't help readability or
hackability to have this elsewhere.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:149
> +	   struct fake_shm *next;
> +	   int		    refcnt;
> +	   key_t	    key;
> +	   size_t	    size;
> +	   size_t	    alloced_size;
> +	   int		    shmflag;
> +	   int		    shmid;
> +	   void 	   *address;

WebKit style: 
- no formatting, please just use a single space between type and name;
- we strongly prefer full words, and camelCase (so, alloced_size would probably
be allocatedSize or allocationSize).
- except for Objective-C types, stars stick to types.

	void* address;

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:152
> +    static struct fake_shm *fake_shm_head = NULL;

No "struct" in variable definitions in C++.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:155
> +    static key_t shim_ftok(const char *path, int id)

Misplaced star.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:163
> +    static struct fake_shm *find_by_shmid(int shmid)

No "struct", misplaced star, abbreviations.

These occur many times below, I won't repeat the same comment.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:167
> +	   while (shm_p != NULL) {

WebKit style:

while (shm_p) {
}

or better yet, with a more descriptive name than shm_p.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:229
> +	   if (shm_p->address != NULL) {
> +	       if (shmaddr == 0 || shmaddr == shm_p->address) {

WebKit style says to never compare to 0, so this should be 

if (shm_p->address) {
    if (!shmaddr || shmaddr == shm_p->address)

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:235
> +	       shm_p->alloced_size = (shm_p->size+4096) & ~0xFFF;
> +	       rval = shm_p->address = mmap((void *)shmaddr,
shm_p->alloced_size, PROT_READ|PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);

We always add spaces around binary operations.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:248
> +

Extra empty line.


More information about the webkit-reviews mailing list