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

Issue 828697 link

Starred by 4 users
Status: Assigned
Owner:
alex...@chromium.org
Cc:
creis@chromium.org
afakhry@chromium.org
alex...@chromium.org
█ ortuno@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 829235
issue 829688


Sign in to add a comment

RenderViewHost::UpdateWebkitPreferences might be causing bugs with OOPIFs

Project Member Reported by alex...@chromium.org, Apr 4 2018 
During a code review, I noticed that we've got a few places that update webkit prefs and then do something like 
  web_contents_->GetRenderViewHost()->UpdateWebkitPreferences()

This will only send the new prefs to the main frame's renderer process, but not to the OOPIF renderers.  This might be problematic; ideally we'd be sending the updated prefs to all RVHs in the WebContents, i.e. using WebContentsImpl::NotifyPreferencesChanged() instead which already does this.  Unfortunately, content/public currently exposes RenderViewHost::UpdateWebkitPreferences() and not the WebContents version.  We should probably fix this.

Looking at RVH::UpdateWebkitPreferences callers, some affected features might be VR (VrTabHelper::SetIsInVr), presentation receiver (ReceiverPresentationServiceDelegateImpl::CreateForWebContents), and strict mixed content checking in desktop PWA (HostedAppBrowserController::SetAppPrefsForWebContents).  I haven't checked whether any of those things might happen with OOPIFs on a loaded page, or whether they matter for subframes, but it's probably worth auditing.

Comment 1 by alex...@chromium.org, Apr 4 2018

Cc: afakhry@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 4 2018 

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

commit d70bf37b08c43fa8307aae7fd90086bad16e5a53
Author: Ahmed Fakhry <afakhry@google.com>
Date: Wed Apr 04 17:07:24 2018

Touch MLP CL1: Enable mobile-like behavior of webpages in tablet mode

When in tablet mode on Chrome OS, webpages should have a
mobile-like behavior such as:
- Shrink viewport contents to fit the window size.
- Mobile viewport style.

This also exposes WebContents::NotifyPreferencesChanged() so
that it's possible to trigger an update of all RVHs associated
with a WebContents including OOPIFs

BUG=822455,828697
TEST=browser_tests --gtest_filter=TabletModePageBehaviorTest.*

Change-Id: Id80316fdb7c504a8a6f40ca9127c13ca58a9eee9
Reviewed-on: https://chromium-review.googlesource.com/978519
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548104}
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/chromeos/BUILD.gn
[add] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/chromeos/chrome_content_browser_client_chromeos_part.cc
[add] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/chromeos/chrome_content_browser_client_chromeos_part.h
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/ui/ash/tablet_mode_client.cc
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/ui/ash/tablet_mode_client.h
[add] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/browser/ui/ash/tablet_mode_page_behavior_browsertest.cc
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/chrome/test/BUILD.gn
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/d70bf37b08c43fa8307aae7fd90086bad16e5a53/content/public/browser/web_contents.h

Comment 3 by ortuno@chromium.org, Apr 5 2018 

Thanks for raising this issue! I'm currently working on a patch to ensure the right security indicators are shown if for some reason we don't block mixed content inside PWA windows. Once that patch lands, I'll start auditing our code and make sure it works with subframes and OOPIFs.

Comment 4 by ortuno@chromium.org, Apr 5 2018

Blockedon: 829235

Comment 5 by ortuno@chromium.org, Apr 6 2018

Blockedon: 829688

Comment 6 by creis@chromium.org, May 24 2018

Labels: OS-Android
Owner: alex...@chromium.org
Status: Assigned (was: Available)
Assigning to Alex to take a look when he gets back, to get a sense for what's remaining.  (It sounds like at least some might be Android, like the VR cases.)
►

Sign in to add a comment