Fix DNS Prefetch in Chrome: new message type not added to filter.

Fix DNS Prefetch in Chrome: new message type not added to filter.

R=thakis@chromium.org
BUG=455342
Committed: https://crrev.com/913b668663b854e3d88dcf42f5e055a94d824fb1
Cr-Commit-Position: refs/heads/master@{#314972}

[gunsch, 2015-02-05 23:24:21]

On 2015/02/05 23:20:49, gunsch wrote:

Quick context:

this bug was added when componentizing dns_prefetch [1], moving the IPC message
out of render_messages.h [2] and not catching the new message type filtering in
ChromeRenderMessageFilter [3].

[1] https://codereview.chromium.org/644123002
[2]
https://codereview.chromium.org/644123002/diff/710001/chrome/common/render_me...
[3]
https://codereview.chromium.org/644123002/diff/710001/chrome/browser/renderer...

[Nico, 2015-02-05 23:48:48]

Thanks! Is it possible to write a test that makes sure nobody else breaks this
in the future?

[gunsch, 2015-02-05 23:49:59]

On 2015/02/05 23:48:48, Nico wrote:
> Thanks! Is it possible to write a test that makes sure nobody else breaks this
> in the future?

I don't know: is there a good way to write tests that cross the IPC boundary?

I don't think the DNS prefetch feature has ever had any end-to-end tests...

[Nico, 2015-02-05 23:53:05]

On 2015/02/05 23:49:59, gunsch wrote:
> On 2015/02/05 23:48:48, Nico wrote:
> > Thanks! Is it possible to write a test that makes sure nobody else breaks
this
> > in the future?
> 
> I don't know: is there a good way to write tests that cross the IPC boundary?

There's a bunch of tests that stub out one half of the boundary and manually
send things and then check that the other half sends back what's expected.

[gunsch, 2015-02-05 23:56:21]

On 2015/02/05 23:53:05, Nico wrote:
> On 2015/02/05 23:49:59, gunsch wrote:
> > On 2015/02/05 23:48:48, Nico wrote:
> > > Thanks! Is it possible to write a test that makes sure nobody else breaks
> this
> > > in the future?
> > 
> > I don't know: is there a good way to write tests that cross the IPC
boundary?
> 
> There's a bunch of tests that stub out one half of the boundary and manually
> send things and then check that the other half sends back what's expected.

Interesting. I think DNS prefetch is a bit of a black hole in that regard:
render process sends messages to the browser process, and the "indication" that
something happened is that the network stack did a DNS lookup. So AFAIK there
isn't currently a good signal that closes the loop.

One thing Patrick did (CC'd, on the linked bug) was a perf test showing a load
sequence that was much slower without having DNS prefetch working. If something
similar had existed on the perf bots, it would have probably been pretty obvious
when DNS prefetching stopped working. Thoughts?

[Nico, 2015-02-06 00:09:43]

Nah, testing through a perf test is bound to be flaky.

Could you stub out the other side instead and just check that the message to do
the dns lookup comes in (and then not do the lookup)?

[gunsch, 2015-02-06 02:02:01]

a little more perseverance and I managed to get an end-to-end browsertest added
to (an apparently-already-existing) PredictorBrowserTest. Verified it fails
without my change and passes with it.

[Nico, 2015-02-06 02:06:47]

lgtm, thanks!

https://codereview.chromium.org/874163005/diff/20001/chrome/browser/net/predi...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/874163005/diff/20001/chrome/browser/net/predi...
chrome/browser/net/predictor_browsertest.cc:111:
ASSERT_TRUE(test_server()->Start());
(if you only need this in that one test, consider doing it there – starting the
server takes a bit iirc. if this doesn't slow down running the
PredictorBrowserTest.* suite, ignore)

[gunsch, 2015-02-06 02:09:49]

New patchsets have been uploaded after l-g-t-m from thakis@chromium.org

[gunsch, 2015-02-06 02:10:10]

Thanks for the quick review!

https://codereview.chromium.org/874163005/diff/20001/chrome/browser/net/predi...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/874163005/diff/20001/chrome/browser/net/predi...
chrome/browser/net/predictor_browsertest.cc:111:
ASSERT_TRUE(test_server()->Start());
On 2015/02/06 02:06:47, Nico wrote:
> (if you only need this in that one test, consider doing it there
– starting the
> server takes a bit iirc. if this doesn't slow down running the
> PredictorBrowserTest.* suite, ignore)

Done.

[gunsch, 2015-02-06 02:10:23]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2015-02-06 02:11:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874163005/40001

[commit-bot: I haz the power, 2015-02-06 04:00:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-06 04:01:25]

Patchset 3 (id:??) landed as
https://crrev.com/913b668663b854e3d88dcf42f5e055a94d824fb1
Cr-Commit-Position: refs/heads/master@{#314972}