127918 - nsStandardUrl not thread-safe Assertion [@nsResProtocolHandler::ResolveURI]

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[timeless]

./run-mozilla.sh ./TestProtocols resource:///components/prettyStack.jsds

generates *lots* of asserts. I'm going to attach the script (not relevant), the console output, and one stack.

Comment 1

[timeless]

Attachment: prettyStack.jsds

Comment 2

[timeless]

Attachment: prettyStack.log

Comment 3

[timeless]

Attachment: resource protocol standardurl assert threadsafe stack

Comment 4

[timeless]

fwiw bug 127927 covers nsBasicEncoder and nsUnicodeEncodeHelper (a victim), and there are already bugs for pluginhost, registryfactory, and nativecomponentloader.

=> darin based on cvsblame for the consumer.

Comment 5

[Darin Fisher]

whoa!  this looks very nasty... the file transport thread is calling into the
mime service which is calling into the IO service.  the IO service is not
threadsafe!  this is a crash waiting to happen.

Comment 6

[Darin Fisher]

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

Comment 7

[Darin Fisher]

-> dougt

Comment 8

[Doug Turner (:dougt)]

Attachment: Patch v.1

Removes content type result from nsIStreamIO::Open().
Added atrribute content type nsIStreamIO.
Fixes callers.

I expect a minor performance improvement since this patch prevents every file
read via nsIFileIO to cause a mime lookup.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 73045 [details] [diff] [review]
Patch v.1

this patch needs some work still for the sync case.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 73045 [details] [diff] [review]
Patch v.1

jumped the gun a bit.

Comment 11

[rpotts (gone)]

Comment on attachment 73045 [details] [diff] [review]
Patch v.1

hey doug,
i'm still not clear why the 'contentType' attribute is not read-only.

in the 'current world' where the content-type is returned as an [out] parameter
from open(...) it is read-only.

further, it appears that only nsInputStreamIO and nsFileIO implement
SetContentType()...

in the case of nsInputStreamIO, it is already possible to initialize the
content-type using the init(...) method.  So, it seems like having
SetContentType() is unnecessary...

in the case of nsFileIO, there is no notion of using a content-type that was
set... what would it mean to set the content-type of an OutputStream for a
file?

Currently, the nsIOStream interface assumes that the content-type is
read-only... if the content-type needs to be set in a particular instance, it
must be done via another method (such as init() in the nsInputStreamIO case...)

Is this so bad?

-- rick

Comment 12

[timeless]

+     * Returns associated content type, if any.
+     **/
+    attribute string contentType; 

please remove 'Returns'.... I suppose you could replace it with stores but then 
you'ld have a bit of a expaining to do...

i know this is merely adjacent to the code you're changing...

 nsInputStreamIO::Close(nsresult status)
 {
     mStatus = status;
     if (mInputStream)
         return mInputStream->Close();
     else
         return NS_OK;

would you please remove the else [after return]?

Comment 13

[Doug Turner (:dougt)]

Attachment: Patch v.2

Makes contentType readonly.
fixes timeless's concerns.

Comment 14

[Darin Fisher]

so you decided against upgrading to nsACString for the contentType getter?

Comment 15

[Doug Turner (:dougt)]

Some classes dervive from both nsIStreamIO and nsIChannel.  I think that this
change should happen when nsIChannel is fixed.

Comment 16

[gordon]

Although I'd really like to clean up the nsIStreamIO interface, this patch at
least deals with the safety issues.  Go for it Doug!  r=gordon

Comment 17

[Darin Fisher]

Comment on attachment 73195 [details] [diff] [review]
Patch v.2

sr=darin

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 73195 [details] [diff] [review]
Patch v.2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 19

[Doug Turner (:dougt)]

Fixed:

Checking in dom/src/jsurl/nsJSProtocolHandler.cpp;
/cvsroot/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,v  <-- 
nsJSProtocolHandler.cpp
new revision: 1.78; previous revision: 1.77
done
Checking in netwerk/base/public/nsIStreamIO.idl;
/cvsroot/mozilla/netwerk/base/public/nsIStreamIO.idl,v  <--  nsIStreamIO.idl
new revision: 1.4; previous revision: 1.3
done
Checking in netwerk/base/src/nsFileStreams.cpp;
/cvsroot/mozilla/netwerk/base/src/nsFileStreams.cpp,v  <--  nsFileStreams.cpp
new revision: 1.41; previous revision: 1.40
done
Checking in netwerk/base/src/nsFileStreams.h;
/cvsroot/mozilla/netwerk/base/src/nsFileStreams.h,v  <--  nsFileStreams.h
new revision: 1.22; previous revision: 1.21
done
Checking in netwerk/base/src/nsFileTransport.cpp;
/cvsroot/mozilla/netwerk/base/src/nsFileTransport.cpp,v  <--  nsFileTransport.cpp
new revision: 1.121; previous revision: 1.120
done
Checking in netwerk/base/src/nsFileTransport.h;
/cvsroot/mozilla/netwerk/base/src/nsFileTransport.h,v  <--  nsFileTransport.h
new revision: 1.56; previous revision: 1.55
done
Checking in netwerk/base/src/nsInputStreamChannel.cpp;
/cvsroot/mozilla/netwerk/base/src/nsInputStreamChannel.cpp,v  <-- 
nsInputStreamChannel.cpp
new revision: 1.62; previous revision: 1.61
done
Checking in netwerk/base/src/nsInputStreamChannel.h;
/cvsroot/mozilla/netwerk/base/src/nsInputStreamChannel.h,v  <-- 
nsInputStreamChannel.h
new revision: 1.30; previous revision: 1.29
done
Checking in netwerk/protocol/jar/src/nsJARChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/jar/src/nsJARChannel.cpp,v  <--  nsJARChannel.cpp
new revision: 1.85; previous revision: 1.84
done
Checking in netwerk/protocol/jar/src/nsJARChannel.h;
/cvsroot/mozilla/netwerk/protocol/jar/src/nsJARChannel.h,v  <--  nsJARChannel.h
new revision: 1.41; previous revision: 1.40
done

Comment 20

[Aleksey Nogin]

There is an obvious typo in nsInputStreamChannel.cpp introduced by this bug. It
should be fixed by:

--- nsInputStreamChannel.cpp    2002/03/13 00:34:52     1.62
+++ nsInputStreamChannel.cpp    2002/03/13 16:21:08
@@ -431,7 +431,7 @@
             if (NS_FAILED(rv)) return rv;
             mOpened = PR_TRUE;
         }
-        mStreamIO->GetContentType(&mContentType);
+        rv = mStreamIO->GetContentType(&mContentType);
         if (NS_FAILED(rv)) return rv;
     }
     *aContentType = nsCRT::strdup(mContentType);

Bug noticed thanks to a compiler warning (`nsresult rv' might be used
uninitialized in this function).

Comment 21

[Doug Turner (:dougt)]

good find.  I will fix it when the tree opens.

Comment 22

[Doug Turner (:dougt)]

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

Comment 23

[Doug Turner (:dougt)]

Marking this bug fixed.  The concern that you mention will be fixed when 130524
lands.

Comment 24

[benc]

+verifyme

timeless: can you verify this?

Comment 25

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old