New issue
Advanced search Search tips

Issue 752596 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocking:
issue 751376
issue 751401



Sign in to add a comment

Use ReceivedBadMessage throughout guest_view code

Project Member Reported by mcnee@chromium.org, Aug 4 2017

Issue description

In components/guest_view/browser/ and parts of extensions/browser/guest_view/, we kill misbehaving renderers without using ReceivedBadMessage. Instead we directly shutdown the renderer process and record our own BadMessageTerminate_BPGM user action. We should use ReceivedBadMessage helpers so that we have consistent logging, metrics recording, and process termination.
 

Comment 1 by mcnee@chromium.org, Aug 4 2017

Blocking: 751401

Comment 2 by mcnee@chromium.org, Aug 4 2017

Blocking: 751376

Comment 3 by mcnee@chromium.org, Aug 9 2017

Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/33fc0784cd6711bca5014853c84ea7567d7c7c31

commit 33fc0784cd6711bca5014853c84ea7567d7c7c31
Author: Kevin McNee <mcnee@chromium.org>
Date: Fri Aug 11 14:07:17 2017

Use ReceivedBadMessage throughout guest_view code.

Some parts of guest_view are still using ad-hoc code for killing
misbehaving renderers where we directly shutdown the renderer process
and record our own BadMessageTerminate_BPGM user action.

We now use ReceivedBadMessage helpers to have consistent logging,
metrics recording, and process termination.

This introduces a new histogram Stability.BadMessageTerminated.GuestView
along with its enum BadMessageReasonGuestView. This also deprecates
BadMessageTerminate_BPGM.

Bug:  752596 
Change-Id: I5b58e45b069fdc71c60f9e904aca86f7fb529c1c
Reviewed-on: https://chromium-review.googlesource.com/606868
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493722}
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/components/guest_view/browser/BUILD.gn
[add] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/components/guest_view/browser/PRESUBMIT.py
[add] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/components/guest_view/browser/bad_message.cc
[add] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/components/guest_view/browser/bad_message.h
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/components/guest_view/browser/guest_view_manager.cc
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/extensions/browser/bad_message.h
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/tools/metrics/actions/actions.xml
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/33fc0784cd6711bca5014853c84ea7567d7c7c31/tools/metrics/histograms/update_bad_message_reasons.py

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7cfa826feb7cd761d348604b0e08b7ca0c162b9

commit e7cfa826feb7cd761d348604b0e08b7ca0c162b9
Author: Kevin McNee <mcnee@chromium.org>
Date: Wed Aug 16 18:01:27 2017

Kill renderer on unexpected message in OnAttachGuest.

If we receive a message from the renderer to
GuestViewMessageFilter::OnAttachGuest before the creation of
a GuestViewManager, then the renderer is misbehaving.

The message was just being dropped. We now kill the renderer.

Bug:  752596 
Change-Id: I8a46dd658ce36b251958f272bd36fe091e70b82d
Reviewed-on: https://chromium-review.googlesource.com/617164
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494856}
[modify] https://crrev.com/e7cfa826feb7cd761d348604b0e08b7ca0c162b9/components/guest_view/browser/guest_view_message_filter.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/144bfa703e937fd123986080b80bd671ee49e87b

commit 144bfa703e937fd123986080b80bd671ee49e87b
Author: Kevin McNee <mcnee@chromium.org>
Date: Mon Aug 21 16:54:09 2017

Don't send the browser a message to resize MHVGs before their creation.

When MimeHandlerViewContainer::DidResizeElement receives a geometry
update, it sends ExtensionsGuestViewHostMsg_ResizeGuest to the browser.
However, the first update happens before MimeHandlerViewContainer
messages the browser to create a MimeHandlerViewGuest.
ExtensionsGuestViewMessageFilter::OnResizeGuest will drop the resize
message. Moreover, OnResizeGuest considers this to be misbehaviour on
the part of the renderer, although it is not currently enforced.

We now wait for the browser to acknowledge the creation of the
MimeHandlerViewGuest before we send resize messages upon geometry
updates.

Bug:  752596 
Change-Id: I2a15da5a0ba396946b687893bd54d7fec0727aa8
Reviewed-on: https://chromium-review.googlesource.com/619449
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495965}
[modify] https://crrev.com/144bfa703e937fd123986080b80bd671ee49e87b/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc
[modify] https://crrev.com/144bfa703e937fd123986080b80bd671ee49e87b/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.h

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a29d1d5cd3c4126a190084ffec6b304718f64db

commit 0a29d1d5cd3c4126a190084ffec6b304718f64db
Author: Kevin McNee <mcnee@chromium.org>
Date: Mon Aug 21 20:26:07 2017

Kill renderer on unexpected message in OnResizeGuest.

If we receive a message from the renderer to
ExtensionsGuestViewMessageFilter::OnResizeGuest before the creation of
a GuestViewManager, then the renderer is misbehaving.

The message was just being dropped. We now kill the renderer.

Bug:  752596 
Change-Id: I216e93ae8eef7bcfce7f5aa4be718dcf758a4496
Reviewed-on: https://chromium-review.googlesource.com/617065
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496047}
[modify] https://crrev.com/0a29d1d5cd3c4126a190084ffec6b304718f64db/extensions/browser/guest_view/extensions_guest_view_message_filter.cc

Comment 8 by mcnee@chromium.org, Aug 21 2017

Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1739dce5f98f2d381caaa024f3586b8e1120b01c

commit 1739dce5f98f2d381caaa024f3586b8e1120b01c
Author: Kevin McNee <mcnee@chromium.org>
Date: Wed Aug 30 14:40:28 2017

Revert OnResizeGuest message order enforcement due to dropped resize.

This reverts the dropping of resize messages before the ack of guest
creation. Since this reintroduces the behaviour for which we kill the
renderer, it is also necessary to revert the killing of the
renderer so that the former can be safely reverted.

If the first resize seen by MimeHandlerViewContainer::DidResizeElement
is between when the creation message is sent and when the ack is
received, the resize will not be sent to the browser.

I'm reverting the enforcement for now in order to correct this on ToT
and will follow up with corrected logic so we can start enforcing
again.

Bug: 759895,  752596 
Change-Id: I5044b996ae25d196a43f07f41240f5cbb654dee1
Reviewed-on: https://chromium-review.googlesource.com/641337
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498458}
[modify] https://crrev.com/1739dce5f98f2d381caaa024f3586b8e1120b01c/extensions/browser/guest_view/extensions_guest_view_message_filter.cc
[modify] https://crrev.com/1739dce5f98f2d381caaa024f3586b8e1120b01c/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc
[modify] https://crrev.com/1739dce5f98f2d381caaa024f3586b8e1120b01c/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.h

Comment 10 by mcnee@chromium.org, Aug 30 2017

Status: Started (was: Fixed)
There was an issue with resizing messages being dropped (759895), the CLs related to OnResizeGuest were reverted since we're so close to the branch point. A proper fix that allows us to keep the enforcement will follow.
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb41e586f29a75f4fa298f512566da3c1c431d64

commit bb41e586f29a75f4fa298f512566da3c1c431d64
Author: Kevin McNee <mcnee@chromium.org>
Date: Thu Sep 07 21:37:56 2017

Re-enable renderer killing on unexpected message in OnResizeGuest.

We reverted the dropping of resize messages before the ack of guest
creation due to the first resize seen by
MimeHandlerViewContainer::DidResizeElement getting dropped if it is
between when the creation message is sent and when the ack is
received. We also reverted the enforcement in OnResizeGuest to make
the above revert safe.

We now only drop resize messages before the creation message, as
the creation message contains the necessary size information and
sending a resize message after the creation message is well-behaved.
This allows us to re-enable the enforcement.

Bug:  752596 , 759895
Change-Id: I0f6da2fe9a72cb9fa350ccda5f1db28780099725
Reviewed-on: https://chromium-review.googlesource.com/652769
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500396}
[modify] https://crrev.com/bb41e586f29a75f4fa298f512566da3c1c431d64/extensions/browser/guest_view/extensions_guest_view_message_filter.cc
[modify] https://crrev.com/bb41e586f29a75f4fa298f512566da3c1c431d64/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc

Status: Fixed (was: Started)

Sign in to add a comment