141779 - Trunk, M1RC1 Crashes related to DNS caching [@ nsDNSRequest::FireStop]

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[greer]

Looks like something is messing up Gordon's code in 
/mozilla/source/netwerk/dns/src/nsDnsService.cpp

We have users on the Trunk, branch and in the RC1 builds who are crashing with 
differing scenarios. The most common stack and the comments that relate to 
that unique signature are below (I'll include others in an attachment below):

     Count   Platform List 
     17   Windows NT 5.0 build 2195
 
 Stack trace(Frame) 

         nsDNSRequest::FireStop [nsDnsService.cpp  line 399] 
         nsDNSLookup::ProcessRequests   [nsDnsService.cpp  line 790] 
         nsDNSService::ProcessLookup    [nsDnsService.cpp  line 1901] 
         nsDNSEventProc [nsDnsService.cpp  line 1865] 
         USER32.DLL + 0x1b60 (0x77e11b60)  
         USER32.DLL + 0x1cca (0x77e11cca)  
         USER32.DLL + 0x83f1 (0x77e183f1)   
 
     (5832043)  URL: http://mozspam.mozdev.org/index.html
     (5828992)  Comments: posting to Arstechnica.com
     (5816292)  URL: http://lists.debian.org
     (5816292)  Comments: Crashed while idle.  Maybe the download manager?
     (5809459)  URL: http://www.mp3speed.nu/
     (5784104)  Comments: Wasn't doing anything. It just crashed when I focused 
the window.
     (5769403)  URL: http://www.hotmail.com
     (5769403)  Comments: Opened hotmail website (after logging in it crashed)
     (5741797)  URL: www.metafilter.com
     (5741797)  Comments: I just clicked on a link.
     (5669036)  URL: http://www.hotmail.com
     (5669036)  Comments: Sending mail --> crash
     (5528348)  URL: www.sympatico.ca
     (5528348)  Comments: no reason 
     (5528309)  URL: www.sympatico.ca
     (5528309)  Comments: no reason

Comment 1

[greer]

Reassigning to darin instead of "[e-mail]". Darin, 
if that is the correct owner, please re-reassign. And sorry for the trouble.

Comment 2

[greer]

Attachment: Unique stacks under this signature paired with relevant user comments

Comment 3

[greer]

I haven't been able to reproduce this one but I can see from the talkback data 
that some NS folks have:
(5739290) Build: 2002042608 Plat: Windows NT  5.1 build 2600 Time: 2002-04-29 
[e-mail]         
(5756721) Build: 2002042608 Plat: Windows NT  5.1 build 2600 Time: 2002-04-29 
[e-mail]         
(5781570) Build: 2002042608 Plat: Windows NT  5.1 build 2600 Time: 2002-04-30 
[e-mail]         
(5832043) Build: 2002050108 Plat: Windows NT  5.0 build 2195 Time: 2002-05-01 
[e-mail]

cc'ing moied and mscott to see if they can reproduce this.

Comment 4

[Darin Fisher]

this crash could be explained if it ever happened that the nsDNSRequest were
only owned by the socket transport because nsSocketTransport::OnStopLookup
clears it's reference to mUserListener.

we'll have to go through and verify that someone besides mUserListener owns the
nsDNSRequest when FireStop is called.

-> moz 1.0.1

Comment 5

[Jay Patel [:jay]]

Here's another stack trace from a recent MozillaTrunk crash, almost identical to
the one in comment #0, but this one goes through nsDnsService.cpp  line 1946
(instead of line 1901):

Incident ID 6029895
Stack Signature nsDNSRequest::FireStop 8eafa60f
Email Address
Product ID MozillaTrunk
Build ID 2002050508
Trigger Time 2002-05-07 09:24:51
Platform Win32
Operating System Windows NT 5.0 build 2195
Module necko.dll
URL visited testing the list of websites from mozilla site
User Comments copy and paste from a site to compose email
Trigger Reason Access violation
Source File Name nsDnsService.cpp
Trigger Line No. 407
Stack Trace
nsDNSRequest::FireStop [nsDnsService.cpp, line 407]
nsDNSLookup::ProcessRequests [nsDnsService.cpp, line 799]
nsDNSService::ProcessLookup [nsDnsService.cpp, line 1946]
nsDNSEventProc [nsDnsService.cpp, line 1910]
USER32.DLL + 0x1b60 (0x77e11b60)
USER32.DLL + 0x1cca (0x77e11cca)
USER32.DLL + 0x83f1 (0x77e183f1) 

Comment 6

[Darin Fisher]

Attachment: v1 patch

i think i located the race condition that was causing this crash.  if DNS
request is canceled at just the right moment, then we could end up inside
FireStop from two different threads.  since there is no locking to protect each
nsDNSRequest, this could easily result in a crash inside FireStop.

this patch beefs up nsDNSRequest::Cancel to make it bail if it looks like
FireStop has already been called or is in the process of being called on
another thread.  i suspect this patch will fix the crash.

Comment 7

[rpotts (gone)]

Comment on attachment 84131 [details] [diff] [review]
v1 patch

r/sr=[e-mail]
looks good to me.

is there ever the case where mLookup is non-null, but mUserListener is null?

it looks like your new locking should prevent that situation...

the only reason i mention it is because the old code would unconditionally
clear mLookup...  while the new code will only clear mLookup when there is a
mUserListener...

Comment 8

[Darin Fisher]

rick, i think the unconditional clearing of mLookup is redundant.

Comment 9

[gordon]

Comment on attachment 84131 [details] [diff] [review]
v1 patch

r=gordon.

Comment 10

[Darin Fisher]

fixed-on-trunk

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 84131 [details] [diff] [review]
v1 patch

a=[e-mail], let's get this in today for 1.0rc3 (or bust!).  Thanks,

/be

Comment 12

[Darin Fisher]

fixed-on-branch

Comment 13

[benc]

greer: based on talkback, have we seen improvement since the fixes were checked in?

Comment 14

[greer]

benc: Thelast ten days of data don't have any incidents of this crash. That's an
improvement. :) . Marking VERIFIED.

Comment 15

[benc]

VERIFIED branch, per greer.