Bug 34673 – Crash in Flash at http://www.cctv.com/

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Jon Honeycutt]

Created attachment 48284 [details]
patch

Safari crashes in Flash loading http://www.cctv.com/.

<rdar://problem/7436875>

[Eric Seidel (no email)]

Attachment 48284 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/240267

[WebKit Review Bot]

Attachment 48284 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/plugins/PluginDelayedDestruction.cpp:55:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/NP_jsobject.cpp:194:  _NPN_Evaluate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:76:  Declaration has space between type name and * in NPObject *windowScriptObject  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 48284 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/238645

[WebKit Review Bot]

Attachment 48284 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/240336

[Jon Honeycutt]

Created attachment 48292 [details]
patch v2

Adds the files to the GNU makefile and the XCode project. Removes a static initializer.

[WebKit Review Bot]

Attachment 48292 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/plugins/PluginDelayedDestruction.cpp:61:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:76:  Declaration has space between type name and * in NPObject *windowScriptObject  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/NP_jsobject.cpp:194:  _NPN_Evaluate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 48292 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/242134

[WebKit Review Bot]

Attachment 48292 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/242142

[Jon Honeycutt]

Created attachment 48303 [details]
patch v3

GTK and Qt build fixes.

[WebKit Review Bot]

Attachment 48303 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/plugins/PluginDelayedDestruction.cpp:61:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:76:  Declaration has space between type name and * in NPObject *windowScriptObject  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/NP_jsobject.cpp:194:  _NPN_Evaluate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 48303 [details]
patch v3

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.

[Jon Honeycutt]

(In reply to comment #11)
> (From update of attachment 48303 [details])
> 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.

I moved this map into PluginView.cpp and removed the count.

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

I removed this file.

> 
> > +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.

I removed this class.

> 
> > +    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.

Removed these functions.

> 
> > +    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.

Removed this function.

> 
> > +    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.

The new patch mimics Frame's method for doing this.

> 
> > +        PluginDelayedDestruction::setNeedsDelayedDestroy(instance, true);
> 
> This is where the "why" comment needs to go. Someone has to say why!

I added a comment here.

> 
> 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.

Flash calls NPN_Evaluate in response to receiving an event from the system, not the browser, so we cannot wait until we return from Flash to destroy the view as we do elsewhere. I believe that Flash could cause the Windows message loop to run, so I'm not sure that there is a way to guarantee that it's safe to destroy the view at any time. However, I believe that this is what Firefox does.

[Jon Honeycutt]

Created attachment 48376 [details]
patch v4

[Eric Seidel (no email)]

Attachment 48376 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/246207

[WebKit Review Bot]

Attachment 48376 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bridge/NP_jsobject.cpp:194:  _NPN_Evaluate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:76:  Declaration has space between type name and * in NPObject *windowScriptObject  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jon Honeycutt]

Created attachment 48391 [details]
patch v5

Mac build fixes.

[Eric Seidel (no email)]

Attachment 48391 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/250136

[WebKit Review Bot]

Attachment 48391 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bridge/NP_jsobject.cpp:194:  _NPN_Evaluate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:76:  Declaration has space between type name and * in NPObject *windowScriptObject  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jon Honeycutt]

Created attachment 48425 [details]
patch v6

Mac build fix.

[WebKit Review Bot]

Attachment 48425 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:76:  Declaration has space between type name and * in NPObject *windowScriptObject  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/NP_jsobject.cpp:194:  _NPN_Evaluate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 48425 [details]
patch v6

> +        // There is a crash in Flash when evaluating a script that destroys the
> +        // PluginView, so we destroy is asynchronously.

Typo:"destroy is" with "destroy it".

> +static void executeScript(const PluginObject* obj, const char* script)

We should use "object" instead of "obj".

r=me

[Jon Honeycutt]

Made Darin's suggested changes and landed in r54614.

[Eric Seidel (no email)]

It appears this broke mac builds, but there was a fix attempt in http://trac.webkit.org/changeset/54617.

[Eric Seidel (no email)]

My local machine still fails to build.  :(

c1plus: warnings being treated as errors
/Users/eseidel/Projects/WebKit/WebCore/plugins/PluginViewNone.cpp:76: warning: unused parameter ‘variable’
/Users/eseidel/Projects/WebKit/WebCore/plugins/PluginViewNone.cpp:76: warning: unused parameter ‘value’
distcc[40179] ERROR: compile /Users/eseidel/Projects/WebKit/WebCore/plugins/PluginViewNone.cpp on localhost failed

I'll see if John is on IRC, otherwise I'll write up a fix.

[Xan Lopez]

This commit causes two assertions in the 32bit GTK+ debug build: http://build.webkit.org/results/GTK Linux 64-bit Debug/r54614 (3133)/results.html

[Eric Seidel (no email)]

Committed r54622: <http://trac.webkit.org/changeset/54622>