IPC DNS prefetching shouldn't bother sending zero-length hostnames

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
I end up seeing lots of these errors when browsing around in fennec:

WARNING: NS_ENSURE_TRUE(host && *host) failed: file /home/t_mattjo/src/firefox/mobilebase/netwerk/dns/nsHostResolver.cpp, line 501

They come from attempting to prefetch zero-length hostnames, such as javascript: links.  We know they're going to fail, so we might as well not bother sending zer-length hostnames over the wire.
(Assignee)

Comment 1

7 years ago
Created attachment 463746 [details] [diff] [review]
Patch
Assignee: nobody → josh
Attachment #463746 - Flags: review?(doug.turner)

Comment 2

7 years ago
Comment on attachment 463746 [details] [diff] [review]
Patch

How about using net_IsValidHostName?

Updated

7 years ago
Attachment #463746 - Flags: review?(doug.turner) → review-
Created attachment 464725 [details] [diff] [review]
Don't attempt to prefetch invalid hostnames from content processes

net_IsValidHostName() considers the empty string to be a valid hostname, so we still need to filter that.  However, net_IsValidHostName() is a good additional check to make.
Attachment #463746 - Attachment is obsolete: true
Attachment #464725 - Flags: review?(doug.turner)

Comment 4

7 years ago
Comment on attachment 464725 [details] [diff] [review]
Don't attempt to prefetch invalid hostnames from content processes

I think we are all unhappy with the string conversions, and we are also unhappy with net_IsValidHostName(null|"") returning true.  But this patch improves what we have.

ooc, why the #ifdefs around:

+using namespace mozilla::net;

Does it hurt anything to not wrap it?
Attachment #464725 - Flags: review?(doug.turner) → review+
(In reply to comment #4)
> Comment on attachment 464725 [details] [diff] [review]
> Don't attempt to prefetch invalid hostnames from content processes
>
> ooc, why the #ifdefs around:
> 
> +using namespace mozilla::net;
> 
> Does it hurt anything to not wrap it?

I thought mozilla::net was all conditionally-declared necko IPC stuff (and if so, |using mozilla::net| would be a compile error), but I see that we forward-declare some necko things in mozilla::net outside of |ifdef MOZ_IPC|.  Good call, will fix.
Comment on attachment 464725 [details] [diff] [review]
Don't attempt to prefetch invalid hostnames from content processes

This is a safe patch that can eliminate a lot of wasted IPC traffic: ~70 messages while loading news.google.com, e.g.  (We might want to do something smarter for valid hostnames later, too, but that's a bug for another day.)
Attachment #464725 - Flags: approval2.0?

Updated

7 years ago
blocking2.0: --- → betaN+

Updated

7 years ago
Attachment #464725 - Flags: approval2.0?
http://hg.mozilla.org/projects/cedar/rev/70094380fcbb
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.