Closed
Bug 32314
Opened 25 years ago
Closed 10 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•25 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•25 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•25 years ago
|
||
sorry. missed the first email. I will send in a patch tomorrow.
Reporter | ||
Comment 4•25 years ago
|
||
Reporter | ||
Comment 5•25 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•25 years ago
|
||
Reporter | ||
Comment 7•25 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•25 years ago
|
||
conmfirming and adding patch keyword
neeti: can you review the attached patch please? thanks!
Assignee: gagan → neeti
Comment 10•25 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•25 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•25 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•25 years ago
|
||
Reporter | ||
Comment 14•25 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•25 years ago
|
||
Reporter | ||
Comment 16•25 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•25 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•25 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•25 years ago
|
||
Comment 20•25 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•25 years ago
|
||
Reporter | ||
Comment 22•25 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•25 years ago
|
||
Warren,
Yixiong has created a patch with the changes you wanted. Are they ok?
Neeti
Comment 24•25 years ago
|
||
Warren, Gagan,
Your call to make, whether this should be nsbeta3+
Neeti
Keywords: nsbeta3
Comment 25•25 years ago
|
||
I think the patch looks good (although slightly out of date). Neeti, can you
land it?
Comment 26•25 years ago
|
||
Yes, I can land the changes.
Neeti
Comment 27•25 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•25 years ago
|
Target Milestone: M17 → Future
Comment 29•23 years ago
|
||
moving neeti's futured bugs for triaging.
Assignee: neeti → new-network-bugs
Status: ASSIGNED → NEW
Comment 30•10 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: 10 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•