[NoStatePrefetch] Kill renderer after preload scanning

chrome/renderer/prerender/prerender_helper.cc

Index: chrome/renderer/prerender/prerender_helper.cc
diff --git a/chrome/renderer/prerender/prerender_helper.cc b/chrome/renderer/prerender/prerender_helper.cc
index 70f6637c8d45f478f4edafca01ca75ab0398989b..c8f6d9214fe8ddcadfce53abd289ae9ac60119c8 100644
--- a/chrome/renderer/prerender/prerender_helper.cc
+++ b/chrome/renderer/prerender/prerender_helper.cc
@@ -7,6 +7,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "chrome/common/prerender_messages.h"
+#include "content/common/view_messages.h"
 #include "content/public/renderer/document_state.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_view.h"
@@ -69,6 +70,15 @@ PrerenderMode PrerenderHelper::GetPrerenderMode(
   return helper->prerender_mode_;
 }
 
+// static
+void PrerenderHelper::SendPrefetchFinished(content::RenderFrame* render_frame) {

[clamy, 2016/10/12 11:19:57]

A few things here:

1) It's weird to use a ViewHostMsg for something that is keyed by a frame. This
immediately begets the question: what happens with iframes? Generally speaking
we tend to reserve View*Msg for things that interact with a RenderView* and use
Frame*Msg for things that interact with a RenderFrame*.

2) This is rightly causing a CHECKDEPS violation, and I would not support adding
an exception to the rule since this is a layering violation. You have two
alternatives there: either you add plumbing to RenderFrame so that it will send
a FrameHostMsg itself, or use a PrerenderHostMsg that you send from here. Since
the rest of the content/ code seems to be just forwarding this notification to
an observer in the chrome layer, I would go with the second solution, which
skips any modification to the content code.

3) If more changes to content/ are needed, maybe we should think seriously about
moving the prefetch code from chrome/ to content/. A situation where content/
half knows about it isn't ideal IMO.

[pasko, 2016/10/12 13:59:40]

Thanks!
Feels weird to me as well. I will attempt to change that and will let you know
how it goes.

PrerenderManager keys its slaves by RenderProcessHost*, so I thought about
sending a FrameHostMsg, but (a) was not sure whether it shares the channel with
PrerenderHelper and (b) found that it would need to be plumbed back from the
Frame to Process on the browser side, and seemed to be partially in content/,
which I was not happy about, did not look very closely though.
Ah, just realized what this violation actually is about: I saw includes of
content/public and thought that content/common was also OK, but apparently
content/common is internal to content? That makes sense.
See my thoughts about it above.
I considered it too, one problem was that currently PrerenderHostMsgs are only
handled for link rel=prerender messages, and are routed with help of
PrerenderLinkManager, which was not trivial to change for some reason. OK, this
would be my first thing to try now.
Sounds good. Currently we just hooked into Prerender to be able to do A/B
testing more comfortably and be able to move clients to the new thing one by
one. Once Prerender code is removed, we will know more what Prefetch requires to
touch, and it will then be easier to move some parts to content/ in case there
are benefits of doing so. We have not figured multi-prefetch in a process yet,
for example.

[mattcary, 2016/10/12 14:09:17]

Silly question: is the render_process_id_ that the link manager has the same id
as from prerender_contents->GetRenderViewHost()->GetProcess()->GetId()?

[clamy, 2016/10/12 14:16:26]

Regarding the RenderProcessHost* issue, the RenderProcessHost* you would get at
from RenderFrameHost is the same as the one from the RenderViewHost unless the
frame is in a different process from the top level frame (OOPIF, Top Document
isolation etc...). In that case, the RenderProcessHost* reflects which process
the frame happens to be rendered in. You could still get the top RPH by looking
at the RPH for the root of the FrameTree.
Precisely - you can include anything that is in content/public/* outside of
content/ but not the rest. In that case, content/public/common/ is fine, but not
content/common.

[pasko, 2016/10/12 14:28:09]

Thanks. We are prefetching a top-level frame only for now, so this is not an
issue.

Also, even though ViewHostMsg_ is sent here, it is terminated at the
RenderProcessHost as a control message (i.e. without routing), which is another
uncomfortable weirdness. Going to change that now. 

[pasko, 2016/10/12 14:28:09]

That's a good suggestion, thanks!

+  PrerenderHelper* helper = PrerenderHelper::Get(render_frame);
+  if (!helper)
+    return;
+  DCHECK_NE(helper->prerender_mode_, NO_PRERENDER);
+  helper->Send(new ViewHostMsg_PrefetchFinished());
+}
+
 bool PrerenderHelper::OnMessageReceived(
     const IPC::Message& message) {
   IPC_BEGIN_MESSAGE_MAP(PrerenderHelper, message)