Closed
Bug 32314
Opened 24 years ago
Closed 9 years ago
nsBufferStream does not support offset writing.
Categories
(Core :: Networking, defect, P3)
Tracking
()
RESOLVED
WONTFIX
Future
People
(Reporter: yixiong.zou, Unassigned)
Details
(Keywords: helpwanted, Whiteboard: [nsbeta3-])
Attachments
(6 files)
11.07 KB,
application/octet-stream
|
Details | |
190.00 KB,
application/octet-stream
|
Details | |
12.96 KB,
patch
|
Details | Diff | Splinter Review | |
14.27 KB,
patch
|
Details | Diff | Splinter Review | |
10.68 KB,
patch
|
Details | Diff | Splinter Review | |
15.02 KB,
patch
|
Details | Diff | Splinter Review |
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•24 years ago
|
||
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.
Comment 2•24 years ago
|
||
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•24 years ago
|
||
sorry. missed the first email. I will send in a patch tomorrow.
Reporter | ||
Comment 4•24 years ago
|
||
Reporter | ||
Comment 5•24 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•24 years ago
|
||
Reporter | ||
Comment 7•24 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•24 years ago
|
||
conmfirming and adding patch keyword
neeti: can you review the attached patch please? thanks!
Assignee: gagan → neeti
Comment 10•24 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•24 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•24 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•24 years ago
|
||
Reporter | ||
Comment 14•24 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.
Reporter | ||
Comment 15•24 years ago
|
||
Reporter | ||
Comment 16•24 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•24 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•24 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•24 years ago
|
||
Comment 20•24 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•24 years ago
|
||
Reporter | ||
Comment 22•24 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•24 years ago
|
||
Warren, Yixiong has created a patch with the changes you wanted. Are they ok? Neeti
Comment 24•24 years ago
|
||
Warren, Gagan, Your call to make, whether this should be nsbeta3+ Neeti
Keywords: nsbeta3
Comment 25•24 years ago
|
||
I think the patch looks good (although slightly out of date). Neeti, can you land it?
Comment 26•24 years ago
|
||
Yes, I can land the changes. Neeti
Comment 27•24 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•24 years ago
|
Target Milestone: M17 → Future
Comment 29•22 years ago
|
||
moving neeti's futured bugs for triaging.
Assignee: neeti → new-network-bugs
Status: ASSIGNED → NEW
Comment 30•9 years ago
|
||
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
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•