Open Bug 1622209 Opened 4 years ago Updated 2 years ago

Display response for multipart content type

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(Not tracked)

People

(Reporter: Honza, Assigned: bomsy)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

This is a follow up for bug 1611081

As soon as the platform is properly supporting multipart content types - DevTools Network panel backend responsible for collecting HTTP responses - i.e. NetworkResponseListener - should properly collect all parts for mutlipart response (not just the first one).

NetworkResponseListener disconnects the output stream in onStopRequest, which is the reason why we get only the first part of multi part response.

NetworkResponseListener needs to detect nsIMultiPartChannel (by QI-ing) and keep the stream opened until onAfterLastPart is executed.

Very similar to what nsStreamListenerTee is doing now.
https://phabricator.services.mozilla.com/D66713#change-3BjiMBRVqXqi
(we can do pretty much the same in NetworkResponseListener)

See also this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1611081#c31

STR:

  1. Load https://xenon.coheris.com/ff73/
  2. Inspect the response in the Network panel

Honza

Attached image image.png

Harald, we can simply concatenate all the responses together (like on the screenshot) or come up with some improved UI.

Any idea that would improve the response-data-inspection workflow?

Honza

Flags: needinfo?(hkirschner)
Has STR: --- → yes

:Honza, if you think that's a regression, then could you try to find a regression range in using for example mozregression?

Not regression, this is a follow up for platform fix in bug bug 1611081

Honza

Any idea that would improve the response-data-inspection workflow?

That seems easiest, assuming that codemirror makes the best of the content mixed with headers.

Does the same UI apply to multipart request payloads?

Flags: needinfo?(hkirschner) → needinfo?(odvarko)
Attached image image.png

Yes, attaching an example from multipart/form-data HTML form data.

Honza

Flags: needinfo?(odvarko)
Summary: Properly collect response for multipart content type → Display request/response for multipart content type

Clarified that this is for response only. Request already works.

Summary: Display request/response for multipart content type → Display response for multipart content type
Attached image image.png

The bug 1611081 has been fixed and I am attaching a screenshot of what's broken here (following STR from comment #0)

The screenshot on the left is showing that we are missing piece of the response body.

Honza

Assignee: nobody → hmanilla

The onAfterLastPart listener never gets called

Hey Matt,
I have implemented the onAfterLastPart function but it is not getting called. From your patch for bug 1611081 , looks like it should be. I might be missing something. If you have a sec, could you pls have a look. I've also pinged you in the patch.

Thanks

Flags: needinfo?(matt.woodrow)

Is it possible that you're running into bug 1638422?

I can't see any other reason why onAfterLastPart wouldn't be forwarded through.

Flags: needinfo?(matt.woodrow)

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)

I can't see any other reason why onAfterLastPart wouldn't be forwarded through.

Could the problem be with the SameCOMIdentity function?
https://searchfox.org/mozilla-central/rev/559b25eb41c1cbffcb90a34e008b8288312fcd25/netwerk/base/nsStreamListenerTee.cpp#102

I am not seeing it in the other callbacks (onStopRequest, onStartRequests) and they work fine.

Honza

Flags: needinfo?(matt.woodrow)

The interface definition for nsIMultiPartChannelListener (in netwerk/base/nsIMultiPartChannel.idl) needs 'scriptable' for JS code to be able to implement it.

Flags: needinfo?(matt.woodrow)
Depends on: 1643196

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)

The interface definition for nsIMultiPartChannelListener (in netwerk/base/nsIMultiPartChannel.idl) needs 'scriptable' for JS code to be able to implement it.

Ah, great, thanks for filing the bug and providing a patch Matt!

Honza

Bomsy, with all deps closed, is there more work needed in this bug to make the right responses show?

Flags: needinfo?(hmanilla)

@harald Yes. i'll just complete the patch.

Flags: needinfo?(hmanilla)
diff --git a/devtools/server/actors/network-monitor/network-response-listener.js b/devtools/server/actors/network-monitor/network-response-listener.js
--- a/devtools/server/actors/network-monitor/network-response-listener.js
+++ b/devtools/server/actors/network-monitor/network-response-listener.js
@@ -161,6 +161,9 @@ NetworkResponseListener.prototype = {
    */
   request: null,

+  part: null,
+  isLastPart: false,
+
   /**
    * Set the async listener for the given nsIAsyncInputStream. This allows us to
    * wait asynchronously for any data coming from the stream.
@@ -195,8 +198,14 @@ NetworkResponseListener.prototype = {
   onDataAvailable: function(request, inputStream, offset, count) {
     this._findOpenResponse();
     const data = NetUtil.readInputStreamToString(inputStream, count);
+    this.bodySize += count;

-    this.bodySize += count;
+    if (this.isMultiPart) {
+      //console.log("onDataAvailable >> ", data, " >> count", count, offset);
+      console.log("Part id >> ", this.part);
+      console.log("Request >> ", this.request);
+      console.log("Last part >> ", this.isLastPart);
+    }

     if (!this.httpActivity.discardResponseBody) {
       const limit = Services.prefs.getIntPref(
@@ -230,6 +239,10 @@ NetworkResponseListener.prototype = {
     }

     this.isMultiPart = request instanceof Ci.nsIMultiPartChannel;
// onStartRequest
+    if (this.isMultiPart) {
+      this.part = request.partID;
+      this.isLastPart = request.isLastPart;
+    }

     this.request = request;
     this._getSecurityInfo();
@@ -387,6 +400,7 @@ NetworkResponseListener.prototype = {

   onAfterLastPart: function(status) {
     // this does not get called
+    console.log("onAfterLastPart >> ", status);
     this.sink.outputStream.close();
   },

@@ -583,6 +597,7 @@ NetworkResponseListener.prototype = {
             available
           );
         } else {
+          console.log("on data available called.", this.offset, stream);
           this.onDataAvailable(this.request, stream, this.offset, available);
         }
       }

@Matt I would love your help to clarify a couple of things

I'm trying to get some info about the parts for the MultiPartChannel but i seem to

  • always get 0 as the partID for all the parts.
  • the isLastPart is never true, even for the last part
    Also the inputStream seems to be returning the full content for both parts rather than the relevant content per part.

Are these expected? or am i missing something?

Thanks for your help

Flags: needinfo?(matt.woodrow)

Sorry for the slow response!

(In reply to Hubert Boma Manilla (:bomsy) from comment #16)

I'm trying to get some info about the parts for the MultiPartChannel but i seem to

  • always get 0 as the partID for all the parts.
  • the isLastPart is never true, even for the last part

These are because the onStartRequest implementation records the first 'request' object in this.request, and then returns early for future onStartRequest calls.

onDataAvailable isn't called by platform, but is synthesized within network-response-listener.js (from onInputStreamReady) using this.request, so it's always called with the request object for the first part.

Also the inputStream seems to be returning the full content for both parts rather than the relevant content per part.

Are these expected? or am i missing something?

Hmm, this is more concerning.

It looks like the interface nsIStreamListenerTree takes all onDataAvailable calls and copies the data into the stream object provided to it from init. onStart/StopRequest get forwarded to the observer (network-response-listener), but onDataAvailable isn't forwarded, the observer just reads (asynchronously) from the stream it provided.

Given that, it seems expected that the stream would contain all the parts concatenated together again, which probably isn't super useful for you.

Flags: needinfo?(matt.woodrow)

Baku, do you have any ideas on the best way to fix this?

It just created D81482, which has an extension to the nsIStreamListenerTee interface that is workable for multi-part responses, but I don't really know enough about the streams API to say whether it's a good idea.

Flags: needinfo?(amarchesini)

It just created D81482, which has an extension to the nsIStreamListenerTee interface that is workable for multi-part responses, but I don't really know enough about the streams API to say whether it's a good idea.

This seems a totally reasonable approach. Just to be sure, let's ask mayhemer too.

Flags: needinfo?(amarchesini) → needinfo?(honzab.moz)

Sorry for late reply. The patches seem to be fine.

Flags: needinfo?(honzab.moz)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: