New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758026 link

Starred by 2 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 304341



Sign in to add a comment

Remove the WebContentsObserver::OnMessageReceived overload that doesn't take a RenderFrameHost

Project Member Reported by lukasza@chromium.org, Aug 22 2017

Issue description

There is a TODO in the code saying:
    // TODO(nick, creis): Replace all uses of this variant of OnMessageReceived
    // with the version that takes a RenderFrameHost, and delete it.

Let's use this bug to track addressing this TODO.
 
Components: Internals>Sandbox>SiteIsolation
Project Member

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

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

commit 89f2c1be46c14a33d4a484f414af8b6dd4934159
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Aug 30 21:36:46 2017

Use frame-specific WebContentsObserver::OnMessageReceived.

The 3 IPC messages touched by this CL are
1) already being sent with a routing id coming from
   RenderFrameObserver::routing_id.
2) already handled by a frame-specific
   WebContentsObserver::OnMessageReceived implemented by
   PluginObserver or PdfPluginPlaceholder

Therefore, it seems that the handlers of these IPCs in
ChromeWebViewPermissionHelperDelegate are incorrect
to listen for this IPCs on the view-related, not frame-specific
OnMessageReceived overload.

This CL moves handling of these IPCs to be consistently done through
frame-specific OnMessageReceived and removes the no-longer-needed
overload where possible (e.g. in WebViewPermissionHelper).

Bug: 758026
Change-Id: I62c42ee54f4ed12d3076a27ab8093cb1c30fd665
Reviewed-on: https://chromium-review.googlesource.com/627341
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498618}
[modify] https://crrev.com/89f2c1be46c14a33d4a484f414af8b6dd4934159/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
[modify] https://crrev.com/89f2c1be46c14a33d4a484f414af8b6dd4934159/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h
[modify] https://crrev.com/89f2c1be46c14a33d4a484f414af8b6dd4934159/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/89f2c1be46c14a33d4a484f414af8b6dd4934159/extensions/browser/guest_view/web_view/web_view_permission_helper.cc
[modify] https://crrev.com/89f2c1be46c14a33d4a484f414af8b6dd4934159/extensions/browser/guest_view/web_view/web_view_permission_helper.h

Comment 3 by a...@chromium.org, Aug 30 2017

This TODO should be in the WebContentsObserver interface too, then, as that's the public interface.
Project Member

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

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

commit cf0a94acf49b961d8ea74b9d43363d56ac7d9a6c
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Aug 31 19:10:09 2017

Remove unnecessary OnMessageReceived overload from RenderFrameHostTracker.

The UIThreadExtensionFunction::RenderFrameHostTracker is a
WebContentsObserver that forwards IPC messages to
UIThreadExtensionFunction::OnMessageReceived.

There is only one overload of
UIThreadExtensionFunction::OnMessageReceived -
PageCaptureSaveAsMHTMLFunction::OnMessageReceived and the only IPC it
handles (ExtensionHostMsg_ResponseAck) is always sent via RenderFrame
(in PageCaptureCustomBindings::SendResponseAck).

Therefore, we can safely remove the
RenderFrameHostTracker::OnMessageReceived overload that doesn't specify
the RenderFrameHost* sender.

Bug: 758026
Change-Id: If323ca7c42aafe0e53c6d0268ba66229fb622b23
Reviewed-on: https://chromium-review.googlesource.com/644509
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498945}
[modify] https://crrev.com/cf0a94acf49b961d8ea74b9d43363d56ac7d9a6c/content/public/browser/web_contents_observer.h
[modify] https://crrev.com/cf0a94acf49b961d8ea74b9d43363d56ac7d9a6c/extensions/browser/extension_function.cc

Project Member

Comment 5 by sheriffbot@chromium.org, Sep 3

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Recharge-Cold
This still needs to be cleaned-up.  According to code search the remaining overrides of the *RFH-less* OnMessageReceived are:
- BrowserPluginGuest::OnMessageReceived (handling IPCs like ViewHostMsg_LockMouse, ViewHostMsg_TakeFocus, ViewHostMsg_ShowWidget, ...)
- BlinkTestController::OnMessageReceived (handling LayoutTest IPCs)
- DeclarativeContentCssConditionTracker::PerWebContentsTracker::OnMessageReceived (handling a single ExtensionHostMsg_OnWatchedPageChange IPC)
- PrintPreviewObserver::OnMessageReceived (test-only implementation that handles PrintHostMsg_DidStartPreview)

Sign in to add a comment