[webkit-reviews] review denied: [Bug 34673] Crash in Flash at http://www.cctv.com/ : [Attachment 48303] patch v3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jon Honeycutt <jhoneycutt at apple.com>'s
request for review:
Bug 34673: Crash in Flash at http://www.cctv.com/
https://bugs.webkit.org/show_bug.cgi?id=34673

Attachment 48303: patch v3
https://bugs.webkit.org/attachment.cgi?id=48303&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks generally great. I have a few comments and a question or two.

I see why we need a map from NPP to PluginView*. That map seems to be something
PluginView* should just keep itself, not something that should be some another
class's responsibility.

I don't see why we keep the delayed destruction count inside a map. If we need
a count lets just keep it in the PluginView instead of using a boolean there.

> + * Copyright (C) 2009 Apple Inc. All Rights Reserved.

2010

> +class PluginDelayedDestruction : public Noncopyable {

I don't think this needs to be a class. We can just have free functions. But as
I mentioned above, I think this should just be PluginView responsibility
anyway.

> +    static void registerPluginView(PluginView&);
> +    static void unregisterPluginView(PluginView&);

If we had these, they should probably take pointers instead of references. Just
because we normally use pointers to manipulate these objects. I can't see any
strong arguments either way.

> +    static void setNeedsDelayedDestroy(NPP, bool);

This is a misleading function. It doesn't actually set/clear a per-NPP delayed
destroy flag. Instead each time it's called it either increments or decrements
a count. Thus you must call true before false and always balance each true with
a false.

> +    static void delayDestroy(PassRefPtr<PluginView> plugin) { new
PluginDelayedDestroyer(plugin); }

I think a logical name for this would be delayDestruction rather than
delayDestroy.

This is doing a similar thing for PluginView to what Frame::keepAlive does for
Frame, but it does it in a different way and uses different terminology. Maybe
we could unite the two mechanisms in terminology or even share code.

> +	   PluginDelayedDestruction::setNeedsDelayedDestroy(instance, true);

This is where the "why" comment needs to go. Someone has to say why!

I don't understand the code entirely. The idea seems to be that there are
certain critical windows within which if you destroy the plug-in it needs to be
destroyed on a later run loop iteration. But I don't see why when the critical
window ends you can't simply destroy the plug-in immediately at that time. If
you can't do it then, then I question whether the critical window actually has
closed. Or if there are any times that actually qualify as not in the critical
window.

I'm going to say review- because I think you should probably do at least one of
the things I mentioedn above.