Closed Bug 283487 Opened 16 years ago Closed 13 years ago

nsStringInputStream is still broken

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

Details

(Keywords: crash)

Attachments

(3 obsolete files)

problems (possibly incomplete list):
mEOF is useless and not always properly maintained.
LengthRemaining() isn't used consistently.
nsStringInputStream::Seek() uses LengthRemaining() instead of mLength for 
filesize XXX this is the cause of our crashes.
nsStringInputStream::Seek() can only manipulate 31bit streams, although the 
class itself tries to manage 32bits.
nsIRandomAccessStore is a random class instead of being a real interface.

notes:
by getting rid of mEOF, the comparison of newPosition with fileSize doesn't 
need to handle the case when newPosition equals fileSize because getAtEOF 
already deals with it.

crashes are encountered when you use seek and then pass data from the 
inputstream through xpconnect. we're using 18a5 with the previous patch to 
stringinputstream.
Attached patch fix the class (obsolete) — Splinter Review
Attachment #175458 - Flags: review?(dougt)
seems to me like nsIRandomAccessStore can be removed and the users be switched
to nsISeekableStream...
Comment on attachment 175458 [details] [diff] [review]
fix the class

defer to darin.
Attachment #175458 - Flags: review?(dougt) → review?(darin)
Comment on attachment 175458 [details] [diff] [review]
fix the class

Yeah, can we just remove nsIRandomAccessStore once and for all?  The 3 users in
mailnews don't seem to care about AtEOF.  The only code that does seem to care
is some of the obsolete nsFileStreams code.  But, that code could probably be
fixed to record AtEOF as a member variable since it only uses AtEOF internally.
i don't think that's for me to call.
dougt: want to kill nsIRandomAccessStore please? :)
love to see it go.  reassign if you need me to actually work on it. ;-)
-> dougt
Assignee: timeless → dougt
I am not sure that this fixes the bug, but it does remove this interface from
the tree (leaving it in the obsolete lib).
Comment on attachment 186495 [details] [diff] [review]
patch to remove nsIRandomAccessStore

I see nsIRandomAccessStore being removed, but I don't see it being added back
to the obsolete directory someplace.  Is part of the patch missing?
Comment on attachment 186495 [details] [diff] [review]
patch to remove nsIRandomAccessStore

wow @$%!
Attachment #186495 - Flags: review+
Comment on attachment 186495 [details] [diff] [review]
patch to remove nsIRandomAccessStore

mscott, could you give this a once over.  Basically change two or three call
sites in mailnews.  You shouldn't feel anything.
Attachment #186495 - Flags: superreview?(mscott)
Attachment #186495 - Flags: approval1.8b3?
Comment on attachment 186495 [details] [diff] [review]
patch to remove nsIRandomAccessStore

looks good
Attachment #186495 - Flags: superreview?(mscott) → superreview+
Attachment #186495 - Flags: approval1.8b3? → approval1.8b3+
Checking in mailnews/base/util/nsMsgDBFolder.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.253; previous revision: 1.252
done
Checking in mailnews/imap/src/nsImapOfflineSync.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapOfflineSync.cpp,v  <-- 
nsImapOfflineSync.cpp
new revision: 1.54; previous revision: 1.53
done
Checking in xpcom/io/nsIStringStream.idl;
/cvsroot/mozilla/xpcom/io/nsIStringStream.idl,v  <--  nsIStringStream.idl
new revision: 1.6; previous revision: 1.5
done
Checking in xpcom/io/nsStringStream.cpp;
/cvsroot/mozilla/xpcom/io/nsStringStream.cpp,v  <--  nsStringStream.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in xpcom/io/nsStringStream.h;
/cvsroot/mozilla/xpcom/io/nsStringStream.h,v  <--  nsStringStream.h
new revision: 1.4; previous revision: 1.3
done


I have removed the nsIRandomAccessStore interface.  

Timeless, do you have time to update your patch?  if not, please let me know
Comment on attachment 175458 [details] [diff] [review]
fix the class

This patch is now obsolete I presume.  We don't want to expose
nsIRandomAccessStore as a legit interface ;-)
Attachment #175458 - Attachment is obsolete: true
Attachment #175458 - Flags: review?(darin) → review-
Does this also fix the case where data is null and dataLen is -1? Because then we crash in strlen(), as for instance in:

(gdb) bt
#0  0x900030e8 in strlen ()
#1  0x1b508330 in nsStringInputStream::SetData (this=0x204465e0, data=0x0, dataLen=-1) at /Users/awuest-devel/Documents/devel/src/mozilla/trees/MOZILLA_1_8_0_BRANCH/mozilla/xpcom/io/nsStringStream.cpp:144
sounds like that should be filed as its own bug.
(In reply to comment #18)
> sounds like that should be filed as its own bug.

I filed it as https://bugzilla.mozilla.org/show_bug.cgi?id=351418.
Assignee: dougt → nobody
QA Contact: xpcom
Attached patch use LengthRemaining consistently (obsolete) — Splinter Review
the only change I found left from the comment 0 that seemed relevant was this...
Assignee: nobody → timeless
Attachment #186495 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #247626 - Flags: review?(darin.moz)
Comment on attachment 247626 [details] [diff] [review]
use LengthRemaining consistently

OK
Attachment #247626 - Flags: review?(darin.moz) → review+
Comment on attachment 247626 [details] [diff] [review]
use LengthRemaining consistently

mozilla/xpcom/io/nsStringStream.cpp 	1.25
Attachment #247626 - Attachment is obsolete: true
timeless, is more work needed or is this bug FIXED?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.