Summary: | Crash in Flash at http://www.cctv.com/ | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Honeycutt <jhoneycutt> | ||||||||||||||
Component: | Plug-ins | Assignee: | Jon Honeycutt <jhoneycutt> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | eric, gustavo, 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
Jon Honeycutt
2010-02-05 21:00:26 PST
Attachment 48284 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/240267 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.
Attachment 48284 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/238645 Attachment 48284 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/240336 Created attachment 48292 [details]
patch v2
Adds the files to the GNU makefile and the XCode project. Removes a static initializer.
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.
Attachment 48292 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/242134 Attachment 48292 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/242142 Created attachment 48303 [details]
patch v3
GTK and Qt build fixes.
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 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. (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. Created attachment 48376 [details]
patch v4
Attachment 48376 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/246207 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.
Created attachment 48391 [details]
patch v5
Mac build fixes.
Attachment 48391 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/250136 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.
Created attachment 48425 [details]
patch v6
Mac build fix.
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 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 Made Darin's suggested changes and landed in r54614. It appears this broke mac builds, but there was a fix attempt in http://trac.webkit.org/changeset/54617. 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. 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 Committed r54622: <http://trac.webkit.org/changeset/54622> |