nsBufferStream does not support offset writing.

RESOLVED WONTFIX

Status

()

Core
Networking
P3
normal
RESOLVED WONTFIX
18 years ago
2 years ago

People

(Reporter: Yixiong Zou, Unassigned)

Tracking

({helpwanted})

Trunk
Future
All
Other
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-])

Attachments

(6 attachments)

(Reporter)

Description

18 years ago
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.
yixiong.zou@intel.com - are you still seeing this problem with recent builds of 
Mozilla?

Gerv
Summary: nsBufferStream does not support offset writing. → nsBufferStream does not support offset writing.
yixiong.zou@intel.com, or anyone@intel.com - this bug needs some work (and an 
attached testcase or useful URL) or it's going to get closed :-(

Gerv
(Reporter)

Comment 3

18 years ago
sorry. missed the first email. I will send in a patch tomorrow. 
(Reporter)

Comment 4

18 years ago
Created attachment 9331 [details]
modified version of cache testcases.
(Reporter)

Comment 5

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

Comment 6

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

Comment 7

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

18 years ago
conmfirming and adding patch keyword
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch

Comment 9

18 years ago
neeti: can you review the attached patch please? thanks!
Assignee: gagan → neeti

Comment 10

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

Comment 11

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

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

Comment 13

18 years ago
Created attachment 10085 [details] [diff] [review]
patch bufferstream.diff
(Reporter)

Comment 14

18 years ago
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.

Updated

18 years ago
Target Milestone: --- → M17

Updated

18 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 15

18 years ago
Created attachment 10421 [details] [diff] [review]
a new patch that can run on all platforms.
(Reporter)

Comment 16

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

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

Comment 18

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

Comment 19

18 years ago
Created attachment 10586 [details] [diff] [review]
diff against latest source

Comment 20

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

Comment 21

18 years ago
Created attachment 10675 [details] [diff] [review]
made nsBufferedStream::Seek an abstract function
(Reporter)

Comment 22

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

18 years ago
Warren,

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

Neeti

Comment 24

18 years ago
Warren, Gagan,

Your call to make, whether this should be nsbeta3+

Neeti
Keywords: nsbeta3

Comment 25

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

Comment 26

18 years ago
Yes, I can land the changes.

Neeti

Comment 27

18 years ago
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. 
Keywords: helpwanted
Whiteboard: [nsbeta3-]

Updated

18 years ago
Target Milestone: M17 → Future

Comment 28

17 years ago
mass move, v2.
qa to me.
QA Contact: tever → benc

Comment 29

16 years ago
moving neeti's futured bugs for triaging.
Assignee: neeti → new-network-bugs
Status: ASSIGNED → NEW
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.)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.