Don't call NPP_SetWindow during the painting of windowless plugins.

On Windows, Flash seems to only start executing script actions after it received an NPP_SetWindow with a 
non-NULL NPWindow.window (HDC). It is possible that Flash then invokes JS to modify DOM of the page.

If Flash movie's widget is on-screen at page load, this call is made during layout and before even the NPP_Write is called,
which is the desired sequence of events.

However, if it is off-screen, this call occurs during painting, which leads to re-entrancy issues (layout while painting)
and bizarre crashes.

As a solution, we remove calls to NPP_SetWindow during painting and instead opt to never provide a null HDC to the plugin.
If no valid HDC is available, we feed it a disposable monochrome 1x1 context to have at least something to draw on.

R=ananta,darin,jam
BUG=16114
TEST=LayoutTests/plugins/flash-setwindow-paint-crash.html (bug reduction).

Review URL: http://codereview.chromium.org/159717

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@22383 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 63 insertions, 35 deletions

diff --git a/chrome/test/plugin/plugin_test.cpp b/chrome/test/plugin/plugin_test.cpp
index 5a5b844..dfed2f0 100644
--- a/chrome/test/plugin/plugin_test.cpp
+++ b/chrome/test/plugin/plugin_test.cpp
@@ -50,6 +50,7 @@
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/registry.h"
+#include "chrome/browser/net/url_request_mock_http_job.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/automation/tab_proxy.h"
@@ -94,15 +95,21 @@ class PluginTest : public UITest {
     UITest::SetUp();
   }
 
-  void TestPlugin(const std::wstring& test_case, int timeout) {
-    GURL url = GetTestUrl(test_case);
+  void TestPlugin(const std::wstring& test_case,
+                  int timeout,
+                  bool mock_http) {
+    GURL url = GetTestUrl(test_case, mock_http);
     NavigateToURL(url);
-    WaitForFinish(timeout);
+    WaitForFinish(timeout, mock_http);
   }
 
   // Generate the URL for testing a particular test.
   // HTML for the tests is all located in test_directory\plugin\<testcase>
-  GURL GetTestUrl(const std::wstring &test_case) {
+  // Set |mock_http| to true to use mock HTTP server.
+  GURL GetTestUrl(const std::wstring &test_case, bool mock_http) {
+    if (mock_http)
+      return URLRequestMockHTTPJob::GetMockUrl(L"plugin/" + test_case);
+
     FilePath path;
     PathService::Get(chrome::DIR_TEST_DATA, &path);
     path = path.AppendASCII("plugin");
@@ -111,11 +118,11 @@ class PluginTest : public UITest {
   }
 
   // Waits for the test case to finish.
-  void WaitForFinish(const int wait_time) {
+  void WaitForFinish(const int wait_time, bool mock_http) {
     const int kSleepTime = 500;      // 2 times per second
     const int kMaxIntervals = wait_time / kSleepTime;
 
-    GURL url = GetTestUrl(L"done");
+    GURL url = GetTestUrl(L"done", mock_http);
     scoped_refptr<TabProxy> tab(GetActiveTab());
 
     std::string done_str;
@@ -135,41 +142,46 @@ class PluginTest : public UITest {
 };
 
 TEST_F(PluginTest, Quicktime) {
-  TestPlugin(L"quicktime.html", kShortWaitTimeout);
+  TestPlugin(L"quicktime.html", kShortWaitTimeout, false);
 }
 
 TEST_F(PluginTest, DISABLED_MediaPlayerNew) {
-  TestPlugin(L"wmp_new.html", kShortWaitTimeout);
+  TestPlugin(L"wmp_new.html", kShortWaitTimeout, false);
 }
 
 // http://crbug.com/4809
 TEST_F(PluginTest, DISABLED_MediaPlayerOld) {
-  TestPlugin(L"wmp_old.html", kLongWaitTimeout);
+  TestPlugin(L"wmp_old.html", kLongWaitTimeout, false);
 }
 
 TEST_F(PluginTest, Real) {
-  TestPlugin(L"real.html", kShortWaitTimeout);
+  TestPlugin(L"real.html", kShortWaitTimeout, false);
 }
 
 TEST_F(PluginTest, Flash) {
-  TestPlugin(L"flash.html", kShortWaitTimeout);
+  TestPlugin(L"flash.html", kShortWaitTimeout, false);
 }
 
 TEST_F(PluginTest, FlashOctetStream) {
-  TestPlugin(L"flash-octet-stream.html", kShortWaitTimeout);
+  TestPlugin(L"flash-octet-stream.html", kShortWaitTimeout, false);
 }
 
 TEST_F(PluginTest, FlashSecurity) {
-  TestPlugin(L"flash.html", kShortWaitTimeout);
+  TestPlugin(L"flash.html", kShortWaitTimeout, false);
+}
+
+// http://crbug.com/16114
+TEST_F(PluginTest, FlashLayoutWhilePainting) {
+  TestPlugin(L"flash-layout-while-painting.html", kShortWaitTimeout, true);
 }
 
 // http://crbug.com/8690
 TEST_F(PluginTest, DISABLED_Java) {
-  TestPlugin(L"Java.html", kShortWaitTimeout);
+  TestPlugin(L"Java.html", kShortWaitTimeout, false);
 }
 
 TEST_F(PluginTest, Silverlight) {
-  TestPlugin(L"silverlight.html", kShortWaitTimeout);
+  TestPlugin(L"silverlight.html", kShortWaitTimeout, false);
 }
 
 typedef HRESULT (__stdcall* DllRegUnregServerFunc)();
@@ -185,7 +197,7 @@ class ActiveXTest : public PluginTest {
       RegisterTestControl(true);
       dll_registered = true;
     }
-    TestPlugin(test_case, timeout);
+    TestPlugin(test_case, timeout, false);
   }
   virtual void TearDown() {
     PluginTest::TearDown();
diff --git a/webkit/glue/plugins/webplugin_delegate_impl.cc b/webkit/glue/plugins/webplugin_delegate_impl.cc
index 996e272..05975f4 100644
--- a/webkit/glue/plugins/webplugin_delegate_impl.cc
+++ b/webkit/glue/plugins/webplugin_delegate_impl.cc
@@ -67,6 +67,38 @@ base::LazyInstance<iat_patch::IATPatchFunction> g_iat_patch_track_popup_menu(
 base::LazyInstance<iat_patch::IATPatchFunction> g_iat_patch_set_cursor(
     base::LINKER_INITIALIZED);
 
+// http://crbug.com/16114
+// Enforces providing a valid device context in NPWindow, so that NPP_SetWindow
+// is never called with NPNWindoTypeDrawable and NPWindow set to NULL.
+// Doing so allows removing NPP_SetWindow call during painting a windowless
+// plugin, which otherwise could trigger layout change while painting by
+// invoking NPN_Evaluate. Which would cause bad, bad crashes. Bad crashes.
+// TODO(dglazkov): If this approach doesn't produce regressions, move class to
+// webplugin_delegate_impl.h and implement for other platforms.
+class DrawableContextEnforcer {
+ public:
+  explicit DrawableContextEnforcer(NPWindow* window)
+      : window_(window),
+        disposable_dc_(window && !window->window) {
+    // If NPWindow is NULL, create a device context with monochrome 1x1 surface
+    // and stuff it to NPWindow.
+    if (disposable_dc_)
+      window_->window = CreateCompatibleDC(NULL);
+  }
+
+  ~DrawableContextEnforcer() {
+    if (!disposable_dc_)
+      return;
+
+    DeleteDC(static_cast<HDC>(window_->window));
+    window_->window = NULL;
+  }
+
+ private:
+  NPWindow* window_;
+  bool disposable_dc_;
+};
+
 }  // namespace
 
 WebPluginDelegate* WebPluginDelegate::Create(
@@ -906,9 +938,6 @@ void WebPluginDelegateImpl::WindowlessUpdateGeometry(
   if (window_rect == window_rect_ && clip_rect == clip_rect_)
     return;
 
-  // Set this flag before entering the instance in case of side-effects.
-  windowless_needs_set_window_ = true;
-
   // We will inform the instance of this change when we call NPP_SetWindow.
   clip_rect_ = clip_rect;
   cutout_rects_.clear();
@@ -943,19 +972,7 @@ void WebPluginDelegateImpl::WindowlessPaint(HDC hdc,
   damage_rect_win.right  = damage_rect_win.left + damage_rect.width();
   damage_rect_win.bottom = damage_rect_win.top + damage_rect.height();
 
-  // We need to pass the HDC to the plugin via NPP_SetWindow in the
-  // first paint to ensure that it initiates rect invalidations.
-  if (window_.window == NULL)
-    windowless_needs_set_window_ = true;
-
   window_.window = hdc;
-  // TODO(darin): we should avoid calling NPP_SetWindow here since it may
-  // cause page layout to be invalidated.
-
-  // We really don't need to continually call SetWindow.
-  // m_needsSetWindow flags when the geometry has changed.
-  if (windowless_needs_set_window_)
-    WindowlessSetWindow(false);
 
   NPEvent paint_event;
   paint_event.event = WM_PAINT;
@@ -985,10 +1002,7 @@ void WebPluginDelegateImpl::WindowlessSetWindow(bool force_set_window) {
   window_.x = window_rect_.x();
   window_.y = window_rect_.y();
   window_.type = NPWindowTypeDrawable;
-
-  if (!force_set_window)
-    // Reset this flag before entering the instance in case of side-effects.
-    windowless_needs_set_window_ = false;
+  DrawableContextEnforcer enforcer(&window_);
 
   NPError err = instance()->NPP_SetWindow(&window_);
   DCHECK(err == NPERR_NO_ERROR);
diff --git a/webkit/glue/plugins/webplugin_delegate_impl.h b/webkit/glue/plugins/webplugin_delegate_impl.h
index 9fec1f6..408a710 100644
--- a/webkit/glue/plugins/webplugin_delegate_impl.h
+++ b/webkit/glue/plugins/webplugin_delegate_impl.h
@@ -162,6 +162,8 @@ class WebPluginDelegateImpl : public WebPluginDelegate {
   gfx::Rect windowed_last_pos_;
 #endif
 
+  // TODO(dglazkov): No longer used by Windows, make sure the removal
+  // causes no regressions and eliminate from other platforms.
   // this is an optimization to avoid calling SetWindow to the plugin
   // when it is not necessary.  Initially, we need to call SetWindow,
   // and after that we only need to call it when the geometry changes.