Fix TextInputClientMac related crashes of Fullscreen RenderWidget

content/renderer/text_input_client_observer.cc

Index: content/renderer/text_input_client_observer.cc
diff --git a/content/renderer/text_input_client_observer.cc b/content/renderer/text_input_client_observer.cc
index 84c8b88a99e658eac7a9a8351081325aea8fb13f..8d2644102c19bf2e426247e5fc4c7f8811d2e6a3 100644
--- a/content/renderer/text_input_client_observer.cc
+++ b/content/renderer/text_input_client_observer.cc
@@ -8,6 +8,7 @@
 
 #include <memory>
 
+#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "content/common/text_input_client_messages.h"
 #include "content/renderer/pepper/pepper_plugin_instance_impl.h"
@@ -52,12 +53,12 @@ bool TextInputClientObserver::Send(IPC::Message* message) {
 
 blink::WebFrameWidget* TextInputClientObserver::GetWebFrameWidget() const {
   blink::WebWidget* widget = render_widget_->GetWebWidget();
-  // We should always receive a WebFrameWidget in the call above. RenderViewImpl
-  // however might return a WebView if the main frame is destroyed, but as long
-  // as there is a rendered page, we should not be in that state and the RVImpl
-  // should be returning a frame widget.
-  DCHECK(widget->isWebFrameWidget());
-  return static_cast<blink::WebFrameWidget*>(render_widget_->GetWebWidget());
+  // |widget| should only be a WebFrameWidget. It is however possible to get
+  // here through a fullscreen flash plugin where the |widget| is not a
+  // WebFrameWidget.
+  return widget->isWebFrameWidget()
+             ? static_cast<blink::WebFrameWidget*>(widget)
+             : nullptr;
 }
 
 blink::WebLocalFrame* TextInputClientObserver::GetFocusedFrame() const {
@@ -83,10 +84,12 @@ PepperPluginInstanceImpl* TextInputClientObserver::GetFocusedPepperPlugin()
 
 void TextInputClientObserver::OnStringAtPoint(gfx::Point point) {
 #if defined(OS_MACOSX)
+  NSAttributedString* string = nil;
   blink::WebPoint baselinePoint;
-  NSAttributedString* string = blink::WebSubstringUtil::attributedWordAtPoint(
-      GetWebFrameWidget(), point, baselinePoint);
-
+  if (GetWebFrameWidget()) {

[EhsanK, 2016/10/18 01:59:37]

When this is false, it is not necessarily because of a transient state (full
screen flash does not have a frame widget). 

I think the right thing to do is to not send the IPC for a full screen flash to
its RWH, but rather, send it to the RWH of embedder of the flash. I was not sure
however what the straightforward way of getting embedder's RWH from full screen
flash RWH is.

[Avi (use Gerrit), 2016/10/18 02:04:08]

Can we find out?

[alexmos, 2016/10/18 17:52:01]

Just to clarify, is this whole class for Mac dictionary popup?  How should it
behave for Flash (fullscreen or not)?  

If it's not supposed to do anything, then is it possible to detect this in the
browser process and not send the IPC (TextInputClientMsg_StringAtPoint?) in the
first place?  When Flash fullscreen is active, WebContentsImpl maintains the
fullscreened RWH's process ID and routing ID, and resolves them up in
GetFullscreenRenderWidgetHostView().  So you could check whether the RWH's view
is the same RWHV as that to detect whether you're in this mode or not.

If the dictionary popup needs to do something for Flash, then I don't know how
sending the IPC through the embedder's RWH would help.  Feels like either way
you'd need some special handling to extract data you need from the PepperWidget
(which is the WebWidget that seems to be used for FS Flash) and/or
RenderWidgetFullScreenPepper.

If you wanted to get to the embedder here on the renderer side, the
render_widget_'s opener_id_ should be set to the embedder RenderWidget (it's
currently not exposed to the outside though), or alternatively it seems you
could cast render_widget_ to RenderWidgetFullScreenPepper and then do
plugin()->render_frame() to get to the embedding frame.  Mentioning just in
case; not sure if you'll need this.

This doc is a bit outdated, but still has good explanations about how fullscreen
flash works (mostly browser side):
http://www.chromium.org/developers/design-documents/embedding-flash-fullscree...

[EhsanK, 2016/11/03 07:13:19]

Except for OnCharacterIndexForPoint which is for IME. I think for now we should
just stop sending the IPC when there is a fullscreen flash.  I will have to find
a nice way to keep track of the parent of the flash to send the IPC to from the
browser side.

Also I was not clear on how I could get the parent RenderWidget on the renderer
side? Is it possible to use the opener_id_ for that? Although, if we want the
IPC to be handled for flash by its parent, then we should as well send it to the
parent RenderWidget.

[alexmos, 2016/11/04 20:34:03]

Yes, I think you could opener_id_ to get the the parent RenderWidget, though
that's currently not really exposed, and I'd try to avoid using it if possible. 
I agree that we should just send it to the parent RenderWidget in the first
place.

[EhsanK, 2016/11/08 16:59:44]

Acknowledged.

+    string = blink::WebSubstringUtil::attributedWordAtPoint(
+        GetWebFrameWidget(), point, baselinePoint);
+  }
   std::unique_ptr<const mac::AttributedStringCoder::EncodedString> encoded(
       mac::AttributedStringCoder::Encode(string));
   Send(new TextInputClientReplyMsg_GotStringAtPoint(
@@ -98,29 +101,34 @@ void TextInputClientObserver::OnStringAtPoint(gfx::Point point) {
 
 void TextInputClientObserver::OnCharacterIndexForPoint(gfx::Point point) {
   blink::WebPoint web_point(point);
-  uint32_t index = static_cast<uint32_t>(
+  uint32_t index = 0;
+  if (GetWebFrameWidget()) {
+    index = static_cast<uint32_t>(
       GetFocusedFrame()->characterIndexForPoint(web_point));
+  }
   Send(new TextInputClientReplyMsg_GotCharacterIndexForPoint(
       render_widget_->routing_id(), index));
 }
 
 void TextInputClientObserver::OnFirstRectForCharacterRange(gfx::Range range) {
   gfx::Rect rect;
+  if (GetWebFrameWidget()) {
 #if defined(ENABLE_PLUGINS)
-  PepperPluginInstanceImpl* focused_plugin = GetFocusedPepperPlugin();
-  if (focused_plugin) {
-    rect = focused_plugin->GetCaretBounds();
-  } else
+    PepperPluginInstanceImpl* focused_plugin = GetFocusedPepperPlugin();

[lfg, 2016/10/19 17:40:32]

This looks strange to me. Why would flash receive the IME events when it's not
fullscreen, but stop when it goes fullscreen?

[EhsanK, 2016/11/03 07:13:19]

This is why I think we should send the IPC to the embedder of the flash. I
assume the fullscreen flash is still a focused pepper plugin, right?

I think for now we should stop sending the IPC. I will look into the IME issue
with characterIndexForPoint. But it will not break or crash IME (only perhaps
the composition window might appear at incorrect position. I don't think it
should stop us from landing this fix first).

+    if (focused_plugin) {
+      rect = focused_plugin->GetCaretBounds();
+    } else
 #endif
-  {
-    blink::WebLocalFrame* frame = GetFocusedFrame();
-    // TODO(yabinh): Null check should not be necessary.
-    // See crbug.com/304341
-    if (frame) {
-      blink::WebRect web_rect;
-      frame->firstRectForCharacterRange(range.start(), range.length(),
-                                        web_rect);
-      rect = web_rect;
+    {
+      blink::WebLocalFrame* frame = GetFocusedFrame();
+      // TODO(yabinh): Null check should not be necessary.
+      // See crbug.com/304341
+      if (frame) {
+        blink::WebRect web_rect;
+        frame->firstRectForCharacterRange(range.start(), range.length(),
+                                          web_rect);
+        rect = web_rect;
+      }
     }
   }
   Send(new TextInputClientReplyMsg_GotFirstRectForRange(
@@ -131,12 +139,14 @@ void TextInputClientObserver::OnStringForRange(gfx::Range range) {
 #if defined(OS_MACOSX)
   blink::WebPoint baselinePoint;
   NSAttributedString* string = nil;
-  blink::WebLocalFrame* frame = GetFocusedFrame();
-  // TODO(yabinh): Null check should not be necessary.
-  // See crbug.com/304341
-  if (frame) {
-    string = blink::WebSubstringUtil::attributedSubstringInRange(
-        frame, range.start(), range.length(), &baselinePoint);
+  if (GetWebFrameWidget()) {
+    blink::WebLocalFrame* frame = GetFocusedFrame();
+    // TODO(yabinh): Null check should not be necessary.
+    // See crbug.com/304341
+    if (frame) {
+      string = blink::WebSubstringUtil::attributedSubstringInRange(
+          frame, range.start(), range.length(), &baselinePoint);
+    }
   }
   std::unique_ptr<const mac::AttributedStringCoder::EncodedString> encoded(
       mac::AttributedStringCoder::Encode(string));