Closed Bug 115891 Opened 19 years ago Closed 18 years ago

[viewpoint] [FIX] Byte range request are not giving the correct data

Categories

(Core :: Plug-ins, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: aberger, Assigned: srgchrpv)

References

()

Details

(Keywords: topembed+, Whiteboard: [ADT2])

Attachments

(5 files, 6 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    

Our plugin requests a file that we already have partailly downloaded.  Once we 
get the first data on the file and check that the modification date matches the 
date that we have in our own cache, we call NPN_RequestRead. The byte range 
that you add to the http header looks correct, but you give us the wrong data:
For example, let's say we have a 100000 byte file and we alrady have the first 
20000.  We request:
Byte=20000, length=80000
we expect bytes 20000-100000 to the end.
Instead, we get 0-80000.
So we get the right number of bytes, but the wrong ones.
I know that we are getting the wrong bytes because:
1) Write is called with an offset of 0
2) I look at the buffer and it is the begining of the file

The second (related, I assume?) problem is that we don't get a destroystream 
message when the data is done.
However, we do get the destroystream at the end- when we leave the page.
I get a destroystream with a reason of USER_BREAK, and the url of the stream 
has been set to ",".  The rest of the data is still ok- eg lastmodified.


Reproducible: Always
Steps to Reproduce:
1.Install our plugin (see http://cole.viewpoint.com/~aberger/readme.txt
2.View http://cole.viewpoint.com/~aberger/interrupt/index.html
3.File references an mtx file that is large (over 4MB) (has a large comment 
block...)
4. Before content displays, hit back
5. Hit forward.  We will request the url 
(http://cole.viewpoint.com/~aberger/interrupt/test.mtx)
and once we verify that the date matches the date of the partial file we have 
in our OWN cache we request only the end of the file.
6. we get the wrong data and don't display the content


getting this far requires the patch for 115119- otherwise we don't get the 
user_break message the first time.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
oh god.  when did this break?
I think this may be a new issue. Shrirang, can you test byte ranges in trunk and
branch builds?
What I mean was test "Acrobat" byte range support....
I did a lot of tests to check this one out on 1217 trunk winNT::

Testcases from bugs:
85529
84332
53365

Adobe's Testsuites:
http://acroeng.adobe.com/BrowserTestSuite/
http://access.adobe.com/browser/netscape/net6.pdf

Byte serving is working fine, I see no errors, pages load just fine. I had 
"background loading' turned OFF. I see no errors, no crash. 
Keywords: edt0.9.4
shrirang can you test adobe through compuserve. Get with Brent or go to our lab.
I have pointed out the tests I used above. They are fairly easy, I currently do 
not have cycles to go thru all of the testcases on 'Compuserve'. Brent can 
contact me if he has any questions on the tests. Thanks!
guys,
byte ranges won't work for "our embedding vendor" because of networking issues
but there may be ways around it, e-mail me privately. I'm have a patch that may
do the trick but I need a build to try it out. But that's another issue that
should be moved to Bugscape if it's not there already.
Summary: Byte range request are not giving the correct data → [viewpoint] Byte range request are not giving the correct data
--->av

"not a show stopper"
Assignee: peterl → av
Status: ASSIGNED → NEW
Keywords: edt0.9.4edt0.9.4-
We "minused" this because it was considered not a "show stopper."  But after
talking to Ari, we're going to have to revisit that.  I'll remove the "-" so
that we can stand for renomination.
Keywords: edt0.9.4-edt0.9.4
CC'ing Martin- he will give additional information and test case.
Some more info on the behaviour, to repeat the steps below you'll need our 
plugin version 3.0.10.11 or later which Ari should be making available later 
today. (See http://cole.viewpoint.com/~aberger/readme.txt to find it).

The behaviour can be repeated as follows:
1) With the version of the plugin mentioned installed, view 
http://cole.viewpoint.com/~mgentry/mozilla/byterange.html and everything should 
be seen fine.  Move off the page so the plugin will release any cached files it 
has a hold of.

2) In the VMP resource cache, find the .mtx file for the scene just viewed (it 
will contain many comments which just pad the file (probably found at 
C:\Program Files\Viewpoint\Viewpoint Media Player\Resources\ResourceFolder_02\-
398721625.mtx).

3) Edit the file in a text editor to remove most of the end of the file (I left 
everything below the first three comments with resulting file size of 688).

4) View the same page again and our NPP_Write gets some odd data sent in.

We get a stream to download a file which is partially contained in our cache 
and after calling NPN_RequestRead with the range for the rest of the file 
specified the sequense of offsets and lengths we receive subsequently is as 
follows:
0, 16384
15696, 688
16384, 16384
32768, 16384
...
98304, 16384
114688, 8988

The file referenced on cole.viewpoint.com is 107980 bytes, however we are 
handed more than that.  Actually how much more we are handed than we should be 
is related to how much of the file we already had it seems in that in my 
example we received 16384-688=15696 too many bytes.

Also, as is obvious from the first few ranges we are handed, we are given an 
overlapped range, which is the lesser of the two problems.

Also, looking at the HTTP request that Mozilla is sending and the response from 
Apache SEEMS to look reasonable (I'm by no means an authority on well-formed 
HTTP requests and responses).  Not that comparing to IE is necessarily useful, 
but the one main difference as compared to IE is the lack of an If-Range header 
in the GET from Mozilla.
Whiteboard: [NEED ETA]
I cannot reproduce this using the above steps. The cube draws fine. In fact, I 
don't see requestread called at all. Although, I see ranges passed to NPP_Write.
If you look at the last NPP_Write you'll see that it all ends up with the
correct number of bytes -- 107980. Any ideas?
Some empty lines in the previous log have been inserted by the developer studio, 
don't pay attention to them.
Probably my fault.  I had made some installer changes.
Please use the NEWEST installer- (3.0.10.12)- it is now on my page 
(http://cole.viewpoint.com/~aberger/VMPFullInstall.exe).  Note that we now 
install into a different folder: c:\Program Files\Viewpoint\Viewpoint 
Experience Technolgy- that is where you will now find the Resources folder 
Martin mentioned.
If you still can't repeat, let us know.
Some more info on our attempt to seek...

After requesting the file we are going to want to seek on (because we already 
have some in our cache), we receive a NPP_NewStream call and set stype to 
NP_SEEK.  From my understanding of the docs for NPP_NewStream, we should not 
then start getting NPP_Write calls until we have called NPN_RequestRead with 
the range we wish to receive.  However, very soon we do receive a blob of bytes 
which are the beginning of the file we requested (from byte 0-n, instead of 
starting at the range we requested).

As our code sits now, when we start receiving the data we didn't ask for, we 
ignore the data, call NPN_RequestRead with what we DO want, and return that we 
have consumed all the data given to us.  The next blob of bytes we receive does 
in fact seem to be the range we asked for, however the offset we are given in 
the call to NPP_Write is as though Mozilla assumes it's the continuation of the 
first chunk given to us.

Am I misunderstanding the use of seekable streams or is Mozilla's behaviour 
wrong?
Whiteboard: [NEED ETA] → [NO ETA]
At some point I see -1 returned from NPP_Write, is it done on purpose?

Still cannot reproduce. Trying.
As far as I can tell, the only reason that we would return -1 from write is if 
we decide that we have the whole file in our own cache and we don't want the 
stream after all.

To clarify, here is the sequence that we see (numbers are just a typical 
example):
NPP_NewStream (we set the stream to seek)
NPP_WriteReady
NPP_Write(bytes 0 - 16K)
    We call NPN_RequestRead(range: 40K until end of file)
    return 16K
NPP_WriteReady
NPP_Write(bytes 16K- 32K): CORRECT DATA, but the offset passed in to Write is 
wrong!!!  It is passing us the data at 40K, like we asked, but the offset 
parameter thinks that it is just continuing the file. 

If we can trust that you are going to keep doing this incorrectly, we can 
probably work around it- doing some math, assuming that the offset is wrong.  
But that will cause problems if this ever gets fixed.

av- let's get in touch tomorrow and Martin or I can walk you through repeating 
the problem.
We definitely should. I am still struggling in attempt to see what you see. I 
see several NPP_NewStream calls but neither sets stream type seekable.
I can't reproduce the bug the way Martin said.  The problem is that once you 
edit the file, the date no longer matches the file on the server, and we won't 
requestread- we will just redownload the whole file.
Instead, here is how I reproduce it:
Delete your Resources directory in your Prgram Files\Viewpoint\Viewpoint 
Experience Technology folder.
Go to the page Martin mentions.  IMMEDIATELY before the content loads, leave 
the page.  (You can make sure you got it in time by checking the mtx file in 
the cache- \Resources\ResourceFolder_02\-398721625.mtx- the file is about 107K 
big.  I will have Martin make it bigger.)

We will try to resume the download.
av- let me know if this helps.

OK, now I can at least reproduce it now. Going into.
av- we have made changes in our plugin to try to do this as correctly as 
possible.  I will send you an update next time we do our build with exactly 
what we are doing.
I debugged this a bit with Peter.  Here is the problem: If we only pass in ONE 
byte range (as we do), Mozilla doesn't process it properly.  If we pass in MORE 
THAN ONE byte range, it does.  So In RequestRead, I now pass in the real range 
and a fake range (something after the end of the file).  It works.

Peter found one problem in the code:
In nsPluginStreamInfo::RequestRead(nsByteRange* rangeList)
 if (numRequests > 1) {
Shouldn't this be >= 1 ?!? What is wrong with passing in just one range.

I tried changing it to >=1, and then we never get any writes at all.  Is there 
another place where it is repeating this >1 check?
so, is the dummy range a reasonable workaround?
I'm willing to use this workaround.  I am getting a build to our QA today.  If 
it doesn't cause any problems, we're good to go.
the check for (numRequests > 1) is correct.  There is/was no need to go through
the multipart stream converter for single range responses.  These responses
could be directly forwarded to the end consumer.  There were alot of changes to
the converter, so things may have changed a bit. 

I think I saw another problem which may or may not be important. If on 
NPP_NewStream call plugin says it wants it seekable, we still send something to 
the stream. Should we do that? My understanding was we should wait for 
RequestRead. Ari, if this is the case, what do you actually do with this 
premature data?
The problem in the plugin code is that the QI on the other end doesn't detect a
nsIByteRequest and goes down the wrong code path:
http://lxr.mozilla.org/mozilla/source/modules/plugin/base/src/nsPluginHostImpl.cpp#2076

I thought Acrobat is expecting the rest of the data from a NewStream() for doing
background downloading?
Keywords: edt0.9.4edt0.9.4-
We agree that the call to NPP_Write before we've called NPN_RequestRead is 
wrong (at least as far as the API docs go).  However we are currently relying 
on this behaviour and use the opportunity to make our call to NPN_RequestRead.

We ignore the premature data but return it's length from NPP_Write to indicate 
that we consumed it so that we won't be handed the same data immediately.
Blocks: 120411
Keywords: topembed
nominating nsbeta1, mozilla1.0, 4xp
Keywords: 4xp, mozilla1.0, nsbeta1
Marking nsbeta1+ per ADT triage.
Keywords: nsbeta1nsbeta1+
Target Milestone: --- → mozilla1.0
-->serge per plugin stuff meeting
Assignee: av → serge
plusing for topembed
Keywords: topembedtopembed+
Attached patch patch v1 (obsolete) — Splinter Review
This patch does handle single range request as well as does not brake existing
multiple range behavior, I've heavily tested it against acrobat over http://,
also it gets rid of nsIPluginStreamListener2 interface bug 83186,
The one thing I'm not certain is two new methods in nsIPluginStreamInfo
interface, but it's not frozen yet
Comment on attachment 74482 [details] [diff] [review]
patch v1

Looks like the idl file is not used. Should not we get rid of it?
Really nice patch! But I have some questions:

1. Should nsIPluginStreamInfo.idl be changed rather than the header file? Do we
need both?

2. It doesn't look like the nsIPluginStreamInfo interface has changed since it's
landing and the comments on the IDL file make me think this interface is
depricated/frozen because of being part of the old "new API". Maybe it should
have some better comments? Is it okay add to the end of such an interface or
better to have a new one? Do Real and the MRJ plugin still work?

3. Is it safe to use ptrStreamBuffer instead of mStreamBuffer? Does Shockwave
registration and update (bug 85334) still work?

4. How does zeroBytesWriteCount work? 

5. Maybe NS_ASSERTION(numtowrite,"WriteReady returned Zero") can be a
NS_WARN_IF_FALSE?
nsPluginStreamInfo.idl is a relic, it is not in the build process. I do not know
why. We need to remove it or use it. I guess.
>Do we need both?
 I vote for removing nsPluginStreamInfo.idl
> Do Real and the MRJ plugin still work?
I've tried real player it works, I did not  try MRJ though.
>Is it safe to use ptrStreamBuffer instead of mStreamBuffer?
my original concern was about possible alignment problem because NPP_Write 
expects  void *buf, but it we have exactly the same case in 
void *memmove( void *dest, const void *src, size_t count );
*src is not void aligned, of course memmove implementation can care about 
alignment...
what I'm trying to say is so far I have not seen any problem on 32 bits 
machines, I'm going to test in on 64 bits asap.
>Does Shockwave registration and update (bug 85334) still work.
I'll test it, but why do you think this patch could break it?
>How does zeroBytesWriteCount work
probably I wasn't clear enough in the comments:
here is the extraction from the code:
      // it is possible plugin's NPP_Write() returns 0 byte consumed
      // we use zeroBytesWriteCount to count situation like this
      // and break the loop
      PRInt32 zeroBytesWriteCount = 0;
      PRInt32 amountRead = 12345;
      do {
        PRInt32   numtowrite;
        PRInt32   writeCount = 0; // bytes consumed by plugin instance
        writeCount = NPP_Write(....);
        // if NPP_Write() returns writeCount == 0 lets say 3 times in a raw
        // consider this as end of loop without error.
        if (writeCount == 0) {
          if (++zeroBytesWriteCount == 3) {
            rv = NS_OK;
            break;
          }
        } else if (writeCount < 0) {
          rv = NS_ERROR_FAILURE;
          break;
        } else {
         amountRead -= writeCount;
          zeroBytesWriteCount = 0;
          ptrStreamBuffer += writeCount; 
        }
      } while (amountRead > 0);
What is the way to break this do{} loop when amountRead > 0 and by unknown 
reason plugin returns 0 from NPP_Write()?
I use zeroBytesWriteCount to do this job.

>Maybe NS_ASSERTION(numtowrite,"WriteReady returned Zero")...
maybe, but this is old code, and I think that part of code should be touched to 
fix bug 83186.
av, peterl what's your vote on nsPluginStreamInfo.idl?
We have the whole bunch of ghost idls. Let's remove them.
look good to me, r=peterl

Ari, does this patch fix Viewpont single byte range requests?
Whiteboard: [NO ETA]
Attachment #74482 - Attachment is obsolete: true
Attachment #74873 - Attachment is obsolete: true
also it gets rid of nsIPluginStreamListener2 interface bug 83186
Peter, could you try it on Mac please?
Comment on attachment 75297 [details] [diff] [review]
patch with revise logic to allocate mStreamBuffer in ns4xPluginStreamListener::OnDataAvailable() only

Having some problems applying:

patch: **** malformed patch at line 66: Index: nsPluginHostImpl.cpp

Can you diff from modules/plugin/base?
Attachment #75297 - Attachment is obsolete: true
hmm, on w2k I did cat patch1 patch2 > patch3, it always works on unix:(
Attached patch patch v1.2 (obsolete) — Splinter Review
this one againt 2002-03-21 cvs tree
also fixes
bug 83186 nsIPluginStreamListener2 should be removed
bug 120411 [viewpoint] byte range requests do not work on https
please review
Attachment #75430 - Attachment is obsolete: true
adding ADT2 in status whiteboard as per discussion with beppe.
Whiteboard: [ADT2]
Comment on attachment 75657 [details] [diff] [review]
patch v1.3 improved to fix bug 120411

wow Serge, what a great patch! works good on Mac too, r=peterl
Attachment #75657 - Flags: review+
Thanks Peter.
Who can sr= for this?
Doug, would you?
Comment on attachment 75657 [details] [diff] [review]
patch v1.3 improved to fix bug 120411

>Index: public/MANIFEST

> nsIPluginStreamListener.h
>-nsIPluginStreamListener2.h

nsIPluginStreamListener does not appear to be frozen, so why
nsIPluginStreamListener2?


>Index: src/ns4xPluginInstance.cpp

> ///////////////////////////////////////////////////////////////////////////////
> NS_IMETHODIMP
> ns4xPluginStreamListener::OnDataAvailable(nsIPluginStreamInfo* pluginInfo,
>                                           nsIInputStream* input,
>                                           PRUint32 length)
> {
>+  nsresult rv = NS_OK;
>   if (!mInst || !mInst->IsStarted())
>-    return NS_ERROR_FAILURE;
>+    return rv;


it is usually bad to return NS_OK and not read |length| bytes of data from
the stream.  perhaps you meant to return a failure code, which would cancel
the stream?

>+  // check out if plugin implements NPP_Write call
>+  if(!callbacks || !callbacks->write || !length)
>+    return rv; // it'll cancel necko transaction 

likewise.


>+    // for https:// byte range request (bug #120411) 
>+    // or in case when there is no disk cache available
>+    // do not check for return code from input->Read(),
>+    // because nsInputStreamTee::TeeSegment() will fail to write into file and return an error,
>+    // but we actually got the data from the net if amountRead !=0
>+    input->Read(mStreamBuffer, bytesToRead, (PRUint32*)&amountRead);
>+    if (amountRead == 0) // there is nothing left in the input stream
>+      break; // it's almost impossible to get here

whoa!!	seems to me that we need to fixup nsInputStreamTee or use something
different here.  it's not a good idea to ignore exceptions... you cannot trust
the result of an out param if the method throws an exception.
Attachment #75657 - Flags: needs-work+
> nsIPluginStreamListener does not appear to be frozen, so why
> nsIPluginStreamListener2?

nsIPluginSreamListener is implicitly frozen because it is depricated and in use
by major plugins like Real and Java. This patch removes the second interface
which was added months ago because the signature of depricated interfaced changed.

Thank you Darin for quick review.
>+  nsresult rv = NS_OK;
definitely is wrong initialization and will it be changed

>seems to me that we need to fixup nsInputStreamTee
to me it looks like it's doing the right thing
>or use something different here.
I'll think about it.
Please review this patch.
The problem with byte range request over https:// appears when	
nsInputStreamTee returns NS_BASE_STREAM_CLOSED because we closed output local
plugin cache file in nsPluginStreamListenerPeer::OnStopRequest() and for that
closed fd in ODA we were doing tee of data
    mPluginStreamInfo->GetLocalCachedFileStream(getter_AddRefs(outStream));
    if (outStream) {
	rv = NS_NewInputStreamTee(getter_AddRefs(stream), aIStream, outStream);

---
calling mPluginStreamInfo->SetLocalCachedFileStream(0) after closing this
stream in OnStopRequest() breaks of the reference to it and we'll not call tee,
and everything seams works fine.
I'm not caching byte range into to the local file, I see no needs to do so.
Any thought?
Attachment #75657 - Attachment is obsolete: true
Comment on attachment 76157 [details] [diff] [review]
patch v1.4 addressed darin's comments

r=peterl
Attachment #76157 - Flags: review+
this patch seems to fix bug 122783 also
Attachment #76157 - Attachment is obsolete: true
Comment on attachment 76267 [details] [diff] [review]
patch v1.5 w/o changes for ns4xPlugin.cpp which not suppose to be there

I carry over prefious r=peterl
Attachment #76267 - Flags: review+
cc darin for possible sr=
Comment on attachment 76267 [details] [diff] [review]
patch v1.5 w/o changes for ns4xPlugin.cpp which not suppose to be there

ok, sr=darin

but if you are not caching the results of byte range requests (and i know for a
fact that HTTP doesn't cache byte range requests) who does?  and does it
matter?  will acroread performance suffer?
Attachment #76267 - Flags: superreview+
thanks darin,
>who does?
no code does
>and does it matter? 
for plugins it doesn't matter.
>will acroread performance suffer?
it's difficult to say, because if we do not have byte_range_request_cache_impl
we cannot estimate how it will perform, we always go
total_time = net_time + multi_mixed_converter_time.
so for slow connection it looks not good:(
Comment on attachment 76267 [details] [diff] [review]
patch v1.5 w/o changes for ns4xPlugin.cpp which not suppose to be there

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76267 - Flags: approval+
Checked in
/cvsroot/mozilla/modules/plugin/base/public/MANIFEST
new revision: 1.19; previous revision: 1.18
/cvsroot/mozilla/modules/plugin/base/public/Makefile.in
new revision: 1.26; previous revision: 1.25
/cvsroot/mozilla/modules/plugin/base/public/makefile.win
new revision: 1.25; previous revision: 1.24
/cvsroot/mozilla/modules/plugin/base/public/nsIPluginStreamInfo.h
new revision: 1.8; previous revision: 1.7
/cvsroot/mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp
new revision: 1.88; previous revision: 1.87
/cvsroot/mozilla/modules/plugin/base/src/ns4xPluginStreamListener.h
new revision: 1.10; previous revision: 1.9
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp
new revision: 1.372; previous revision: 1.371
Blocks: 55959
*** Bug 83186 has been marked as a duplicate of this bug. ***
Is this fixed? Shouldn't this be marked as fixed?
I'm going to mark it fixed after testing the release build
Attached file tester ini file
test case for win32 bits only.
drop tester plugin npapi.dll attachment 76499 [details] & npapi.ini attachment 76500 [details] into 
plugin folder
start mozilla and access
http://lxr.mozilla.org/seamonkey/source/modules/plugin/tools/tester/testcase/tes
tapi.html
click on "Clear" button 
choose NPN_RequestRead from API call list box
set url, range and go.
e.g. for http://mozilla.org and range:100-100 I got in log frame:
NPP_Write(0x02ec9f64, 0x02b8f47c, 100, 100, 0x030a3008("text/css"]
[LINK REL="icon" HREF...")))
Returning: 100
Time: 710852 710852 (0)
--
where second param is offset & third one is len
resoved as fixed
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Summary: [viewpoint] Byte range request are not giving the correct data → [viewpoint] [FIX] Byte range request are not giving the correct data
i see exactly the same results as serge mentioned above on 0327 trunk on 
windows.Verif.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.