32314 - nsBufferStream does not support offset writing.

Status: RESOLVED WONTFIX

Description

[Yixiong Zou]

if you run mozilla/netwerk/test/TestRawCache with option "-f" to test file 
cache. The last test TestOffsetWrite would fail because nsBufferedOutputStream 
file cache uses does not support offset writing. when nsBufferStream 
initializes, it always set mOffset to 0.

Comment 1

[Gervase Markham [:gerv] (not reading bugmail)]

[e-mail] - are you still seeing this problem with recent builds of 
Mozilla?

Gerv

Comment 2

[Gervase Markham [:gerv] (not reading bugmail)]

[e-mail], or [e-mail] - this bug needs some work (and an 
attached testcase or useful URL) or it's going to get closed :-(

Gerv

Comment 3

[Yixiong Zou]

sorry. missed the first email. I will send in a patch tomorrow. 

Comment 4

[Yixiong Zou]

Created attachment 9331 [details]
modified version of cache testcases.

Comment 5

[Yixiong Zou]

a Confirmation: I am still experience the same problem with the latest May 21st 
builds. 

I am submitting 2 modified files, they are 
1) mozilla/netwerk/test/TestRawCache.cpp 
2) mozilla/netwerk/test/TestCacheMgr.cpp

how to run them:
1. Modify the Makefile in mozilla/netwerk/test, compile these two test cases. 
2. Edit your default_prefs.js if you don't have one. Put the following lines in 
it:
user_pref("browser.cache.directory", "/what/ever/you/like/");
user_pref("browser.cache.enable", 1);
user_pref("browser.cache.disk.enable", 1);
3. Run "TestRawCache -f", the assertion would show up at the end of the test 
cases, corresponding to the failure of Offset Writting test. 

Comment 6

[Yixiong Zou]

Created attachment 9334 [details]
this is a patch that synced with 05/21's source tree that can has a fix to this bug

Comment 7

[Yixiong Zou]

I've already sended a patch for this bug. 

It includes a couple modifications. Including modified methods of 
nsDiskCacheRecordChannel::SetTransferOffset(), 
nsFileTransport::OpenOutputStream(),
nsBufferedOutputStream::Seek()

This seems to be able to solve the problem. Could someone review it for me so 
that I can check it in? 

Comment 8

[Asa Dotzler [:asa]]

conmfirming and adding patch keyword

Comment 9

[Gagan]

neeti: can you review the attached patch please? thanks!

Comment 10

[neeti]

I am not able to read the 2nd attachment(9334). Could you please send the 
patches as diffs instead of the whole file. 

Thanks,
Neeti

Comment 11

[Yixiong Zou]

Have you tried to untar the file? The extension got screwed up after you 
download it. I just tried it. Seems to be fine. If you still have troubles, let 
me know. And I will send in diffs instead. 

Thank you. 

Comment 12

[neeti]

I was able to untar the first attachment(9331). But I was not able to untar the 
second attachment(9334). Could you please send the diffs and also specify the 
extensions on your files.

Thanks,
Neeti

Comment 13

[Yixiong Zou]

Created attachment 10085 [details] [diff] [review]
patch bufferstream.diff

Comment 14

[Yixiong Zou]

I just created another attachment. It's a diff against all the changes I've
made. To apply the patch, under mozilla/netwerk, do "patch bufferstream.diff"
and you have to type in file names for patch to work. But you can find out the
filename as part of the warning message. 

Don't know how to automate the process, Sorry.

Comment 15

[Yixiong Zou]

Created attachment 10421 [details] [diff] [review]
a new patch that can run on all platforms.

Comment 16

[Yixiong Zou]

neeti, I just created new testcases that uses profile instead of
default_prefs.js. sorry that we missed M16 target. Can you verify this early
please so we don't miss out on M17? 

Thank you. 

Comment 17

[Warren Harris]

Neeti: I just looked at the last diff, but there are a few things that don't 
look right:

- swapping arguments on AsyncRead looks wrong
- you should use nsnull instead of 0 when the value is a pointer, not an integer
- switching from nsMemory to nsAllocator is wrong -- perhaps something is not 
getting diff'd properly here (?)
- why are we using the profile manager in this test (StartupWithArgs)?
- don't use true/false for PRBool values -- use PR_TRUE/PR_FALSE
- several occurrences of commented out code -- should this be in or out? If out, 
then let's delete it.
- where did the nsBufferedOutputStream::Seek code come from? It looks copied 
from somewhere else.

Comment 18

[Yixiong Zou]

- swapping arguments on AsyncRead looks wrong 
- you should use nsnull instead of 0 when the value is a pointer, not an integer

  I just swapped them so that they would compile. Will correct this. 

- switching from nsMemory to nsAllocator is wrong -- perhaps something is not 
getting diff'd properly here (?)

  I will double check. 

- why are we using the profile manager in this test (StartupWithArgs)?
  
  Basically, Filecache needs to know where Cache directory is. We used to use 
default_prefs.js. But that file doesn't exists on Windows. That's why I changed 
to use Profile manager to do the path discovery. 

- don't use true/false for PRBool values -- use PR_TRUE/PR_FALSE

  I will correct them. 

- several occurrences of commented out code -- should this be in or out? If 
out, then let's delete it.

  Yes, they should be deleted. 

- where did the nsBufferedOutputStream::Seek code come from? It looks copied 
from somewhere else.

  Yes, the nsBufferedOutputStream::Seek was copied from nsBufferedStream::Seek. 
I just made a little modification. 

Comment 19

[Yixiong Zou]

Created attachment 10586 [details] [diff] [review]
diff against latest source

Comment 20

[Warren Harris]

Is there any way to make nsBufferedStream::Seek work for both cases? If not, 
then I'd prefer to move nsBufferedStream::Seek's body down to 
nsBufferedInputStream::Seek, and make nsBufferedStream::Seek pure virtual. 
Otherwise, this looks good now.

Fixing this also fixes bug 37714.

Comment 21

[Yixiong Zou]

Created attachment 10675 [details] [diff] [review]
made nsBufferedStream::Seek an abstract function

Comment 22

[Yixiong Zou]

Warren, I've submitted a new diff. I tried to combine two Seek() into one
nsBufferedStream::Seek(). But turned out to be difficult, especially for a file
that's larger than the buffersize 64k. So I made them into sperate classes and
made nsBufferedStream::Seek() abstract. 

Please let me know if there are problems. 

Comment 23

[neeti]

Warren,

Yixiong has created a patch with the changes you wanted. Are they ok?

Neeti

Comment 24

[neeti]

Warren, Gagan,

Your call to make, whether this should be nsbeta3+

Neeti

Comment 25

[Warren Harris]

I think the patch looks good (although slightly out of date). Neeti, can you 
land it? 

Comment 26

[neeti]

Yes, I can land the changes.

Neeti

Comment 27

[Gagan]

neeti: since the patch is attached here, as long as you have reviewed it, we
could let someone else check it in. You have way too many things on your plate
already. I am going to minus it, but add helpwanted. 

Comment 28

[benc]

mass move, v2.
qa to me.

Comment 29

[Doug Turner (:dougt)]

moving neeti's futured bugs for triaging.

Comment 30

[Patrick McManus [:mcmanus]]

This isn't on anyone's work list and realistically is an abandoned idea. I will close as wontfix - if someone has a patch or is actively going to work on it please reopen. (but please, only then.)