583576 - Certain range-requests cause Core to destroy npstream with spurious NETWORK_ERR

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Rudi Sherry]

Attachment: plug-in and dev env to show the bug

When a plug-in is getting a stream (NP_NewStream, NP_Write etc) and during that time makes a byte-range request (NPN_RequestRead), Firefox will honor that request and them immediately call NP_DestroyStream( stream, NPERR_NETWORK_ERR ) even though there was no network error, and all the bytes of the stream have not yet been delivered to the plugin.

To make this happen, it appears that the range of the request must contain the last byte of the file, although that is not certain.

Attached is a windows plug-in (plus a project for it) that will make this request causing the problem.  It responds to PDF files (so replace nppddf32.dll).  It is not necessary to have Acrobat or Reader installed, the plug-in is self-contained.

The plug-in and symbols are in the "win/pre-built" in the unzipped attachment.

There is a mac project but it may or may not build; the windows project is the one that has been tested to demonstrate the problem.

Comment 1

[Rudi Sherry]

If we can't get this fixed we would have to change Acrobat/Reader to download the entire PDF before showing it, when in Firefox; for many PDFs this would make it take longer in Firefox than other browsers.

Comment 2

[Rudi Sherry]

This does not happen with Firefox 3.5.11; it appears to be new somewhere in the 3.6 line.

Firefox 3.5.1 acts correctly: one the request for the byte-range has been made, the original download stops, the byte-range request is satisfied with calls to NP_Write, and the stream is *not* destroyed (it is waiting for further byte-range requests).

Comment 3

[Benjamin Smedberg]

Can you check this with a Firefox 4 nightly? I cannot reproduce it on trunk (going to check with a branch build shortly). http://nightly.mozilla.org

Also in Firefox 3.6.4+, some plugins such as Flash are run out-of-process, but Acrobat is not (we had some hang issues). In Firefox 4 all plugins will be run out-of-process. Are you testing with the default settings which would be running Acrobat and your test plugin in-process?

Did this problem occur in Firefox 3.6.3, or is it new with Firefox 3.6.4?

Comment 4

[Benjamin Smedberg]

Confirmed the problem in branch builds:

 	npTestPlugin.dll!NPP_DestroyStream(instance=0x05ff908c, stream=0x0628f370, reason=0x0001)  Line 153	C++
 	xul.dll!nsNPAPIPluginStreamListener::CleanUpStream(reason=0x0001)  Line 288	C++
 	xul.dll!nsNPAPIPluginStreamListener::OnStopBinding(pluginInfo=0x0709d8c4, status=0x804b0002)  Line 815	C++
>	xul.dll!nsPluginStreamListenerPeer::OnStopRequest(request=0x00762d1c, aContext=0x00000000, aStatus=0x804b0002)  Line 2048	C++
 	xul.dll!nsMediaDocumentStreamListener::OnStopRequest(request=0x00762d1c, ctxt=0x00000000, status=0x804b0002)  Line 98	C++
 	xul.dll!nsDocumentOpenInfo::OnStopRequest(request=0x00762d1c, aCtxt=0x00000000, aStatus=0x804b0002)  Line 324	C++
 	xul.dll!nsStreamListenerTee::OnStopRequest(request=0x00762d1c, context=0x00000000, status=0x804b0002)  Line 73	C++
 	xul.dll!nsHttpChannel::OnStopRequest(request=0x00762d1c, ctxt=0x00000000, status=0x804b0002)  Line 5313	C++
 	xul.dll!nsInputStreamPump::OnStateStop()  Line 579	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(stream=0x0622ac08)  Line 404	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 192	C++
 	xul.dll!nsThread::ProcessNextEvent(mayWait=0x00000001, result=0x0034f320)  Line 533	C++

This is the original/main request (not the byte-range request).

* When we receive NPN_RequestRead, nsPluginStreamListenerPeer::RequestRead sets mAbort.
* The next nsPluginStreamListenerPeer::OnDataAvailable returns NS_BINDING_ABORTED (this is correct, I think)
* This ends up calling nsHttpChannel::OnStopRequest -> nsPluginStreamListenerPeer::OnStopRequest -> nsNPAPIPluginStreamListener::OnStopBinding -> nsNPAPIPluginStreamListener::CleanUpStream

The code looks very similar on trunk, I'm still looking to see what exactly is different.

Comment 5

[Daniel Veditz [:dveditz]]

We'll take a branch patch but this won't block a release.

Comment 6

[Benjamin Smedberg]

Why not, exactly? This is a pretty serious regression which we probably introduced in 3.6.4, and is probably responsible for the rash of "PDFs aren't displaying properly in Firefox" bugs we've been seeing.

I really think this should block 3.6.10, if not .9

Comment 7

[Benjamin Smedberg]

Attachment: Initialize mPendingRequests for full-page and embedded streams, rev. 1

I'm still trying to figure out a way to test this, but this works. In the OOPP refactoring code this got lost, I think.

Comment 8

[Rudi Sherry]

I'm going to echo Benjamin's call for making it block a release (for the obvious reasons).  As PDFs are the major user (if not the only) of byte-range requests from plug-ins, this affects us in a major way but very few (if any) other plug-ins.

Automated testing of this is a very difficult problem; we struggle with that internally in our development as well.

Comment 9

[Josh Aas]

Comment on attachment 462825 [details] [diff] [review]
Initialize mPendingRequests for full-page and embedded streams, rev. 1

I agree that this should block .9 or .10 on the 1.9.2 branch.

Comment 10

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/a0694218d21e

Comment 11

[LegNeato]

Comment on attachment 462825 [details] [diff] [review]
Initialize mPendingRequests for full-page and embedded streams, rev. 1

a=LegNeato for 1.9.2.9.

Comment 12

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/00ba242d503f

Comment 13

[Al Billings [:abillings - ex-MoCo]]

I'm trying to test this manually but it is not clear what the expected behavior here using the attached plugin.

I've created a key in the registry editor at HKLM\SOFTWARE\MozillaPlugins for the test plugin and put a Path attribute in it to where I have the dll.

About:plugins shows that it is running in the browser:

Adobe Plugin Test

File: npTestPlugin.dll
Version: 1.0.0.0
Adobe Test Plug-In For Firefox and Netscape

MIME Type 	Description 	Suffixes 	Enabled
application/pdf 	Plugin Test File 	pdf 	Yes

When I load a PDF, I just get a black page when I load http://www.irs.gov/pub/irs-pdf/f1040.pdf.

So, more details on the STR would be useful here to verify the fix.

Comment 14

[Benjamin Smedberg]

The attached plugin has NP visible behavior. The automated tests shoud be enough verification here.

Comment 15

[Rudi Sherry]

The plug-in doesn't have any behavior except to make that request; there is no "pass/fail" difference, it is not something that can be tested in a black-box way.  You need to debug and show that the stream is not closed, and no network error is being passed to it.

You have the sources to the plug-in, you could have it crash if it gets a network error and not crash if it doesn't (or put up a FAIL message box if it gets a network error).

It would be hard to do a definitive test in Reader 9.3.x, which will hang sometimes trying to read a PDF but triggering the Firefox bug depends on Reader making the right NPN_RequestRange calls at the right time;l that depends on : network speeed, latency, computer speed, specific PDF, server load, computer load etc etc.  There is no way to create a repeatable case in Reader itself.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #14)
> The attached plugin has NP visible behavior. The automated tests shoud be
> enough verification here.

There were no new automated tests in the patch. What automated tests do you mean?

Comment 17

[Benjamin Smedberg]

Oops, I'm getting my bugs confused. I'm not sure there is anything you can do to verify this bug.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Ok. Marking it as such for QA.

Comment 19

[Rudi Sherry]

The behavior has changed radically: I just tested 3.6.10 with the plug-in and the NPN_RequestRead returns NPERR_GENERIC_ERROR and is ignored.

For obscure reasons we request bytes that we may already have, so if the error is because we already have bytes 1-2 that won't work for us.

Is this deliberate?  Are you seeing the same results?