Closed Bug 219376 Opened 17 years ago Closed 17 years ago

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)

Categories

(Core :: Networking, defect, P1)

x86
Windows 95
defect

Tracking

()

RESOLVED FIXED
mozilla1.6alpha

People

(Reporter: opi, Assigned: darin.moz)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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.
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 ?
Keywords: regression
Attached file 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.
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.)
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.6alpha
this bug does not happen under Win2k.
Nor does it happen under XP.
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 :(
Verify bug on winME
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?
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.
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)?
*** Bug 219449 has been marked as a duplicate of this bug. ***
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.
Attached patch v1 patch (obsolete) — Splinter Review
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.
Attachment #131919 - Flags: review?(dougt)
Attachment #131919 - Flags: superreview?(bryner)
Blocks: 219150
*** Bug 220498 has been marked as a duplicate of this bug. ***
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;
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?
Attached patch v1.1 patchSplinter Review
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.
Attachment #131919 - Attachment is obsolete: true
Attachment #131919 - Flags: superreview?(bryner)
Attachment #131919 - Flags: review?(dougt)
Comment on attachment 132718 [details] [diff] [review]
v1.1 patch

dougt: can you please review this patch.  thx!
Attachment #132718 - Flags: review?(dougt)
Attachment #132718 - Flags: superreview?(bryner)
*** Bug 219657 has been marked as a duplicate of this bug. ***
*** Bug 219150 has been marked as a duplicate of this bug. ***
--------------------------------------------------
+        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);
--------------------------------------------------
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
No longer blocks: 219150
Summary: Trying to resolve IP's via DNS → Trying to resolve IP's via DNS (Failed connections stall instead of giving Connection Failure Error; pages stop/don't finish/complete loading if ad hosts/scripts fail)
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?
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.
>mDone may be set to PR_TRUE inside GetNextAddr,

that's what I meant by "cleared somewhere" - thx, that answers my question.
Attachment #132718 - Flags: superreview?(bryner) → superreview+
Attachment #132718 - Flags: review?(dougt) → review+
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
*** Bug 220409 has been marked as a duplicate of this bug. ***
Working for me! 2003100711 on XP. Woho!
Ok, works on win95 (Should I mark bug as verified?)
Depends on: 223145
No longer depends on: 223145
*** Bug 226328 has been marked as a duplicate of this bug. ***
Alexander: please assign qa to yourself and verify.

Darin: this fix was purely Win 95/98/ME, right?
Summary: Trying to resolve IP's via DNS (Failed connections stall instead of giving Connection Failure Error; pages stop/don't finish/complete loading if ad hosts/scripts fail) → 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)
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.
*** Bug 219150 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.