565163 - e10s: nsHTMLDNSPrefetch needs to be forwarded to chrome

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jason Duell]

We do DNS prefetching of URIs in web pages, as implemented in bug 453403.  The logic that calls the DNS prefetching is in content, but DNS will live only in chrome, so we need to forward these requests to chrome.

From a glance at the code it looks like nsHTMLDNSPrefetch::Prefetch is the place we need to hook, to send an async msg to chrome and have it do the DNS resolution.  The IPDL message could probably just belong to PNecko (it could live in a "PDNSPrefetch.ipdl" subprotocol of PNecko, but there's not much point in this for a single method IMO).

There is also nsDNSPrefetch, but from a look at mxr it's only called by nsHttpChannel::AsyncOpen, which already happens only in chrome, so no work is needed.

Comment 1

[Jae-Seong Lee-Russo]

Attachment: patch, 0

Comment 2

[Josh Matthews [:jdm]]

>+bool
>+RecvHTMLDNSPrefetch(const nsAutoString& hostname, const PRUint16& flags)

I don't see how this builds.  This needs to be in NeckoParent.

>+  nsHTMLDNSPrefetch::nsListener *dnsListener =
>+    new nsHTMLDNSPrefetch::nsListener();
>+  if (!dnsListener)
>+    return false;

Infallible new means that the null check is unneeded.

However, overall I'm curious as to why you're replicating all the work of Prefetch in RecvHTMLDNSPrefetch instead of simply calling dnsService->Prefetch(hostname, flags)?

Comment 3

[Jae-Seong Lee-Russo]

(In reply to comment #2)
> >+bool
> >+RecvHTMLDNSPrefetch(const nsAutoString& hostname, const PRUint16& flags)
> 
> I don't see how this builds.  This needs to be in NeckoParent.

It somehow built on windows and Linux, so I didn't.  I will add it.

> However, overall I'm curious as to why you're replicating all the work of
> Prefetch in RecvHTMLDNSPrefetch instead of simply calling
> dnsService->Prefetch(hostname, flags)?

I couldn't think of a better way.  Josh, do you have other advice?  dnsService->Prefetch didn't work (class nsIDNSService’ has no member named ‘Prefetch’).

Comment 4

[Jae-Seong Lee-Russo]

Attachment: patch, 1

Comment 5

[Jae-Seong Lee-Russo]

Comment on attachment 447485 [details] [diff] [review]
patch, 1

This patch built fine on windows, but fails to build on Linux.

Comment 6

[Jae-Seong Lee-Russo]

Attachment: patch, 2

Comment 7

[Steffen Imhof]

Attachment: Updated the existing patch in a few areas

I tried out the attached patch version 2 and for me it looked like there were basically two things missing:

 - the whole DNS prefetching is not active in the content process
 - there are two methods to prefetch (hostname and Link based) and only one was IPC'ed

I attached an updated version of the patch that tries to fix these issues and also contains some slight updates to make the patch apply on current mozilla-central.

One other thing that struck me as odd, was the explicit usage of nsAutoString in the IPDL interface. I had no luck with nsAString but settled for nsString as some kind of compromise.

Comment 8

[Josh Matthews [:jdm]]

>+    // IPC the performance hit should be negligable.

s/negligable/negligible/

>+    if (NS_SUCCEEDED(aElement->GetHostname(hostname))) {
>+      if (!mozilla::net::gNeckoChild->
>+          SendHTMLDNSPrefetch(hostname, flags))
>+        return NS_ERROR_UNEXPECTED;
>+      return NS_OK;
>+    }

If GetHostname fails, we'll fall through to the regular prefetching.  Let's have an explicit failure here.  Also, there's no need to check return values for Send functions in the child.

> nsresult
> nsHTMLDNSPrefetch::Prefetch(nsAString &hostname, PRUint16 flags)
> {
>+#ifdef MOZ_IPC
>+  if (mozilla::net::IsNeckoChild()) {
>+    if (!mozilla::net::gNeckoChild->
>+        SendHTMLDNSPrefetch(nsAutoString(hostname), flags))
>+      return NS_ERROR_UNEXPECTED;
>+    return NS_OK;
>+  }
>+#endif
>+

Same change here.  In fact, why not |return Prefetch(hostname, flags)| from the earlier one and minimize changes?

>+#include "prenv.h"

Is this necessary?

>+  if (NS_FAILED(rv))
>+    return false;
>+  return true;

return NS_SUCCEEDED(rv);

Otherwise it looks pretty good to me.  I agree about the use of nsAutoString; you should just be able to pass the IPDL parameters as-is to Prefetch, so give that a try please.

Comment 9

[Steffen Imhof]

(In reply to comment #8)
> >+    // IPC the performance hit should be negligable.
> 
> s/negligable/negligible/
> 
> >+    if (NS_SUCCEEDED(aElement->GetHostname(hostname))) {
> >+      if (!mozilla::net::gNeckoChild->
> >+          SendHTMLDNSPrefetch(hostname, flags))
> >+        return NS_ERROR_UNEXPECTED;
> >+      return NS_OK;
> >+    }
> 
> If GetHostname fails, we'll fall through to the regular prefetching.  Let's
> have an explicit failure here.  Also, there's no need to check return values
> for Send functions in the child.
> 
> > nsresult
> > nsHTMLDNSPrefetch::Prefetch(nsAString &hostname, PRUint16 flags)
> > {
> >+#ifdef MOZ_IPC
> >+  if (mozilla::net::IsNeckoChild()) {
> >+    if (!mozilla::net::gNeckoChild->
> >+        SendHTMLDNSPrefetch(nsAutoString(hostname), flags))
> >+      return NS_ERROR_UNEXPECTED;
> >+    return NS_OK;
> >+  }
> >+#endif
> >+
> 
> Same change here.  In fact, why not |return Prefetch(hostname, flags)| from the
> earlier one and minimize changes?

Thanks, good points. I'll fix these issues.
 
> >+#include "prenv.h"
> 
> Is this necessary?

IsNeckoChild() is defined in the header and it uses PR_GetEnv(). (I guess it was not needed before, because preenv.h was indirectly included in every place NeckoCommon.h was included.)

> >+  if (NS_FAILED(rv))
> >+    return false;
> >+  return true;
> 
> return NS_SUCCEEDED(rv);

Ok.

> Otherwise it looks pretty good to me.  I agree about the use of nsAutoString;
> you should just be able to pass the IPDL parameters as-is to Prefetch, so give
> that a try please.

Hmm, there are two cases where I currently still use nsAutoString:
 1. in NeckoParent::RecvHTMLDNSPrefetch() when calling nsHTMLDNSPrefetch::Prefetch() something is needed, because the IPDL parameter is const reference while the original prefetch method expects a non-const reference for whatever reason. To fix that I would have to change the signature of nsHTMLDNSPrefetch's methods. Not sure if this would be ok?!

 2. in nsHTMLDNSPrefetch::Prefetch() when calling NeckoChild::SendHTMLDNSPrefetch() I need an nsString instead of the nsAString, because I did not manage to use nsAString in the IPDL interface. That might be solvable though. My problem was that the generated code wanted to have a constructable object and nsAString obviously was not. Any ideas?

Comment 10

[Josh Matthews [:jdm]]

Actually, I wouldn't worry about the nsAutoString usage.  Since we're just passing hostnames around they're most likely going to be shorter strings, so keeping them in stack-allocated char buffers (as nsAutoString does) is fine.

Comment 11

[Steffen Imhof]

Attachment: Patch updated according to Josh's comments

Updated the patch after Josh's remarks.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 459871 [details] [diff] [review]
Patch updated according to Josh's comments

>diff --git a/content/html/content/src/Makefile.in b/content/html/content/src/Makefile.in
>--- a/content/html/content/src/Makefile.in
>+++ b/content/html/content/src/Makefile.in
>@@ -47,16 +47,25 @@ LIBRARY_NAME	= gkconhtmlcon_s
> LIBXUL_LIBRARY	= 1
> 
> 
> EXPORTS		= \
> 		nsImageMapUtils.h \
> 		nsClientRect.h \
> 		$(NULL)
> 
>+ifdef MOZ_IPC
>+EXPORTS        += \
>+                nsHTMLDNSPrefetch.h \
>+                $(NULL)
>+
>+include $(topsrcdir)/config/config.mk
>+include $(topsrcdir)/ipc/chromium/chromium-config.mk

But these two includes but the others config includes below.  There is no need to protect them with MOZ_IPC.


> nsresult
> nsHTMLDNSPrefetch::Prefetch(Link *aElement, PRUint16 flags)
> {
>+#ifdef MOZ_IPC
>+  if (mozilla::net::IsNeckoChild()) {
>+    // Instead of transporting the Link object to the other process
>+    // we are using the hostname based function here, too. Compared to the 
>+    // IPC the performance hit should be negligible.

I am not sure I understand... why not just pass the whole URI?  we already have support for serialization of nsIURI.

>-
>+  

revert ws change.



> 
>+    // allow DNS prefetching for content processes, which is turned off
>+    // by default in embedding scenarios

I don't understand this comment.

>+    nsCOMPtr<nsIWebBrowserSetup> webBrowserSetup = do_QueryInterface(baseWindow);
>+    if (webBrowserSetup) {
>+      webBrowserSetup->SetProperty(nsIWebBrowserSetup::SETUP_ALLOW_DNS_PREFETCH,
>+                                   PR_TRUE);
>+    } else {
>+        NS_WARNING("baseWindow doesn't QI to nsIWebBrowserSetup, skipping "
>+                   "prefetch enable step.");


> #include "nsXULAppAPI.h"
>+#include "prenv.h"


why?



>+bool
>+NeckoParent::RecvHTMLDNSPrefetch(const nsString& hostname,
>+                                 const PRUint16& flags)
>+{
>+  nsAutoString h(hostname);

why the temp?

>+  nsresult rv = nsHTMLDNSPrefetch::Prefetch(h, flags);
>+  return NS_SUCCEEDED(rv);

Just return true.  the ipc stuff doesn't care about your failure.

Getting close!

Comment 13

[Steffen Imhof]

(In reply to comment #12)
Thanks for the review, I will fix the issues right away, just some explanations and further questions below:

> >+#ifdef MOZ_IPC
> >+  if (mozilla::net::IsNeckoChild()) {
> >+    // Instead of transporting the Link object to the other process
> >+    // we are using the hostname based function here, too. Compared to the 
> >+    // IPC the performance hit should be negligible.
> 
> I am not sure I understand... why not just pass the whole URI?  we already have
> support for serialization of nsIURI.

Hmm, but we have Link object here. I could ask it for the whole URI instead of the hostname, but is that actually helping for the DNS lookup? To call the Link based function on the parent side I would have to create a new Link object from that. Or am I missing something here?

> >+    // allow DNS prefetching for content processes, which is turned off
> >+    // by default in embedding scenarios
> 
> I don't understand this comment.

Would
    // IPC uses a WebBrowser object for which DNS prefetching is turned off
    // by default. But here we really want it, so enable it explicitly
be more clear?

> > #include "nsXULAppAPI.h"
> >+#include "prenv.h"
>  
> why?

PR_GetEnv is used, see also comment #9

> >+  nsAutoString h(hostname);
> 
> why the temp?

nsHTMLDNSPrefetch::Prefetch needs a non-const reference, see also comment #9

Comment 14

[Steffen Imhof]

Attachment: V5 of patch

Updated the patch addressing most of Doug's concerns.

I changed the unclear comment as indicated in my comment above and I did not yet change the hostname to a full URI in the case of the Link based DNS prefetch.

Comment 15

[Jason Duell]

dougt landed this, but looks like he forgot to close this bug. Reopen if I'm confused.

http://hg.mozilla.org/mozilla-central/rev/0fc77dad4ec7

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 459927 [details] [diff] [review]
V5 of patch

> include $(topsrcdir)/config/rules.mk
>+include $(topsrcdir)/config/config.mk
>+include $(topsrcdir)/ipc/chromium/chromium-config.mk

rules.mk includes config.mk, so not only is that second line unnecessary but it actually breaks the build subtly on certain platforms.  I landed http://hg.mozilla.org/mozilla-central/rev/e2d5c315e1cd to fix this and make double including config.mk fatal so this doesn't happen in the future.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I believe this patch is completely wrong.  It effectively disables the delayed prefetch queue, so gives a big hit on DOM manipulation performance.

The comment about "compared to the IPC" is just bogus; the right comparison is to not doing the prefetch at all because you get evicted from the queue.

Please file a followup bug to fix this the right way (i.e. without a big DOM performance hit).