Convert ChromeViewMsg_SetAllowRunningInsecureContent to use mojo.

Convert ChromeViewMsg_SetAllowRunningInsecureContent to use mojo.

The relevant browser_tests (which call
ContentSettingMixedScriptBubbleModel::OnCustomLinkClicked) are:

  - ContentSettingBubbleModelMixedScriptTest.MainFrame
  - ContentSettingsMixedScriptIgnoreCertErrorsTest.MainFrameMetrics

BUG=577685
Review-Url: https://codereview.chromium.org/2674843002
Cr-Commit-Position: refs/heads/master@{#449803}
Committed: https://chromium.googlesource.com/chromium/src/+/5d69ef86d128ef832cb680fc83b9595c8d4e0003

[nigeltao1, 2017-02-03 06:17:32]

nigeltao@chromium.org changed reviewers:
+ sammc@chromium.org

[nigeltao1, 2017-02-03 06:17:32]

https://codereview.chromium.org/2674843002/diff/1/content/public/app/mojo/con...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2674843002/diff/1/content/public/app/mojo/con...
content/public/app/mojo/content_renderer_manifest.json:38:
"chrome::mojom::InsecureContentRenderer",
It doesn't feel right to be adding a "chrome::foo" to a file under content/, but
I tried a similar change to
chrome/browser/chrome_content_browser_manifest_overlay.json instead, and got
test failures:

----
[1:1:0203/171108.004939:ERROR:interface_registry.cc(218)] InterfaceProviderSpec
"navigation:frame" prevented service: content_browser from binding interface:
chrome::mojom::InsecureContentRenderer exposed by: content_renderer
[1:1:0203/171108.005489:WARNING:interface_registry.cc(222)] 

InterfaceRegistry(navigation:frame):
Owned by:
  content_renderer@3_1 run as: cc86c190-0d41-4e98-8fa6-70f1738cee83

  Providing:
    capability: browser containing interfaces:
      autofill::mojom::AutofillAgent
      autofill::mojom::PasswordAutofillAgent
      autofill::mojom::PasswordGenerationAgent
      blink::mojom::AppBannerController
      blink::mojom::EngagementClient
      blink::mojom::InstallationService
      content::mojom::ImageDownloader
      contextual_search::mojom::OverlayPageNotifierService
      dom_distiller::mojom::DistillerPageNotifierService
      mojom::MediaDevicesListener

  Requiring:
    From: content_browser:
      renderer

Bound to:
  content_browser@ run as: cc86c190-0d41-4e98-8fa6-70f1738cee83

  Providing:
    capability: browser containing interfaces:
      chrome::mojom::InsecureContentRenderer
    capability: renderer containing interfaces:
      autofill::mojom::AutofillDriver
      autofill::mojom::PasswordManagerDriver
      blink::mojom::MediaSessionService
      blink::mojom::PermissionService
      blink::mojom::PresentationService
      blink::mojom::SensitiveInputVisibilityService
      blink::mojom::ShareService
      blink::mojom::WebBluetoothService
      blink::mojom::WebSocket
      bluetooth::mojom::AdapterFactory
      chrome::mojom::PrerenderCanceler
      content::mojom::BrowserTarget
      contextual_search::mojom::ContextualSearchJsApiService
      device::VibrationManager
      device::mojom::GeolocationService
      device::mojom::SensorProvider
      device::mojom::VRService
      device::mojom::WakeLockService
      device::nfc::mojom::NFC
      device::usb::ChooserService
      device::usb::DeviceManager
      discardable_memory::mojom::DiscardableSharedMemoryManager
      dom_distiller::mojom::DistillabilityService
      dom_distiller::mojom::DistillerJavaScriptService
      extensions::KeepAlive
      extensions::mime_handler::MimeHandlerService
      media::mojom::InterfaceFactory
      media::mojom::RemoterFactory
      media::mojom::Renderer
      media_router::mojom::MediaRouter
      mojom::MediaDevicesDispatcherHost
      mojom::OmniboxPageHandler
      mojom::PluginsPageHandler
      mojom::SiteEngagementUIHandler
      mojom::UsbInternalsPageHandler
      password_manager::mojom::CredentialManager
      payments::mojom::PaymentRequest
      translate::mojom::ContentTranslateDriver
      ui::mojom::Gpu

  Requiring:
    From: content_renderer:
      browser

Binders registered for:
 * autofill::mojom::AutofillAgent
 * autofill::mojom::PasswordAutofillAgent
 * autofill::mojom::PasswordGenerationAgent
 * blink::mojom::AppBannerController
 * blink::mojom::InstallationService
   chrome::mojom::InsecureContentRenderer
 * content::mojom::ImageDownloader
 * mojom::MediaDevicesListener

 * - denotes an interface exposed to remote per policy.
----

It's probably a simple fix to do it properly, but it's not obvious what I should
be doing instead.

[nigeltao1, 2017-02-03 06:17:41]

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

[commit-bot: I haz the power, 2017-02-03 06:17:59]

Dry run: 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, 2017-02-03 07:16:14]

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

[commit-bot: I haz the power, 2017-02-03 07:16:15]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-02-03 17:36:58]

https://codereview.chromium.org/2674843002/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2674843002/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1031: static
void SetAllowRunningInsecureContent(
Make this a function in the anonymous namespace.

https://codereview.chromium.org/2674843002/diff/1/chrome/common/insecure_cont...
File chrome/common/insecure_content_renderer.mojom (right):

https://codereview.chromium.org/2674843002/diff/1/chrome/common/insecure_cont...
chrome/common/insecure_content_renderer.mojom:11:
SetAllowRunningInsecureContent(bool allow, bool reload_frame);
These parameters seem unnecessary; the renderer can correctly predict their
values. Can we get rid of them?

https://codereview.chromium.org/2674843002/diff/1/content/public/app/mojo/con...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2674843002/diff/1/content/public/app/mojo/con...
content/public/app/mojo/content_renderer_manifest.json:38:
"chrome::mojom::InsecureContentRenderer",
On 2017/02/03 06:17:32, nigeltao1 wrote:
> It doesn't feel right to be adding a "chrome::foo" to a file under content/,
but
> I tried a similar change to
> chrome/browser/chrome_content_browser_manifest_overlay.json instead, and got
> test failures:
> 
> ----
> [1:1:0203/171108.004939:ERROR:interface_registry.cc(218)]
InterfaceProviderSpec
> "navigation:frame" prevented service: content_browser from binding interface:
> chrome::mojom::InsecureContentRenderer exposed by: content_renderer
> [1:1:0203/171108.005489:WARNING:interface_registry.cc(222)] 
> 
> InterfaceRegistry(navigation:frame):
> Owned by:
>   content_renderer@3_1 run as: cc86c190-0d41-4e98-8fa6-70f1738cee83
> 
>   Providing:
>     capability: browser containing interfaces:
>       autofill::mojom::AutofillAgent
>       autofill::mojom::PasswordAutofillAgent
>       autofill::mojom::PasswordGenerationAgent
>       blink::mojom::AppBannerController
>       blink::mojom::EngagementClient
>       blink::mojom::InstallationService
>       content::mojom::ImageDownloader
>       contextual_search::mojom::OverlayPageNotifierService
>       dom_distiller::mojom::DistillerPageNotifierService
>       mojom::MediaDevicesListener
> 
>   Requiring:
>     From: content_browser:
>       renderer
> 
> Bound to:
>   content_browser@ run as: cc86c190-0d41-4e98-8fa6-70f1738cee83
> 
>   Providing:
>     capability: browser containing interfaces:
>       chrome::mojom::InsecureContentRenderer
>     capability: renderer containing interfaces:
>       autofill::mojom::AutofillDriver
>       autofill::mojom::PasswordManagerDriver
>       blink::mojom::MediaSessionService
>       blink::mojom::PermissionService
>       blink::mojom::PresentationService
>       blink::mojom::SensitiveInputVisibilityService
>       blink::mojom::ShareService
>       blink::mojom::WebBluetoothService
>       blink::mojom::WebSocket
>       bluetooth::mojom::AdapterFactory
>       chrome::mojom::PrerenderCanceler
>       content::mojom::BrowserTarget
>       contextual_search::mojom::ContextualSearchJsApiService
>       device::VibrationManager
>       device::mojom::GeolocationService
>       device::mojom::SensorProvider
>       device::mojom::VRService
>       device::mojom::WakeLockService
>       device::nfc::mojom::NFC
>       device::usb::ChooserService
>       device::usb::DeviceManager
>       discardable_memory::mojom::DiscardableSharedMemoryManager
>       dom_distiller::mojom::DistillabilityService
>       dom_distiller::mojom::DistillerJavaScriptService
>       extensions::KeepAlive
>       extensions::mime_handler::MimeHandlerService
>       media::mojom::InterfaceFactory
>       media::mojom::RemoterFactory
>       media::mojom::Renderer
>       media_router::mojom::MediaRouter
>       mojom::MediaDevicesDispatcherHost
>       mojom::OmniboxPageHandler
>       mojom::PluginsPageHandler
>       mojom::SiteEngagementUIHandler
>       mojom::UsbInternalsPageHandler
>       password_manager::mojom::CredentialManager
>       payments::mojom::PaymentRequest
>       translate::mojom::ContentTranslateDriver
>       ui::mojom::Gpu
> 
>   Requiring:
>     From: content_renderer:
>       browser
> 
> Binders registered for:
>  * autofill::mojom::AutofillAgent
>  * autofill::mojom::PasswordAutofillAgent
>  * autofill::mojom::PasswordGenerationAgent
>  * blink::mojom::AppBannerController
>  * blink::mojom::InstallationService
>    chrome::mojom::InsecureContentRenderer
>  * content::mojom::ImageDownloader
>  * mojom::MediaDevicesListener
> 
>  * - denotes an interface exposed to remote per policy.
> ----
> 
> It's probably a simple fix to do it properly, but it's not obvious what I
should
> be doing instead.

You need to add it to
chrome/browser/chrome_content_renderer_manifest_overlay.json.

[nigeltao1, 2017-02-04 06:29:10]

https://codereview.chromium.org/2674843002/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2674843002/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1031: static
void SetAllowRunningInsecureContent(
On 2017/02/03 17:36:58, Sam McNally wrote:
> Make this a function in the anonymous namespace.

Done.

https://codereview.chromium.org/2674843002/diff/1/chrome/common/insecure_cont...
File chrome/common/insecure_content_renderer.mojom (right):

https://codereview.chromium.org/2674843002/diff/1/chrome/common/insecure_cont...
chrome/common/insecure_content_renderer.mojom:11:
SetAllowRunningInsecureContent(bool allow, bool reload_frame);
On 2017/02/03 17:36:58, Sam McNally wrote:
> These parameters seem unnecessary; the renderer can correctly predict their
> values. Can we get rid of them?

Done.

https://codereview.chromium.org/2674843002/diff/1/content/public/app/mojo/con...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2674843002/diff/1/content/public/app/mojo/con...
content/public/app/mojo/content_renderer_manifest.json:38:
"chrome::mojom::InsecureContentRenderer",
Duh, renderer, not browser. Thanks. Done.

[Sam McNally, 2017-02-09 22:57:27]

Conversion-wise this LGTM, but I'm not sure whether the subframes survive long
enough (with the main frame reload) for their notifications to matter.

[nigeltao1, 2017-02-10 05:38:19]

On 2017/02/09 22:57:27, Sam McNally wrote:
> Conversion-wise this LGTM, but I'm not sure whether the subframes survive long
> enough (with the main frame reload) for their notifications to matter.

Are you concerned about a use-after-free on a content::RenderFrame*, which is
unsafe, or merely that we might be doing wasteful (but safe) work?

It is true that the ordering is slightly different before/after, since I merged
two IPC messages into one Mojo RPC. Previously, with N frames, you'd get N+1
messages, N times SetAllowRunningInsecureContent and 1 times ReloadFrame, with
the ReloadFrame guaranteed to come last. Afterwards, you'd get N messages, and
the main frame would reload as a side effect, without necessarily happening
last.

Do you want me to split it back into two Mojo RPCs, to maintain ordering?

In any case, adding the usual suspects (R=sky,tsepez) for OWNERS.

[nigeltao1, 2017-02-10 05:38:36]

nigeltao@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[Sam McNally, 2017-02-10 05:48:09]

On 2017/02/10 05:38:19, nigeltao1 wrote:
> On 2017/02/09 22:57:27, Sam McNally wrote:
> > Conversion-wise this LGTM, but I'm not sure whether the subframes survive
long
> > enough (with the main frame reload) for their notifications to matter.
> 
> Are you concerned about a use-after-free on a content::RenderFrame*, which is
> unsafe, or merely that we might be doing wasteful (but safe) work?

Just wasteful work; nothing on the browser side will be deleted during the send
loop and mojo will safely discard any messages received after teardown. If the
subframes are going to be discarded as part of the reload, setting any state on
them seems wasteful. It's no worse post-conversion, and I might just be missing
something that explains what's going on here.
> 
> It is true that the ordering is slightly different before/after, since I
merged
> two IPC messages into one Mojo RPC. Previously, with N frames, you'd get N+1
> messages, N times SetAllowRunningInsecureContent and 1 times ReloadFrame, with
> the ReloadFrame guaranteed to come last. Afterwards, you'd get N messages, and
> the main frame would reload as a side effect, without necessarily happening
> last.
> 
> Do you want me to split it back into two Mojo RPCs, to maintain ordering?
> 
> In any case, adding the usual suspects (R=sky,tsepez) for OWNERS.

[sky, 2017-02-10 18:14:59]

LGTM

[Tom Sepez, 2017-02-10 19:30:36]

lgtm

[nigeltao1, 2017-02-10 23:13:38]

The CQ bit was checked by nigeltao@chromium.org

[commit-bot: I haz the power, 2017-02-10 23:14:14]

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, 2017-02-11 00:24:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486768418357090,
"parent_rev": "bfb6d62e98fe74682a7a0c81444b8f837bb48350", "commit_rev":
"5d69ef86d128ef832cb680fc83b9595c8d4e0003"}

[commit-bot: I haz the power, 2017-02-11 00:24:48]

Description was changed from

==========
Convert ChromeViewMsg_SetAllowRunningInsecureContent to use mojo.

The relevant browser_tests (which call
ContentSettingMixedScriptBubbleModel::OnCustomLinkClicked) are:

  - ContentSettingBubbleModelMixedScriptTest.MainFrame
  - ContentSettingsMixedScriptIgnoreCertErrorsTest.MainFrameMetrics

BUG=577685
==========

to

==========
Convert ChromeViewMsg_SetAllowRunningInsecureContent to use mojo.

The relevant browser_tests (which call
ContentSettingMixedScriptBubbleModel::OnCustomLinkClicked) are:

  - ContentSettingBubbleModelMixedScriptTest.MainFrame
  - ContentSettingsMixedScriptIgnoreCertErrorsTest.MainFrameMetrics

BUG=577685

Review-Url: https://codereview.chromium.org/2674843002
Cr-Commit-Position: refs/heads/master@{#449803}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d69ef86d128ef832cb680fc83b9...
==========

[commit-bot: I haz the power, 2017-02-11 00:24:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5d69ef86d128ef832cb680fc83b9...