Closed Bug 115891 Opened 23 years ago Closed 23 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

x86
Windows NT
defect
Not set
critical

Tracking

(Not tracked)

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) (deleted) — 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) (deleted) — Splinter Review
this one againt 2002-03-21 cvs tree
Attached patch patch v1.3 improved to fix bug 120411 (obsolete) (deleted) — Splinter Review
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.
Attached patch patch v1.4 addressed darin's comments (obsolete) (deleted) — Splinter Review
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
Attached patch patch to BackwardAdapter.cpp (deleted) — Splinter Review
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 dll release build (deleted) —
Attached file tester ini file (deleted) —
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: 23 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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: