283487 - nsStringInputStream is still broken

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

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.

Comment 1

[timeless]

Attachment: fix the class

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

seems to me like nsIRandomAccessStore can be removed and the users be switched
to nsISeekableStream...

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 175458 [details] [diff] [review]
fix the class

defer to darin.

Comment 4

[Darin Fisher]

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.

Comment 5

[timeless]

i don't think that's for me to call.

Comment 6

[Darin Fisher]

dougt: want to kill nsIRandomAccessStore please? :)

Comment 7

[Doug Turner (:dougt)]

love to see it go.  reassign if you need me to actually work on it. ;-)

Comment 8

[Darin Fisher]

-> dougt

Comment 9

[Doug Turner (:dougt)]

Attachment: patch to remove nsIRandomAccessStore

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 10

[Darin Fisher]

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 11

[Doug Turner (:dougt)]

it was already defined.  scary huh:

http://lxr.mozilla.org/seamonkey/source/xpcom/obsolete/nsIFileStream.h#79

Comment 12

[Darin Fisher]

Comment on attachment 186495 [details] [diff] [review]
patch to remove nsIRandomAccessStore

wow @$%!

Comment 13

[Doug Turner (:dougt)]

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 14

[Scott MacGregor]

Comment on attachment 186495 [details] [diff] [review]
patch to remove nsIRandomAccessStore

looks good

Comment 15

[Doug Turner (:dougt)]

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 16

[Darin Fisher]

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 ;-)

Comment 17

[Andreas Wuest]

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

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

sounds like that should be filed as its own bug.

Comment 19

[Andreas Wuest]

(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.

Comment 20

[timeless]

Attachment: use LengthRemaining consistently

the only change I found left from the comment 0 that seemed relevant was this...

Comment 21

[Darin Fisher]

Comment on attachment 247626 [details] [diff] [review]
use LengthRemaining consistently

OK

Comment 22

[timeless]

Comment on attachment 247626 [details] [diff] [review]
use LengthRemaining consistently

mozilla/xpcom/io/nsStringStream.cpp 	1.25

Comment 23

[Ryan VanderMeulen [:RyanVM]]

timeless, is more work needed or is this bug FIXED?