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

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Ari Berger]

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.

Comment 1

[Doug Turner (:dougt)]

oh god.  when did this break?

Comment 2

[Peter Lubczynski]

I think this may be a new issue. Shrirang, can you test byte ranges in trunk and
branch builds?

Comment 3

[Peter Lubczynski]

What I mean was test "Acrobat" byte range support....

Comment 4

[shrirang khanzode]

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. 

Comment 5

[Michael Dunn]

shrirang can you test adobe through compuserve. Get with Brent or go to our lab.

Comment 6

[shrirang khanzode]

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!

Comment 7

[Peter Lubczynski]

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.

Comment 8

[Peter Lubczynski]

--->av

"not a show stopper"

Comment 9

[Arun Ranganathan]

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.

Comment 10

[Ari Berger]

CC'ing Martin- he will give additional information and test case.

Comment 11

[Martin Gentry]

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.

Comment 12

[shrirang khanzode]

file not found error :
http://cole.viewpoint.com/~mgentry/mozilla/byterange.html

Comment 13

[Martin Gentry]

My apologies, the correct URL is:
http://cole.viewpoint.com/~mgentry/mozilla/byterange/byterange.html

Comment 14

[av (gone)]

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.

Comment 15

[av (gone)]

Attachment: Log of NPN_*/NPP_* calls when loading the test page

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?

Comment 16

[av (gone)]

Some empty lines in the previous log have been inserted by the developer studio, 
don't pay attention to them.

Comment 17

[Ari Berger]

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.

Comment 18

[Martin Gentry]

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?

Comment 19

[av (gone)]

At some point I see -1 returned from NPP_Write, is it done on purpose?

Still cannot reproduce. Trying.

Comment 20

[Ari Berger]

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.

Comment 21

[av (gone)]

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.

Comment 22

[Ari Berger]

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.

Comment 23

[av (gone)]

OK, now I can at least reproduce it now. Going into.

Comment 24

[Ari Berger]

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.

Comment 25

[Ari Berger]

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?

Comment 26

[Judson Valeski]

so, is the dummy range a reasonable workaround?

Comment 27

[Ari Berger]

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.

Comment 28

[Doug Turner (:dougt)]

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. 

Comment 29

[av (gone)]

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?

Comment 30

[Peter Lubczynski]

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?

Comment 31

[Martin Gentry]

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.

Comment 32

[Peter Lubczynski]

nominating nsbeta1, mozilla1.0, 4xp

Comment 33

[Arun Ranganathan]

Marking nsbeta1+ per ADT triage.

Comment 34

[av (gone)]

-->serge per plugin stuff meeting

Comment 35

[Michael Dunn]

plusing for topembed

Comment 36

[serge (gone)]

Attachment: patch v1

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 37

[av (gone)]

Comment on attachment 74482 [details] [diff] [review]
patch v1

Looks like the idl file is not used. Should not we get rid of it?

Comment 38

[Peter Lubczynski]

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?

Comment 39

[av (gone)]

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.

Comment 40

[serge (gone)]

>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.

Comment 41

[serge (gone)]

Attachment: patch v1.1 addresses src buffer alignment problem.

av, peterl what's your vote on nsPluginStreamInfo.idl?

Comment 42

[av (gone)]

We have the whole bunch of ghost idls. Let's remove them.

Comment 43

[Peter Lubczynski]

look good to me, r=peterl

Ari, does this patch fix Viewpont single byte range requests?

Comment 44

[serge (gone)]

Attachment: patch with revise logic to allocate mStreamBuffer in ns4xPluginStreamListener::OnDataAvailable() only

also it gets rid of nsIPluginStreamListener2 interface bug 83186

Comment 45

[serge (gone)]

Peter, could you try it on Mac please?

Comment 46

[Peter Lubczynski]

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?

Comment 47

[serge (gone)]

hmm, on w2k I did cat patch1 patch2 > patch3, it always works on unix:(

Comment 48

[serge (gone)]

Attachment: patch v1.2

this one againt 2002-03-21 cvs tree

Comment 49

[serge (gone)]

Attachment: patch v1.3 improved to fix bug 120411

also fixes
bug 83186 nsIPluginStreamListener2 should be removed
bug 120411 [viewpoint] byte range requests do not work on https
please review

Comment 50

[Winnie Lam]

adding ADT2 in status whiteboard as per discussion with beppe.

Comment 51

[Peter Lubczynski]

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

Comment 52

[serge (gone)]

Thanks Peter.
Who can sr= for this?
Doug, would you?

Comment 53

[Darin Fisher]

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.

Comment 54

[Peter Lubczynski]

> 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.

Comment 55

[serge (gone)]

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.

Comment 56

[serge (gone)]

Attachment: patch v1.4 addressed darin's comments

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?

Comment 57

[Peter Lubczynski]

Comment on attachment 76157 [details] [diff] [review]
patch v1.4 addressed darin's comments

r=peterl

Comment 58

[serge (gone)]

Attachment: patch v1.5 w/o changes for ns4xPlugin.cpp which not suppose to be there

this patch seems to fix bug 122783 also

Comment 59

[serge (gone)]

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

Comment 60

[serge (gone)]

cc darin for possible sr=

Comment 61

[Darin Fisher]

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?

Comment 62

[serge (gone)]

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 63

[Asa Dotzler [:asa]]

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

Comment 64

[serge (gone)]

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

Comment 65

[serge (gone)]

Attachment: patch to BackwardAdapter.cpp

Comment 66

[Peter Lubczynski]

*** Bug 83186 has been marked as a duplicate of this bug. ***

Comment 67

[rubydoo123]

Is this fixed? Shouldn't this be marked as fixed?

Comment 68

[serge (gone)]

I'm going to mark it fixed after testing the release build

Comment 69

[serge (gone)]

Attachment: tester dll release build

Comment 70

[serge (gone)]

Attachment: tester ini file

Comment 71

[serge (gone)]

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

Comment 72

[serge (gone)]

resoved as fixed

Comment 73

[shrirang khanzode]

i see exactly the same results as serge mentioned above on 0327 trunk on 
windows.Verif.