redesign MockRenderProcessHost so that RenderProcessHost -> RenderProcessHostImpl is a safe cast |
|||
Issue description
In most of content/browser, there is a convention that casting from the public interface to its Impl is safe. RenderProcessHost is an exception to this, and the convention is strong enough that we sometimes see designs that hinge on bad casts, and a few of them are in the codebase at any time.
There are a few interrelated problems here.
First, RenderProcessHost is doing double-duty as both the public interface of RenderProcessHostImpl and as the base class for a Mock version of RenderProcessHostImpl. When we want to have an operation that works on both RenderProcessHostImpl and MockRenderProcessHost, the current pattern is to implement a static method on RenderProcessHostImpl that takes a RenderProcessHost*. Examples include:
static bool IsSuitableHost(RenderProcessHost* host,
BrowserContext* browser_context,
const GURL& site_url);
static void RegisterHost(int host_id, RenderProcessHost* host);
static void UnregisterHost(int host_id);
static void FilterURL(RenderProcessHost* rph, bool empty_allowed, GURL* url);
Second, RenderProcessHostImpl is just too big to mock effectively. There are a lot of behaviors in RenderProcessHostImpl today that could easily work in the context of a mocked-out IPC channel. Seems that historically, MockRenderProcessHost used to inherit from RenderProcessHostImpl (this changed as part of the content api extraction:
https://chromium.googlesource.com/chromium/src/+/f3b1a084a01ab82caf998daefcb989c66ff16135). I gather that we moved away from that because of problems with partial stubbing. Anyhow, there's a bunch of stuff that lives in RenderProcessHostImpl because it's a convenient place to put "stuff that we need only one of per launched renderer process" -- e.g. MessageFilters and instances of process-wide mojo interfaces (mojo::Renderer).
===
One fix would be for us to extract a content-private class
{RenderProcessHostBase : public RenderProcessHost}, and move methods there from
both the public interface (like RenderProcessHost::Init) and static methods from
RenderProcessHostImpl. RPH -> RPHB would be a safe cast.
A more extreme fix, would be to make RPH->RPHI a safe cast, ideally by moving
the child-process-launcher behaviors into an interface that's mockable, and
making RPHI a final class. MockRPH would really only exist as a
RenderProcessHostFactory that passes a mock process-laucher-delegate or whatever
instance to the RPHI ctor, plus some easy way to get at the IPC::TestSink and
friends. This actually sounds pretty good to me as an end state, though the
reality of what the code needs to get done seems like it could throw any number
of wrenches in the feasibility of this idea.
,
Oct 5 2016
Ah right, MockRenderProcessHost is content/public. It sounds like this is analoguous to the RenderFrameHostTester, a pattern that I believe is really hard to use outside of content/. Are there any alternatives to the following two choices?
Option 1: diamond inheritance
RenderProcessHost;
MockRenderProcessHost : public RenderProcessHost;
RenderProcessHostBase : public RenderProcessHost;
RenderProcessHostImpl : public RenderProcessHostBase;
MockRenderProcessHostImpl : public RenderProcessHostBase,
public MockRenderProcessHost;
Option 2:
RenderProcessHost;
RenderProcessHostTester {
static RenderProcessHostTester* For(RenderProcessHost);
RenderProcessHost* render_process_host();
}
RenderProcessHostBase : public RenderProcessHost;
RenderProcessHostImpl : public RenderProcessHostBase;
MockRenderProcessHostImpl : public RenderProcessHostBase,
public RenderProcessHostTester;
,
Oct 5 2016
plus 1 to option 1
,
Oct 5 2016
With option 1, is, it safe to static_cast from RenderProcessHost* to RenderProcessHostBase*? Or do we need virtual inheritance?
,
Oct 6 2017
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. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 16 2018
Since this came back up recently, adding note here. Diamond inheritance is something that should be avoided. If you do no use virtual inheritance for the shared base in RPH, then you end up with 2 RPHs conceptually inside the MRPHI. Even if there is no actual data elements, you will still have to disambiguate which method is being inherited into MRPHI either via overrides or using statements. If you use virtual inheritance, then you have to use dynamic_cast<> and I have no clue how that works out if we disable rtti.. I'm certain it can be made to work, but we fall into C++ edge case land.
,
Oct 17
|
|||
►
Sign in to add a comment |
|||
Comment 1 by jam@chromium.org
, Oct 5 2016