Closed Bug 283487 Opened 16 years ago Closed 13 years ago
String Input Stream is still broken
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.
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. ;-)
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?
it was already defined. scary huh: http://lxr.mozilla.org/seamonkey/source/xpcom/obsolete/nsIFileStream.h#79
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.
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 ;-)
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
the only change I found left from the comment 0 that seemed relevant was this...
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.