Closed Bug 347805 Opened 15 years ago Closed 13 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: jmott, Assigned: jst)

References

Details

Attachments

(2 files, 2 obsolete files)

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
Component: General → Plug-ins
Product: Firefox → Core
Version: unspecified → Trunk
Summary: Plugin has no way to get response body when an HTTP error occurs → Mozilla Plug-In API Proposal: Enable plugins to receive response body when an HTTP error occurs
URL: n/a
QA Contact: general → plugins
Flags: blocking1.8.1?
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.
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8.1? → blocking1.8.1-
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.
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.
Flags: wanted-next+
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
Attachment #316126 - Attachment is obsolete: true
Attachment #316143 - Attachment is obsolete: true
Third revision of the patch. This one includes Opera's NPPVpluginUrlRequestsDisplayedBool constant to avoid collisions.
Attachment #318947 - Flags: review?
Attachment #318947 - Flags: review? → review?(jst)
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.
Attached patch Updated diff.Splinter Review
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:16-jst@mozilla.com-plugin-allstreams/
Assignee: nobody → jst
Status: NEW → ASSIGNED
Flags: wanted1.9.1+
Priority: -- → P2
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!
Attachment #318947 - Flags: superreview+
Attachment #318947 - Flags: review?(jst)
Attachment #318947 - Flags: review+
I tested the build with the applied patch and it passes our test cases. 
Thank you for landing the change!
Thanks for testing! This change isn't in the main tree yet, but will be in for Firefox 3.1.
Keywords: checkin-needed
Attachment #330324 - Attachment is patch: true
Attachment #330324 - Attachment mime type: application/octet-stream → text/plain
Fix checked in, marking FIXED. Thanks for the patch Dimcho! 
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Duplicate of this bug: 493714
You need to log in before you can comment on or make changes to this bug.