425284 - Adobe PDF: Multiple pending NPN_RequestRead() interfere with each other

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[Rudi Sherry]

Adobe PDFs use the NSAPI to request bytes from the server (when Accept: Bytes is passed back from the GET request.

Debugging has shown that in 2.0.0.13 (and I think 3.0bx), when NPN_RequestRead() is called, nsPluginStreamInfo::setStreamOffset() is called with the offset for that byte request... but if an already-issued request subsequently comes in, it uses that new streamOffset rather than the already-issued request offset.  That is:

NPN_RequestRead( offset 100, len 10 )
....setStreamOffset(100)
NPN_RequestRead( offset 200, len 10 )
...setStreamOffset(200)
OnDataAvailable() for the 100/10 request
...getStreamOffset() returns 200, NPP_Write() called with offset 200 but with the bytes for offset 100.

I'm trying to debug to understand how this might be fixed, but would appreciate some (a lot of) help.

I think this may be the cause of a number of existing bugs:
410235
417352 (maybe)

...breaking Adobe PDF is (IMHO) critical (but I'm biased).

Comment 1

[Rudi Sherry]

This only happens if multiple single-range requests are made.  If multiple-range requests are made, it works.

Comment 2

[Rudi Sherry]

On a related note, for some reason too many ranges in a single request is now not handled correctly.  We used to allow a maximum of 100 ranges, but we are seeing problems with 2.0.0.13 and we had to reduce that to 5... but that has to wait until another Acrobat/Reader update, and I don't know when that will go in.  Any help figuring out why the number of ranges had to be so drastically reduced would be welcom.

Comment 3

[Rolf Bjarne Kvinge]

I can reproduce this too (though it is some sort of race condition, since it works correctly about ~10% of the time).

I had a look at the source code, and I think Rudi's diagnosis is correct.

Comment 4

[Rolf Bjarne Kvinge]

So, I mocked up a test case that just sends a lot of seek requests and asserts that the data gotten back is correct. It fails immediately (<1s) here.

I tried fixing it myself (my idea was to use the nsPluginByteRangeStreamListener instance as the context parameter when opening the http channel, and put the current write position in an instance field on nsPluginByteRangeStreamListener), but I ran into compilation problems I wasn't able to figure out (error: ‘nsISupports’ is an ambiguous base of ‘nsPluginStreamListenerPeer’).

Comment 5

[Rolf Bjarne Kvinge]

Attachment: Test plugin

This test case was built on linux (OpenSuse), there's a Makefile attached that can be used to build/run it.

You'll need a web server that supports byte range requests running on localhost (I used apache) - it's a lot harder to reproduce the issue with file:// urls (probably because things happen a lot faster).

Comment 6

[Rudi Sherry]

IIRC the code checks to see how many byte-ranges there were and did something different if there was only one, and that was the problem.  My gut feeling is to remove that test and treat a single range the same way as multiple ranges are treated.  It may not be quite as efficient but it simplifies the code and that's always good.

Comment 7

[Rolf Bjarne Kvinge]

Yeah, that's one problem, another is that it's possible to have several seek requests running at the same time, and they could still step on each other toes (unlikely, given that the information is stored in a hash table keyed on the starting position of the range request, but still possible).

Comment 8

[Rudi Sherry]

Ah, I didn't know that.  This may be significant because we have a workaround (not yet released) for bug 528807; every range request we send, we will also send [1..2] as a range to make sure that the ranges are disjoint (our algorithm never requests anything in the first K otherwise, we always use the original response for that much).

This means we will often have a number of live range requests starting at 1; however, they will all be 1..2, and they will always be part of multiple ranges in the same request, so as far as i can tell we should be OK.

Comment 9

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm marking this bug as WONTFIX per bug #1269807.

For more information see - https://blog.mozilla.org/futurereleases/2015/10/08/npapi-plugins-in-firefox/