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

RESOLVED FIXED in mozilla1.0

Status

()

P1
critical
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: timeless, Assigned: dougt)

Tracking

({crash})

Trunk
mozilla1.0
x86
FreeBSD
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

17 years ago
./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.
(Reporter)

Comment 1

17 years ago
Created attachment 71553 [details]
prettyStack.jsds
(Reporter)

Comment 2

17 years ago
Created attachment 71554 [details]
prettyStack.log
(Reporter)

Comment 3

17 years ago
Created attachment 71556 [details]
resource protocol standardurl assert threadsafe stack
(Reporter)

Comment 4

17 years ago
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.
Assignee: new-network-bugs → darin

Comment 5

17 years ago
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.
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.0

Updated

17 years ago
Keywords: crash, mozilla1.0

Updated

17 years ago
Keywords: nsbeta1

Updated

17 years ago
Severity: major → critical
Priority: P2 → P1

Comment 6

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

Updated

17 years ago
Keywords: nsbeta1+

Updated

17 years ago
Keywords: nsbeta1

Comment 7

17 years ago
-> dougt
Assignee: darin → dougt
Status: ASSIGNED → NEW
(Assignee)

Comment 8

17 years ago
Created attachment 73045 [details] [diff] [review]
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.
(Assignee)

Comment 9

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

this patch needs some work still for the sync case.
Attachment #73045 - Flags: needs-work+
(Assignee)

Comment 10

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

jumped the gun a bit.
Attachment #73045 - Flags: needs-work+

Comment 11

17 years ago
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
(Reporter)

Comment 12

17 years ago
+     * 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]?
(Assignee)

Comment 13

17 years ago
Created attachment 73195 [details] [diff] [review]
Patch v.2

Makes contentType readonly.
fixes timeless's concerns.
Attachment #73045 - Attachment is obsolete: true

Comment 14

17 years ago
so you decided against upgrading to nsACString for the contentType getter?
(Assignee)

Comment 15

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

Comment 16

17 years ago
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

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

sr=darin
Attachment #73195 - Flags: superreview+
Attachment #73195 - Flags: review+

Comment 18

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73195 - Flags: approval+
(Assignee)

Comment 19

17 years ago
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
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 20

17 years ago
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

17 years ago
good find.  I will fix it when the tree opens.
(Assignee)

Comment 22

17 years ago
*** Bug 130683 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

17 years ago
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

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

Comment 24

17 years ago
+verifyme

timeless: can you verify this?
Keywords: verifyme
Crash Signature: [@nsResProtocolHandler::ResolveURI]
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.