219376 - DNS: attempting IP address resolution (Failed connections stall instead of giving Connection Failure Error; pages stop/don't finish/complete loading if ad hosts/scripts fail)

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Alexander Opitz]

If I enter an URL as IP mozilla seems to try to resolve it as name with the DNS.
Also websites with links/forwards to an IP have this problem
(http://www.funcomeunity.de) but then you never get an error message, that
mozilla can't find the host.
Since 2003091510 but I haven't tried mozilla since 2 weeks on the win95 system.
Problem seems only with win95 cause no confirmation from #mozilla.de users.

Comment 1

[Olivier Cahagne]

could be a regression specific to Win95 from bug 205726.
Can you attach a HTTP log (minimized to the failing query) via "create a new
attachment" ? See instructions here:
http://www.mozilla.org/projects/netlib/http/http-debugging.html
Do you have a proxy configured: auto (pac) / manual ?

Comment 2

[Alexander Opitz]

Attachment: logfile

There is no proxy used. Used: 2003091604, have started mozilla with blank page,
typed 217.160.130.21 in the url bar and closed mozilla after ok on error
message.

Comment 3

[Darin Fisher]

under Win95, we just call gethostbyname.  perhaps it does not support numeric
addresses.  indeed, MSDN has this to say about gethostbyname:

  The gethostbyname function cannot resolve IP address strings passed to it. Such 
  a request is treated exactly as if an unknown host name were passed. Use 
  inet_addr to convert an IP address string the string to an actual IP address, 
  then use another function, gethostbyaddr, to obtain the contents of the hostent 
  structure.

oops!  patch coming up...  (this likely effects all WIN32 builds.)

Comment 4

[Darin Fisher]

this bug does not happen under Win2k.

Comment 5

[Jason Bassford]

Nor does it happen under XP.

Comment 6

[Darin Fisher]

ok, so it seems like this bug only effects Win95 and probably derivatives like
Win98, etc.  i suspect most other platforms support numeric ip addresses with
gethostbyname.  perhaps we should add code to NSPR to handle this problem.

wtc: what do you think?  in the past, the necko DNS code handled this problem;
however, there were things that probably never worked before (e.g., the old
SOCKS code used to call PR_GetHostByName directly to resolve the SOCKS proxy
host).  implementing this in NSPR is going to be a little bit painful since
we'll need to populate a PRHostEnt with the result of PR_StringToNetAddr :(

Comment 7

[Alexander Opitz]

Verify bug on winME

Comment 8

[Wan-Teh Chang]

Darin, needless to say, I'd like you to restore the PR_StringToNetAddr
workaround in the necko DNS code.

You wrote:
> implementing this in NSPR is going to be a little bit painful since
> we'll need to populate a PRHostEnt with the result of PR_StringToNetAddr :(

By PRHostEnt, you really meant PRAddrInfo, right?

Comment 9

[Darin Fisher]

wtc: no, i meant PRHostEnt, implying that the change would effect
PR_GetHostByName, which PR_GetAddrInfoByName calls under Win9x :)

ok, i will write up a patch for necko.  i just thought it would be good to fix
this in NSPR instead so that PR_GetHostByName and friends behave similarly
("port-ably") on all platforms.

Comment 10

[Wan-Teh Chang]

Darin, could you outline how you'd implement this in
PR_GetHostByName and friends?  Would you modify
PR_GetAddrInfoByName, too, and why (or why not)?

Comment 11

[Darin Fisher]

*** Bug 219449 has been marked as a duplicate of this bug. ***

Comment 12

[Darin Fisher]

wtc:

for windows only, i would call PR_StringToNetAddr up at the top of
PR_GetHostByName (and probably also PR_GetIPNodeByName), and then i would copy
the result into the PRHostEnt buffer and return.  no change would be necessary
to PR_GetAddrInfoByName since it either uses PR_GetHostByName or getaddrinfo.

however, that said, i do have a patch which modifies necko to always call
PR_StringToNetAddr before calling PR_GetAddrInfoByName.  it required me to make
some hefty changes to the necko host resolver code in order to support two kinds
of resulting data structures (PRAddrInfo and PRNetAddr) accessible via the
nsIDNSRecord interface.

Comment 13

[Darin Fisher]

Attachment: v1 patch

ok, this is the patch i mentioned that solves the bug by making necko call
PR_StringToNetAddr before PR_GetAddrInfoByName.  unfortunately, the changes are
non-trivial, but this did give me the opportunity to eliminate nsAddrInfo in
favor of just exposing nsHostRecord directly to the nsDNSService
implementation.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 220498 has been marked as a duplicate of this bug. ***

Comment 15

[Doug Turner (:dougt)]

The patch didn't apply cleanly for me.  

+    PRBool HasResult() const { return (addrinfo || addr) != nsnull; }

Instead of two compares, you could OR these values and do one compare against zero;

Comment 16

[Darin Fisher]

yeah, i'm not surprised the patch has bit-rotted.  i'll post a new patch.


>+    PRBool HasResult() const { return (addrinfo || addr) != nsnull; }
>
>Instead of two compares, you could OR these values and do one compare against 
>zero;

isn't that what i am doing?

Comment 17

[Darin Fisher]

Attachment: v1.1 patch

revised patch; up-to-date.. should apply cleanly.

note: it appears that a temporary fix to this bug actually got mistakenly
checked into the tree.	it was a solution that i rejected in favor of this
patch, but i must have left it in my tree and checked it in along with other
changes to nsSocketTransport2.cpp.  i think that explains the merge conflict. 
at any rate, this patch actually backs out that fix and replaces it with this
fix.  the "bad" fix was only a few lines in nsSocketTransport2.cpp.

Comment 18

[Darin Fisher]

Comment on attachment 132718 [details] [diff] [review]
v1.1 patch

dougt: can you please review this patch.  thx!

Comment 19

[David :Bienvenu]

*** Bug 219657 has been marked as a duplicate of this bug. ***

Comment 20

[David :Bienvenu]

*** Bug 219150 has been marked as a duplicate of this bug. ***

Comment 21

[Jerry Baker]

--------------------------------------------------
+        if (addr->raw.family == PR_AF_INET)
+            addr->inet.port = htons(port);
+        else
+            addr->ipv6.port = htons(port);
--------------------------------------------------
Just a note: This wouldn't compile in VC++ 6 SP5.

d:/Mozilla/mozilla/netwerk/dns/src/nsDNSService2.cpp(115) : error C2065: 'htons'
 : undeclared identifier

But, changing it to allowed it to build and test fine:
--------------------------------------------------
+        if (addr->raw.family == PR_AF_INET)
+            addr->inet.port = PR_htons(port);
+        else
+            addr->ipv6.port = PR_htons(port);
--------------------------------------------------

Comment 22

[David :Bienvenu]

do you need the null check here? most of the delete/free things check for null
themselves.

+    if (addr)
+        free(addr);

other than that and the htons thing we mentioned earlier, this looks good to me
- r=bienvenu

Comment 23

[David :Bienvenu]

the other question I had has to do with this code:

 NS_IMETHODIMP
 nsDNSRecord::HasMore(PRBool *result)
 {
-    *result = !mDone;
+    if (mDone)
+        *result = PR_FALSE;
+    else {
+        // unfortunately, NSPR does not provide a way for us to determine if
+        // there is another address other than to simply get the next address.
+        void *iterCopy = mIter;
+        PRNetAddr addr;
+        *result = NS_SUCCEEDED(GetNextAddr(0, &addr));
+        mIter = iterCopy; // backup iterator
+        mDone = PR_FALSE;
+    }

mDone is already false when we get here - can it be cleared somehow?

Comment 24

[Darin Fisher]

bienvenu: thx for the review :)

i've heard it said that on some platforms free(0) crashes.  it seems bogus
because i think ANSI-C allows free(0), but alecf warned me that it can happen.


>mDone is already false when we get here - can it be cleared somehow?

i don't understand the question.  mDone may be set to PR_TRUE inside
GetNextAddr, so we need to reset it to the value it held when we entered this
block.  same goes for rolling back mIter.

Comment 25

[David :Bienvenu]

>mDone may be set to PR_TRUE inside GetNextAddr,

that's what I meant by "cleared somewhere" - thx, that answers my question.

Comment 26

[Darin Fisher]

fixed-on-trunk

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 220409 has been marked as a duplicate of this bug. ***

Comment 28

[Steve Wardell]

Working for me! 2003100711 on XP. Woho!

Comment 29

[Alexander Opitz]

Ok, works on win95 (Should I mark bug as verified?)

Comment 30

[John Holcomb]

*** Bug 226328 has been marked as a duplicate of this bug. ***

Comment 31

[benc]

Alexander: please assign qa to yourself and verify.

Darin: this fix was purely Win 95/98/ME, right?

Comment 32

[Darin Fisher]

benc: yes, the original problem was Win9x specific, but the solution (the patch)
affected all platforms.  it is possible that other platforms might behave like
Win9x, but that theory is untested.

Comment 33

[Darin Fisher]

*** Bug 219150 has been marked as a duplicate of this bug. ***