59215 - fix pipe refcounting bug

Status: VERIFIED INVALID

(Core :: Networking, defect, P3)

Description

[John Morrison]

Bug 47263 was hijacked to do a bulletproofing fix for RTM (NS6). 
This bug is to put this bug back on track, in that there is still
a problem with this routine being entered twice. 

Relevant comments from the original bug report:



Opened: 2000-08-01 20:02 with comments From [e-mail] 

Some intermittent problem was introduced with the file transport and how it 
refcounts pipes. This shows up in bug 46267 and bug 45358. Danm checked in a 
work-around, but I'm afraid the real problem lurks elsewhere. I need to 
investigate.


------- Additional Comments From [e-mail] 2000-08-14 16:32 -------

  I've run into this problem again. It's difficult to reproduce under a 
debugger 
-- it happens only intermittently and dallying at breakpoints seems to make it 
happen less often, though not to kill it completely.
  So I was encouraged to put printfs into the END_READ case of 
nsFileTransport::Process(), like this:

--- ./netwerk/base/src/nsFileTransport.cpp.1.84 Mon Aug 14 16:10:38 2000
+++ ./netwerk/base/src/nsFileTransport.cpp      Mon Aug 14 16:02:10 2000
@@ -481,7 +481,15 @@
       }
 
       case END_READ: {
-        
+
+        printf("END_READ nsFileTransport %8lX own thread %8lX curr thread 
%8lX\n",
+          (long)this,(long)_mOwningThread,(long)NS_CurrentThread());
+        if (!mBufferOutputStream) {
+          mXferState = CLOSING;
+          printf("***************** bad END_READ ****************\n");
+          break;
+        }
+
         PR_AtomicDecrement(&mService->mInUseTransports);
 
         PR_LOG(gFileTransportLog, PR_LOG_DEBUG,

I get output like this, occasionally including the *** bad END_READ *** part:

Error loading URL file:///d|/download/800Kfile.exe
WEBSHELL+ = 5
END_READ nsFileTransport  38847A0 own thread   944CC0 curr thread  2499A18
END_READ nsFileTransport  335F028 own thread   944CC0 curr thread  2499A18
END_READ nsFileTransport  38C7B20 own thread   944CC0 curr thread  2499A18

result native path = D:\download\test

WEBSHELL+ = 6
************************************************************
** NOTE: This report will only be printed in DEBUG builds.**
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[Exception... "Illegal value"  code: "-2147024809" nsresult: 
"0x8
0070057 (NS_ERROR_ILLEGAL_VALUE)"  location: 
"chrome://global/content/downloadPr
ogress.js Line: 70"] [nsIObserver::Observe]"  nsresult: "0x8057001c 
(NS_ERROR_XP
C_JS_THREW_JS_OBJECT)"  location: "<unknown>"  data: no]
************************************************************
D:\builds\moz\mozilla\xpfe\components\xfer\src\nsStreamXferOp.cpp 389: Observe 
f
ailed, rv=0x8057001C
END_READ nsFileTransport  32A74C8 own thread   944CC0 curr thread   DE3FF8
WEBSHELL- = 5
END_READ nsFileTransport  38D27A8 own thread   944CC0 curr thread   D0FF08
WEBSHELL- = 4
END_READ nsFileTransport  338A028 own thread   944CC0 curr thread   D0FF08
END_READ nsFileTransport  338A028 own thread   944CC0 curr thread  2499A18
***************** bad END_READ ****************
END_READ nsFileTransport  338A028 own thread   944CC0 curr thread   DE3FF8
END_READ nsFileTransport  338A028 own thread   944CC0 curr thread  248EC18

  I'd swear I did the owning thread / current thread comparison correctly. If 
so, there are a couple of things to notice here. First, the current executing 
thread seems *never* to be the object's owning thread. Second, in this example, 
the same nsFileTransport object is sent through the END_READ case twice, once 
each from different threads.
  This causes at least an assertion on debug builds because 
mBufferOutputStream->Flush() is called when mBufferOutputStream is null. It was 
the crux of the problem with bug 46267.

  I don't know where to go from here. It seems that a bunch of threads jump in 
and cause the END_READ case at the end of a file transport. Is this expected? 
It 
seems wrong. My ignorance of the code is making me not want to try to figure 
out 
where the threads are coming from (though a breakpoint in the thread 
constructor 
tells me no new threads were created during the file download).
  Does this ring any bells?
  Another perhaps related thing: if you launch Mozilla and get a directory 
listing by typing into the url bar something like file:///d|/builds, all looks 
normal. If however you launch Mozilla with that -url, all directories in the 
d|/builds directory are listed twice. I mention this here because it seems like 
there may be competing read threads executing. Maybe it's unrelated.


[SNIP]

------- Additional Comments From [e-mail] 2000-10-12 03:44 -------

This is interesting -- since line 517 is the only place we set this to null, it 
would imply that we're coming through END_READ twice. That could screw up the 
mInUseTransports count, among other things. 

I don't think a kungFuDeathGrip will help the situation here since it's an 
instance variable, not something on the stack, but I do think being defensive 
about calling Flush is a good thing. 

Cc'ing jband who might take an interest in taking a look at this hard to spot 
problem.


[SNIP]



The bulletproofing fix was checked in to the branch and trunk.

------- Additional Comments From [e-mail] 2000-10-31 16:59 -------

Checked into trunk. (Note that this is a work-around.)


---

Comment 1

[benc]

mass move, v2.
qa to me.

Comment 2

[peter swain]

this seems to be the source of cpu-bound behavior on Linux,
stracing mozilla reveals continual poll(), returning exactly one ripe fd,
being one end of a pipe. The other end is also open by mozilla, and no other
procs have it open. Probably a pipe set up before fork;exec, but never torn down
when some pre-fork check aborts the spawning.

I've noticed this behavior since netscape 4.5 (earlier??),
but haven't yet explored it in mozilla source

Comment 3

[benc]

can we target this bug?

Comment 4

[John Morrison]

-> gagan for consideration, although perhaps this bug is no longer relevant 
(e.g., perhaps the code has changed in this area).

Comment 5

[Darin Fisher]

this bug is very likely dead since so much has changed to the file transport since
this bug was reported.

danm: what do you think?

Comment 6

[Gagan]

marking as invalid since all this code has changed.

Comment 7

[Bradley Baetz (:bbaetz)]

VERIFIED.