PlzNavigate: Move navigation-level mixed content checks to the browser.

content/public/browser/content_browser_client.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_CLIENT_H_
 #define CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_CLIENT_H_
 
 #include <stddef.h>
 
 #include <map>
@@ skipping 744 matching lines @@
 
   // Allows the embedder to provide its own AudioManager implementation.
   // If this function returns nullptr, a default platform implementation
   // will be used.
   virtual media::ScopedAudioManagerPtr CreateAudioManager(
       media::AudioLogFactory* audio_log_factory);
   // Creates and returns a factory used for creating CDM instances for playing
   // protected content.
   virtual std::unique_ptr<media::CdmFactory> CreateCdmFactory();
 
+  // Reports that passive mixed content was found at the specified WebContents.
+  virtual void PassiveInsecureContentFound(const GURL& resource_url);
+
+  // Checks if running of mixed-content is allowed for the specified
+  // WebContents/tab.
+  virtual bool ShouldAllowRunningInsecureContent(bool allowed_per_settings,

[jam, 2017/01/09 21:15:46]

this method should be on WebContentsDelegate. ContentBrowserClient is generally
used for:
1) embedder calls when there's no more specific delegate interface like WCD
2) web contents delegate methods that are common for all WebContents' used in
the browser. in this case, this method only makes sense for tabs that the user
sees (and the implementation depends on a class that is only added for browser
tabs). so then it can be implemented on chrome's Browser class only.

[carlosk, 2017/01/10 19:13:11]

Understood and thanks for this explanation.

But now I'm also kinda lost in the sea of 72 existing subclasses of WCD... I
know I need to have it implemented for:

* The default implementation, replacing ContentBrowserClient's implementation.
->>> Would be in WebContentsDelegate itself.

* User facing WebContents in Chrome, replacing ChromeContentBrowserClient's
implementation.
->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right
substitute?

* Supporting layout tests within content_shell, replacing
LayoutTestContentBrowserClient's implementation.
->>> As I can't find any WCD subclass under content/shell/browser/layout_test/ I
think I'd have to create a new one to hold that implementation.

* And now that I am thinking about this, it seems active mixed content should
also work for user facing WebContents in content_shell itself, and not be always
blocked. That would be as if I had overridden it in ShellContentBrowserClient.
->>> It seems content::Shell could implement this? As I couldn't find evidence
of settings or flags under content/shell/ I'm assuming they don't exist and I'd
simply return allowed_per_settings.


And shouldn't PassiveInsecureContentFound be moved too? It seems what you
described also applies to it.


Final sanity check: are you sure mixed content checks only applies to user
visible WebContents? I see WCD subclasses for things like extensions, headless,
prerender and more, and they make me wonder...

[jam, 2017/01/10 19:28:37]

right. browser is the one that's currently adding the MixedContentSettings tab
helper which the current implementation depends on.
content/shell/browser/shell.h is the WebContentsDelegate for content_shell (note
a WCD is always required)
yep SG
these don't get tabhelpers, i.e. the MixedContentSettings  class that you added
wouldn't run. so ChromeContentBrowserClient::ShouldAllowRunningInsecureContent
will crash with a null allowed_per_settings


you bring up a good point though. What's the current behavior for extensions,
prerender etc? we should maintain that behavior. if the renderer is doing it for
all types of RenderFrames, which it appears to, then we can keep this method on
ContentBrowserClient. But then you'll need to get the tab helper added for
extensions, prerender etc...

[carlosk, 2017/01/10 20:03:59]

There is logic that is specific to layout tests that shouldn't apply to
content_shell running by itself (see LayoutTestContentBrowserClient's
implementation). As test support is built in (and not in a separate target) it
might be possible to consolidate both in Shell.
I will move both then.
Then I'll keep the current behavior by changing the default return value I I am
currently using: instead of always returning false I'll return
allowed_per_settings' value. This will solve the content_shell case I mentioned
above and will also match the renderer behavior (see
FrameLoaderClientImpl::allowRunningInsecureContent)

[carlosk, 2017/01/11 03:32:21]

Done.

+                                                 const url::Origin& origin,
+                                                 const GURL& resource_url,
+                                                 WebContents* web_contents);
+
   // Populates |mappings| with all files that need to be mapped before launching
   // a child process.
 #if defined(OS_ANDROID)
   virtual void GetAdditionalMappedFilesForChildProcess(
       const base::CommandLine& command_line,
       int child_process_id,
       content::FileDescriptorInfo* mappings,
       std::map<int, base::MemoryMappedFile::Region>* regions) {}
 #elif defined(OS_POSIX) && !defined(OS_MACOSX)
   virtual void GetAdditionalMappedFilesForChildProcess(
@@ skipping 45 matching lines @@
   virtual bool ShouldRedirectDOMStorageTaskRunner();
 
   // If this returns true, all BrowserThreads (but UI/IO) that support it on
   // this platform will experimentally be redirected to TaskScheduler.
   virtual bool RedirectNonUINonIOBrowserThreadsToTaskScheduler();
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_CONTENT_BROWSER_CLIENT_H_