added response body to response object in onResourceReceived event

[dparshin]

added response body to response object in onResourceReceived event

#10158

[richardjharris]

This is useful, thanks! I think having a full proxy interface will be useful for many things.

For comparison, here is a small patch I was previously using to collect resource content.

diff --git c/src/networkaccessmanager.cpp w/src/networkaccessmanager.cpp
index 17112b3..a73218d 100644
--- c/src/networkaccessmanager.cpp
+++ w/src/networkaccessmanager.cpp
@@ -303,11 +307,20 @@ void NetworkAccessManager::handleStarted()
     QNetworkReply *reply = qobject_cast<QNetworkReply*>(sender());
     if (!reply)
         return;
+
+    QByteArray chunk( reply->peek(reply->size()) );
+    if (m_content.contains(reply)) {
+        m_content[reply].append(chunk);
+    }
+    else {
+        m_content[reply] = chunk;
+    }
+
     if (m_started.contains(reply))
         return;

     m_started += reply;
@@ -367,6 +379,8 @@ void NetworkAccessManager::handleFinished(QNetworkReply *reply, const QVariant &
         headers += header;
     }

+    QByteArray content = m_content.value(reply, QByteArray());
+
     QVariantMap data;
     data["stage"] = "end";
     data["id"] = m_ids.value(reply);
@@ -377,9 +391,12 @@ void NetworkAccessManager::handleFinished(QNetworkReply *reply, const QVariant &
     data["redirectURL"] = reply->header(QNetworkRequest::LocationHeader);
     data["headers"] = headers;
     data["time"] = QDateTime::currentDateTime();
+    data["body"] = content.toBase64().data();
+    data["bodySize"] = content.size();

     m_ids.remove(reply);
     m_started.remove(reply);
+    m_content.remove(reply);

     emit resourceReceived(data);
 }
diff --git c/src/networkaccessmanager.h w/src/networkaccessmanager.h
index 1b1a8af..e49351a 100644
--- c/src/networkaccessmanager.h
+++ w/src/networkaccessmanager.h
@@ -108,6 +109,7 @@ private slots:
 private:
     QHash<QNetworkReply*, int> m_ids;
     QSet<QNetworkReply*> m_started;
+    QHash<QNetworkReply*, QByteArray> m_content;
     int m_idCounter;
     QNetworkDiskCache* m_networkDiskCache;
     QVariantMap m_customHeaders;

[dustypebbles]

Hi,

These are great patches adding a much needed feature.

Is there a reason you guys are base64 encoding the body? Couldn't we call btoa() on the value ourselves if we want that?

[dparshin]

@richardjharris: I thought of using 'reply->peek', but it is possible that QWebPage(or any other consumer) can read only a part of the reply, leaving rest of data in reply buffer, in this case on next chunk you will get overlapped data in m_content.

[dparshin]

@dustypebbles : to be honest, I couldn't find a good way to pass binary data to javascript, at first I just used QString without base64 encoding, but it turned out QString cuts the string at first 0 byte.

[richardjharris]

Yeah, if you pass binary data you get truncated content. Typed arrays are the correct way to do it but I am unsure if phantomjs's version of JavascriptCore supports them.

[dustypebbles]

Passing a length to the QString constructor seems to work fine for the filesystem module: https://github.com/ariya/phantomjs/blob/master/src/filesystem.cpp#L109

[dustypebbles]

All constructors are listed in QString documentation: https://qt-project.org/doc/qt-4.8/qstring.html
A QByteArray can be passed straight!

[vitallium]

This needs a lot of tests. To make sure everything works, especially AJAX requests.

[dparshin]

@dustypebbles removed bas64 encoding, now body is converted in the same way as in filesystem module.

[pongells]

coming from here

I am a bit lost, would this allow for storing loaded resources? (e.g. if a page in Phantom loads an image, we can store that image on disk without having to do something like a wget)

[osthafen]

@pongells
Yes that would allow for storing loaded resources.
I am desperately looking for this feature as well.

[dparshin]

@vitallium @dustypebbles thanks for your comments, anything else I can do to accelerate review of this patch?

[tomvangoethem]

I built phantomjs with this commit included (on MacOSX and Ubuntu), but onResourceReceived events are still fired twice: once with an empty body and once with the full body.
This is the script I ran to test:

var system = require('system'),
    fs = require("fs"),
    page = require('webpage').create();

page.onResourceReceived = function(r) {
    console.log('received: "' + r.url + '" with ' + ((r.body === '') ? 'no body' : ('"' + r.body + '" as body')));
};

page.open('http://tomvglabs.be/phantomjs/resource-test.html', function (status) {
    if (status === "success") {
        console.log('opened page!');
    }
    phantom.exit();
});

And this is the result:

$ ./phantomjs resource_test.js
received: "http://tomvglabs.be/phantomjs/resource-test.html" with no body
received: "http://tomvglabs.be/phantomjs/resource-test.html" with "<html><head><link rel="stylesheet" type="text/css" href="style.css"></head></html>" as body
received: "http://tomvglabs.be/phantomjs/style.css" with no body
received: "http://tomvglabs.be/phantomjs/style.css" with "body { background-color: red; }" as body
opened page!

The resources with empty body are unexpected here...

Also: bodySize is undefined when the body is set (not so when no body is set)
Also: when a resource is requested which returns status 204 (No response), onResourceReceived is called only once (with a body). In this case the body is the null byte ('\u0000')

[dparshin]

Please take into account 'stage' attribute of the response object, it takes one of two values: 'start', 'end'. As far as I understand 'start' event is emitted when HTTP headers received, thus it may contain proper 'bodySize' (content-length header), but the body itself can only be obtained on 'end' stage, when all body chunks are received. This is how phantomjs have been working even in absence of my patch. Though I'm going to fix undefined 'bodySize' at the end stage.

[tomvangoethem]

Oh sorry, I was unaware that onResourceReceived was fired twice. The API Reference does not mention this.

Anyways: great stuff you're doing 👍

I evaluated the stability of this patch by visiting the homepage of Alexa's Top 100 websites, and encountered no crashes nor additional errors.

[dparshin]

@tomvangoethem I tried to reproduce the problem, but could not get results you've described. I'm using test/webpage-spec.js test to reproduce:

    it("should return properly from a 401 status", function() {
        var page = require('webpage').create();
        var server = require('webserver').create();
        server.listen(12345, function(request, response) {
            response.statusCode = 200;
//            modifing this line
//            response.write('df');
            response.close();
        });

        var url = "http://localhost:12345/foo";
        var handled = 0;
        runs(function() {
            page.onResourceReceived = function(resource) {
                handled++;
            };
            page.open(url, function(status) {
            });
        });

        waits(1000);

        runs(function() {
            expect(handled).toEqual(1);
            page.onResourceReceived = null;
            server.close();
        });

    });

The behavior I get:

1. If response doesn't contain the body(empty body, or no 'response.write' at all), on onResourceReceived event is fired
2. If response contains any amount of characters, two onResourceReceived events fired.

[tomvangoethem]

@dparshin This is the PhantomJS script I ran.

And this is the result I get back:

onResourceReceived called: (http://tomvglabs.be/phantomjs/resource-test.html) stage=start
onResourceReceived called: (http://tomvglabs.be/phantomjs/resource-test.html) stage=end
=== START BODY ===
"<html><head><link rel=\"stylesheet\" type=\"text/css\" href=\"style.css\"><img src=\"204.php\"></head></html>"
=== END BODY ===
onResourceReceived called: (http://tomvglabs.be/phantomjs/style.css) stage=start
onResourceReceived called: (http://tomvglabs.be/phantomjs/style.css) stage=end
=== START BODY ===
"body { background-color: red; }"
=== END BODY ===
onResourceReceived called: (http://tomvglabs.be/phantomjs/204.php) stage=end
=== START BODY ===
"\u0000"
=== END BODY ===
opened http://tomvglabs.be/phantomjs/resource-test.html

As you can see: there is no entry of 204.php with stage=start. And the body (wrongfully) contains the NULL-byte in stage=end.

This is the PHP code I have running on my server:

<?php
header("status: 204"); 
header("HTTP/1.0 204 No Response");
?>

These are the headers sent by the server when requesting the 204.php file:

  HTTP/1.1 204 No Response
  Date: Tue, 03 Sep 2013 16:38:01 GMT
  Server: Apache
  Content-Length: 0
  Keep-Alive: timeout=5, max=100
  Connection: Keep-Alive
  Content-Type: text/html

This is the result of running the same file (with the body-part commented out ofcourse) with PhantomJS 1.9.1:

onResourceReceived called: (http://tomvglabs.be/phantomjs/resource-test.html) stage=start
onResourceReceived called: (http://tomvglabs.be/phantomjs/resource-test.html) stage=end
onResourceReceived called: (http://tomvglabs.be/phantomjs/style.css) stage=start
onResourceReceived called: (http://tomvglabs.be/phantomjs/style.css) stage=end
onResourceReceived called: (http://tomvglabs.be/phantomjs/204.php) stage=end
opened http://tomvglabs.be/phantomjs/resource-test.html

So the issue is that the body (r.body) is not empty, but contains the NULL byte ('\u0000').

[laurentj]

Hi, this is a useful feature. I implemented it months ago into SlimerJS. However, this is a feature that could take many memory resources (images, videos) ... uselessly, if we don't need the body! This is why in SlimerJS you have to indicate content types of resources for which you want to have the body, in an array webpage.captureContent. this is an array of regular expressions that should match the mime type of content you want to have in the body property.

webpage.captureContent = [ /css/, /image\/.*/ ]

If the content type of the resource does not match one of this regular expression or if it is not the main page, the body property is empty.

What do you think about it?

[chrisparnin]

Just cherry-picked these commits into latest 1.9 branch and got this to build and work on windows. There was one issue in the code though, setHeader() isn't declared in the networkaccessmanger.h, which raises compile errors. Just adding the one line as follows resolves the issue.

class JsNetworkRequest : public QObject
{
     Q_OBJECT
     public:
           JsNetworkRequest(QNetworkRequest* request, QObject* parent = 0);
           Q_INVOKABLE void abort();
           Q_INVOKABLE void changeUrl(const QString& url);
       bool setHeader(const QString& name, const QVariant& value);
     ....
};

[masahirominami]

hi, we are using this for our cases, and

[masahirominami]

hi, sorry accidentally posted, we are using this code and mime type selector will help in our case.

we are using phantomjs with casperjs, so problem is using onResourceReceived make casperjs download() function useless. we need to change download() so that it uses onResourceReceived().

we are running this code on windows and macosx.

[ghost]

+1 This would be immensely useful for my use case as well. I understand the hesitation but can there be a flag that must be run for PhantomJS to enable this feature? The option to parse the body of a request/response is pretty useful.

[pbkhrv]

One more for "would like to see #11484 in master" here

[ghost]

I wonder if here could be an option here for capturing content by MIME type. I.e., I would prefer to specify only loading body for "application/json" requests. My use case for this feature was being able to snoop on server responses, not so much being able to read individual files.

[dparshin]

You still can capture all the content(using ".*" mask) and use only those responses you actually need based on content-type header, will this work?

[vitallium]

This is really risky to implement this as a proxy between PhantomJS and Webkit. I remember we had a lot of issues with similar implementation.

[dparshin]

@vitallium @ariya we've been using this patch for couple of years, issuing about 100k requests daily. I'm pretty sure its not that risky, if you could point me to the issues you have encountered with this approach I'd gladly verify them and make fixes if necessary.

[vitallium]

@dparshin in this case, I think we can merge it.
/cc @ariya

[vitallium]

Ok, I'm going to merge this! I had been playing with it some time. Looks good and stable. If something will not work, blame it on me.
/cc @ariya @dparshin

@dparshin could you please add your name and email to networkreplyproxy.[cpp|h] and networkreplytracker.[cpp|h]?

[dparshin]

@vitallium, done

[vitallium]

@dparshin landed! Thank you! You did awesome job!

[vitallium]

Actually, wait. I can't merge it normally :X

[dparshin]

@vitallium, is there anything wrong with it?

[vitallium]

@dparshin no, I just need to merge it correctly

[vitallium]

Phew. Now it's landed! Thanks!

[dparshin]

@vitallium, great!Thanks.

[kriegaex]

Sorry for the stupid question: The PR was closed, but was it also merged into master or another branch? I cannot find the changes when browsing the repo.

[vitallium]

@kriegaex yes, it was merged into master. But this PR caused weird errors for sync AJAX requests, and we reverted it. We landed a fix for sync AJAX requests some time ago. I think we can bring this feature back into the master branch.

[erikdubbelboer]

@vitallium any ETA? Do you plan to use the same type of proxy class or will that not work anymore because of the AJAX issue?

[vitallium]

@erikdubbelboer I think we can just pull the original code back. I'll try to land it this weekend.

[dpanic]

What is the situation with response.body in onResourceReceived on 2.1.1?

[yeyu456]

Same question.Is it done yet? I need that feature.Anything i can do right now? like using some old version.

[dmncls]

I have some phantom scripts dependent on this functionality (written when it was working with an earlier version of 2.1.1)....and I really needed the functionality back so I have temporarily built using the commit bced581 - it works for my use case. (I hope this helps someone else, and that this functionality is back in an official version soon!)

[redzedi]

Using this commit causes Phantomjs to crash when navigating out of a page that has a window.onbeforeunload function that fires a async ajax POST request . Please refer to the html below :-

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript">
    function testCallbackComplete(){
        console.log("Done onbeforeunload !!");
    }
        window.onbeforeunload = function(){
            var xmlRequest = new XMLHttpRequest();
            xmlRequest.onreadystatechange = testCallbackComplete;
            xmlRequest.open("POST", encodeURI("http://localhost:5000/testEcho"), true);
       xmlRequest.setRequestHeader("Content-Type", "application/json");
        xmlRequest.send("{\"test\":true}");
        return;
    } 

    </script>
    <title>testing On Before Unload</title>
</head>
<body>
   <p>
    <a href="index.html" id="goToIndex">Go to Index</a>
   </p>

</body>
</html>