Bug 34673

Summary: Crash in Flash at http://www.cctv.com/
Product: WebKit Reporter: Jon Honeycutt <jhoneycutt>
Component: Plug-insAssignee: Jon Honeycutt <jhoneycutt>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, gns, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
URL: http://www.cctv.com/
Attachments:
Description Flags
patch
jhoneycutt: commit-queue-
patch v2
jhoneycutt: commit-queue-
patch v3
darin: review-, jhoneycutt: commit-queue-
patch v4
jhoneycutt: commit-queue-
patch v5
jhoneycutt: commit-queue-
patch v6 darin: review+, jhoneycutt: commit-queue-

Description Jon Honeycutt 2010-02-05 21:00:26 PST
Created attachment 48284 [details]
patch

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

<rdar://problem/7436875>
Comment 1 Eric Seidel (no email) 2010-02-05 21:05:14 PST
Attachment 48284 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/240267
Comment 2 WebKit Review Bot 2010-02-05 21:07:29 PST
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.
Comment 3 WebKit Review Bot 2010-02-05 23:32:48 PST
Attachment 48284 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/238645
Comment 4 WebKit Review Bot 2010-02-05 23:40:22 PST
Attachment 48284 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/240336
Comment 5 Jon Honeycutt 2010-02-06 12:51:46 PST
Created attachment 48292 [details]
patch v2

Adds the files to the GNU makefile and the XCode project. Removes a static initializer.
Comment 6 WebKit Review Bot 2010-02-06 12:53:33 PST
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.
Comment 7 WebKit Review Bot 2010-02-06 13:30:53 PST
Attachment 48292 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/242134
Comment 8 WebKit Review Bot 2010-02-06 13:33:30 PST
Attachment 48292 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/242142
Comment 9 Jon Honeycutt 2010-02-06 22:19:07 PST
Created attachment 48303 [details]
patch v3

GTK and Qt build fixes.
Comment 10 WebKit Review Bot 2010-02-06 22:22:38 PST
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.
Comment 11 Darin Adler 2010-02-07 13:05:02 PST
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.
Comment 12 Jon Honeycutt 2010-02-08 16:35:37 PST
(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.
Comment 13 Jon Honeycutt 2010-02-08 16:36:23 PST
Created attachment 48376 [details]
patch v4
Comment 14 Eric Seidel (no email) 2010-02-08 16:38:08 PST
Attachment 48376 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/246207
Comment 15 WebKit Review Bot 2010-02-08 16:39:20 PST
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.
Comment 16 Jon Honeycutt 2010-02-08 20:59:02 PST
Created attachment 48391 [details]
patch v5

Mac build fixes.
Comment 17 Eric Seidel (no email) 2010-02-08 21:01:16 PST
Attachment 48391 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/250136
Comment 18 WebKit Review Bot 2010-02-08 21:04:27 PST
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.
Comment 19 Jon Honeycutt 2010-02-09 09:48:06 PST
Created attachment 48425 [details]
patch v6

Mac build fix.
Comment 20 WebKit Review Bot 2010-02-09 09:52:58 PST
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.
Comment 21 Darin Adler 2010-02-10 10:39:36 PST
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
Comment 22 Jon Honeycutt 2010-02-10 12:43:54 PST
Made Darin's suggested changes and landed in r54614.
Comment 23 Eric Seidel (no email) 2010-02-10 13:51:12 PST
It appears this broke mac builds, but there was a fix attempt in http://trac.webkit.org/changeset/54617.
Comment 24 Eric Seidel (no email) 2010-02-10 13:57:09 PST
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.
Comment 25 Xan Lopez 2010-02-10 14:01:01 PST
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
Comment 26 Eric Seidel (no email) 2010-02-10 14:01:14 PST
Committed r54622: <http://trac.webkit.org/changeset/54622>