Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget during Navigations

Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget during Navigations

When navigating the main frame, the RenderViewImpl lingers around with no WebFrameWidget.
This yields a WebViewImpl in a call to RenderWidget::GetWebWidget().

When user interactions leads to an IME IPC for FirstRectForCharacterRange, the IPC migth arrive during the
navigation and lead to an invalid cast in TextInputClientObserver::GetFocusedFrame().

This CL will avoid those casts by explicitly verifying that the WebWidget returned from GetWebWidget() is a WebFrameWidget.
Also, the IPC handling in TextInputClientObserver is modified such that it now considers the possibilities
of nullptr outputs from GetFocusedFrame() and GetWebFrameWidget().

BUG=664890
Committed: https://crrev.com/95df18c257f410cff287f0b402dd50b3014c7784
Cr-Commit-Position: refs/heads/master@{#434302}

[EhsanK, 2016-11-23 18:00:42]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 18:01:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-11-23 18:56:52]

Description was changed from

==========
Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget
during Navigations

When navigating the main frame, the RenderViewImpl lingers around with no
WebFrameWidget. This yields a WebViewImpl in a call to
RenderWidget::GetWebWidget().

When user interactions leads to an IME IPC for FirstRectForCharacterRange, the
IPC migth arrive during the navigation and lead to an invalid cast in
TextInputClientObserver::GetFocusedFrame().

This CL will avoid those casts by explicitly verifying that the WebWidget
returned from GetWebWidget() is a WebFrameWidget. Also, the IPC handling in
TextInputClientObserver is modified such that it now considers the possibilities
of nullptr outputs from GetFocusedFrame() and GetWebFrameWidget().

BUG=664890
==========

to

==========
Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget
during Navigations

When navigating the main frame, the RenderViewImpl lingers around with no
WebFrameWidget.
This yields a WebViewImpl in a call to RenderWidget::GetWebWidget().

When user interactions leads to an IME IPC for FirstRectForCharacterRange, the
IPC migth arrive during the
navigation and lead to an invalid cast in
TextInputClientObserver::GetFocusedFrame().

This CL will avoid those casts by explicitly verifying that the WebWidget
returned from GetWebWidget() is a WebFrameWidget.
Also, the IPC handling in TextInputClientObserver is modified such that it now
considers the possibilities
of nullptr outputs from GetFocusedFrame() and GetWebFrameWidget().

BUG=664890
==========

[EhsanK, 2016-11-23 19:02:13]

ekaramad@chromium.org changed reviewers:
+ avi@chromium.org

[EhsanK, 2016-11-23 19:02:13]

Hello Avi,

Could you please take a look at this. Also, a fix for this (Release-Block) bug
should probably be merged into 56.

Thanks,

Ehsan

[Avi (use Gerrit), 2016-11-23 21:17:36]

https://codereview.chromium.org/2526563004/diff/20001/content/renderer/text_i...
File content/renderer/text_input_client_observer.cc (right):

https://codereview.chromium.org/2526563004/diff/20001/content/renderer/text_i...
content/renderer/text_input_client_observer.cc:103:
mac::AttributedStringCoder::Encode(string));
Is it safe to pass nil to mac::AttributedStringCoder::Encode?

[EhsanK, 2016-11-23 21:31:32]

Thanks  Avi. PTAL.

https://codereview.chromium.org/2526563004/diff/20001/content/renderer/text_i...
File content/renderer/text_input_client_observer.cc (right):

https://codereview.chromium.org/2526563004/diff/20001/content/renderer/text_i...
content/renderer/text_input_client_observer.cc:103:
mac::AttributedStringCoder::Encode(string));
On 2016/11/23 21:17:36, Avi wrote:
> Is it safe to pass nil to mac::AttributedStringCoder::Encode?

I believe so. We already do this further below OnStringForRange().

In the Encode() method  we will get the NSString from NSAttributedString, i.e.,
[str string].
if (str == nil) then the return value of [str string] is "(null)"".

[Avi (use Gerrit), 2016-11-23 21:33:21]

lgtm

Hm. OK.

[EhsanK, 2016-11-23 22:04:23]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[EhsanK, 2016-11-23 22:05:00]

Thanks Avi. I will try to land this today and monitor Canary for crashes.
Hopefully if everything is green I will be able to merge this to M56.

Thanks!

[EhsanK, 2016-11-23 22:05:09]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-11-23 22:05:10]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-11-23 22:05:22]

The CQ bit was unchecked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-11-23 22:05:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-11-23 22:29:39]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[EhsanK, 2016-11-23 22:29:41]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-11-23 22:29:45]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-11-23 22:29:45]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-11-23 22:29:46]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2526563004/#ps40001
(title: "Fixed a Bug (and failing tests)")

[commit-bot: I haz the power, 2016-11-23 22:30:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 00:34:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 00:34:09]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[EhsanK, 2016-11-24 00:36:19]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-11-24 00:37:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 02:38:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 02:38:52]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[EhsanK, 2016-11-24 03:07:36]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-11-24 03:08:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 05:05:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479956856131090,
"parent_rev": "fb0e0c38e39a884ab4de660ab233d4ac1630df6a", "commit_rev":
"ede44504b8c6e7a90522f66aec48bd2c433db76f"}

[commit-bot: I haz the power, 2016-11-24 05:06:57]

Description was changed from

==========
Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget
during Navigations

When navigating the main frame, the RenderViewImpl lingers around with no
WebFrameWidget.
This yields a WebViewImpl in a call to RenderWidget::GetWebWidget().

When user interactions leads to an IME IPC for FirstRectForCharacterRange, the
IPC migth arrive during the
navigation and lead to an invalid cast in
TextInputClientObserver::GetFocusedFrame().

This CL will avoid those casts by explicitly verifying that the WebWidget
returned from GetWebWidget() is a WebFrameWidget.
Also, the IPC handling in TextInputClientObserver is modified such that it now
considers the possibilities
of nullptr outputs from GetFocusedFrame() and GetWebFrameWidget().

BUG=664890
==========

to

==========
Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget
during Navigations

When navigating the main frame, the RenderViewImpl lingers around with no
WebFrameWidget.
This yields a WebViewImpl in a call to RenderWidget::GetWebWidget().

When user interactions leads to an IME IPC for FirstRectForCharacterRange, the
IPC migth arrive during the
navigation and lead to an invalid cast in
TextInputClientObserver::GetFocusedFrame().

This CL will avoid those casts by explicitly verifying that the WebWidget
returned from GetWebWidget() is a WebFrameWidget.
Also, the IPC handling in TextInputClientObserver is modified such that it now
considers the possibilities
of nullptr outputs from GetFocusedFrame() and GetWebFrameWidget().

BUG=664890
==========

[commit-bot: I haz the power, 2016-11-24 05:06:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-24 05:09:21]

Description was changed from

==========
Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget
during Navigations

When navigating the main frame, the RenderViewImpl lingers around with no
WebFrameWidget.
This yields a WebViewImpl in a call to RenderWidget::GetWebWidget().

When user interactions leads to an IME IPC for FirstRectForCharacterRange, the
IPC migth arrive during the
navigation and lead to an invalid cast in
TextInputClientObserver::GetFocusedFrame().

This CL will avoid those casts by explicitly verifying that the WebWidget
returned from GetWebWidget() is a WebFrameWidget.
Also, the IPC handling in TextInputClientObserver is modified such that it now
considers the possibilities
of nullptr outputs from GetFocusedFrame() and GetWebFrameWidget().

BUG=664890
==========

to

==========
Fix renderer crashes due to non-existing RenderFrameImpl or WebFrameWidget
during Navigations

When navigating the main frame, the RenderViewImpl lingers around with no
WebFrameWidget.
This yields a WebViewImpl in a call to RenderWidget::GetWebWidget().

When user interactions leads to an IME IPC for FirstRectForCharacterRange, the
IPC migth arrive during the
navigation and lead to an invalid cast in
TextInputClientObserver::GetFocusedFrame().

This CL will avoid those casts by explicitly verifying that the WebWidget
returned from GetWebWidget() is a WebFrameWidget.
Also, the IPC handling in TextInputClientObserver is modified such that it now
considers the possibilities
of nullptr outputs from GetFocusedFrame() and GetWebFrameWidget().

BUG=664890

Committed: https://crrev.com/95df18c257f410cff287f0b402dd50b3014c7784
Cr-Commit-Position: refs/heads/master@{#434302}
==========

[commit-bot: I haz the power, 2016-11-24 05:09:22]

Patchset 3 (id:??) landed as
https://crrev.com/95df18c257f410cff287f0b402dd50b3014c7784
Cr-Commit-Position: refs/heads/master@{#434302}