347805 - Mozilla Plug-In API Proposal: Enable plugins to receive response body when an HTTP error occurs

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P2)

Description

[Jeff Mott]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)
Build Identifier: All

With Firefox 1.5, the NPAPI does indeed appear to be a serious limitation. For an ordinary successful stream, Firefox will call NPP_NewStream to start a stream and start delivering data to us via NPP_Write. But when the HTTP status code is 500, we only get a NPP_URLNotify callback, which basically just says "such-and-such URL could not be downloaded because of an error." No data ever arrives.



Reproducible: Always

Steps to Reproduce:
1. Set up a script which always reports HTTP status code 500, like Web services often do. In the response body, have it output some simple XML: <test>Hello, world</test>
2. Hit the script with NPN_GetURLNotify from a plugin

Actual Results:  
Response body unavailable

Expected Results:  
Should be some way to get it

This is a problem for the Flash Player

Comment 1

[Jeff Mott]

I realize I didn't specify what I need, so here it is:

Please deliver NPP_Write messages with the response body after a NPP_RULNotify call.

Comment 2

[Jeff Mott]

Sorry, that doesn't really make a lot of sense, there's no stream open.  I'm open to suggestions here about what could be done in / to the API to make the response body available, and I'll post again if I come up with a better plan.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Plugin API changes would be scary at this point in the 1.8.1 cycle, so minusing blocking request, although this would be good to look at.

Comment 4

[Jeff Mott]

Ok, here's the proposal:

After the browser calls NPP_URLNotify to tell the plugin there's been a network error, if there is a response from the server the browser will then call NPP_GetValue with NPPVpluginErrorStreamBool.  If the plugin returns TRUE, the browser will then pass the server response to the plugin as a normal stream, with NPP_NewStream, NPP_WriteReady, NPP_Write, and NPP_DestroyStream.

Comment 5

[Dimcho Balev]

Many flash developers have continuously asked Adobe for several HTTP related changes in the flash player. They really need:
 
1. Access to the HTTP status code
2. Access to the HTTP response headers
3. Access to the HTTP body even when the http request is unsuccessful

Response headers and status codes (1 & 2) are already available (in up-to-date browsers) via the NPStream struct passed to NPP_NewStream.

The only missing part is the HTTP bodies (3).

I am attaching my first patch that should address the problem. 

The change in summary:

In the cases of http errors the browser should asks the plugin if it is interested in receiving the body of the failed request - it will call NPP_GetValue with new enum called NPPVpluginWantsHTTPFailStreams. If the plugin does not understand what the browser is asking or if the plugin is not interested everything will work as before. In the other case the browser should send NPP_NewStream, NPP_WriteReady, NPP_Write, and NPP_DestroyStream as it does for urls that return 200 OK.

Basically it is an implementation of the same idea the Jeff has already proposed in the previous post.

Comment 6

[Dimcho Balev]

Attachment: First version of the proposed patch

Comment 7

[Dimcho Balev]

Attachment: Second version of the proposed patch

After quick review from Josh Soref I did some small changes:
1. Formatting: replaced all tabs with spaces, change the place of few commas in the enums, changed the place of one comment, some other miner formatting changes
2. Changed the type of the local variable bWantsFailHTTPStreams in ns4xPlugin.cpp from NPBool to PRBool
3. Added a brief comment in npapi.h

Comment 8

[Dimcho Balev]

Attachment: Third revision of the patch

Third revision of the patch. This one includes Opera's NPPVpluginUrlRequestsDisplayedBool constant to avoid collisions.

Comment 9

[timeless]

Comment on attachment 318947 [details] [diff] [review]
Third revision of the patch

+    if (NS_FAILED(rv)) {

note the space after |if|

+    if(responseCode > 206) { // not normal
please add a space to match the style.

+      PRBool bWantsFailHTTPStreams = PR_FALSE;
+        mInstance->GetValue(nsPluginInstanceVariable_WantsFailHTTPStreams, (void *)&bWantsFailHTTPStreams);
this is overindented.

+    if(!bWantsFailHTTPStreams){
     return NS_ERROR_FAILURE;

this is under-indented

- WantsFailHTTPStreams - thinking about this, i really don't like the name.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Updated diff.

This is an updated version of Dimcho Balev's patch, with a couple of changes listed below.

- Rename WantsFailHTTPStreams to WantsAllNetworkStreams (per discussion on plugin-futures).
- Fix timless' indentation issues etc.
- Revert changes to ',' at end of a list in the C++ code (which would break certain compilers).

Other than that this is the same fix, and I'm willing to land this as is assuming it tests out ok.

Dimcho, would you by any chance be able to test this out? Builds with this patch applied should be available shortly at:

https://build.mozilla.org/tryserver-builds/2008-07-18_16:[e-mail]-plugin-allstreams/

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 318947 [details] [diff] [review]
Third revision of the patch

r+sr=jst with the changes I mentioned above (and already made in the last patch). Thanks for the fix!

Comment 12

[Dimcho Balev]

I tested the build with the applied patch and it passes our test cases. 
Thank you for landing the change!

Comment 13

[Johnny Stenback (:jst)]

Thanks for testing! This change isn't in the main tree yet, but will be in for Firefox 3.1.

Comment 14

[Johnny Stenback (:jst)]

Fix checked in, marking FIXED. Thanks for the patch Dimcho!