[1.9.0] "end" stage never reached on HTTP 4** & 5** responses #11163

Closed
n1k0 opened this Issue Mar 24, 2013 · 12 comments

Projects

None yet

5 participants

@n1k0
n1k0 commented Mar 24, 2013

Dunno if it's a bug or feature, at least it's not BC and I couldn't find a CHANGELOG mentioning the fix.

Sample script:

var page = require('webpage').create();
var server = require('webserver').create();
var service = server.listen(8090, function (request, response) {
  var code = parseInt(/^\/(\d+)$/.exec(request.url)[1], 10);
  response.statusCode = code;
  response.write("how");
  response.close();
});

page.onResourceReceived = function(res) {
  console.log(JSON.stringify(res, null, 4));
  console.log('received HTTP ' + res.status);
};

page.open('http://localhost:8090/400', function() {
  console.log('HTTP 400 loaded');
  server.close();
  phantom.exit();
});

PhantomJS 1.9.0 static build, OSX ML:

$ phantomjs p.js
{
    "bodySize": 3,
    "contentType": null,
    "headers": [],
    "id": 1,
    "redirectURL": null,
    "stage": "start",
    "status": 400,
    "statusText": "Bad Request",
    "time": "2013-03-24T10:22:47.046Z",
    "url": "http://localhost:8090/400"
}
received HTTP 400
HTTP 400 loaded

PhantomJS 1.8.2 static build, OSX ML:

$ phantomjs182 p.js
{
    "bodySize": 3,
    "contentType": null,
    "headers": [],
    "id": 1,
    "redirectURL": null,
    "stage": "start",
    "status": 400,
    "statusText": "Bad Request",
    "time": "2013-03-24T10:23:46.394Z",
    "url": "http://localhost:8090/400"
}
received HTTP 400
{
    "contentType": null,
    "headers": [],
    "id": 1,
    "redirectURL": null,
    "stage": "end",
    "status": 400,
    "statusText": "Bad Request",
    "time": "2013-03-24T10:23:46.394Z",
    "url": "http://localhost:8090/400"
}
received HTTP 400
HTTP 400 loaded

Edit: looks like the problem occurs for all 4** and 5** http statuses as well.

@JamesMGreene
Collaborator

I'm guessing this is due to the addition of the new WebPage#onResourceError callback but I thought that was only triggered for networking errors rather than legit HTTP errors. @Vitallium?

@Vitallium
Collaborator

Callback WebPage#onResourceError invokes for all errors. It indicates that something went wrong during the network request.

@n1k0
n1k0 commented Mar 24, 2013

While I'm ok to consider 4** and 5** being error codes (see the HTTP spec), this change is not BC at all, and should therefore be mentioned somewhere with big warnings.

Edit: Also, I don't really see what would prevent to receive a response at end stage, even if related request is considered in error anyway. Eg. maybe one would want to inspect the received resource properties, who knows.

@JamesMGreene
Collaborator

Hmm, that's not the behavior I anticipated. Is it possible to either fire both or else filter which ones fire the onResourceError callback (i.e. not standard HTTP errors)?

@ariya
Owner
ariya commented Mar 24, 2013

That can be a very inconvenient behavior change.

Two possible solutions

(1) A compromise: detect if the page specifies resource error handler or not. If not, continue to fire the received callback.

(2) Still fire the received callback even if there is an error.

@Vitallium Vitallium was assigned Mar 25, 2013
@Vitallium
Collaborator

I'd prefer the second option. To implement this, I need to change only 3-4 lines of code.
Is it ok?

@JamesMGreene
Collaborator

Yes, Option 2 is definitely the right way to patch this.

@ariya
Owner
ariya commented Mar 28, 2013

#2 is fine.

@n1k0
n1k0 commented Mar 28, 2013

+1 for (2) as well

@Vitallium
Collaborator
@Vitallium Vitallium closed this Apr 3, 2013
@dotjs dotjs added a commit to cloudflare/phantomjs that referenced this issue Apr 26, 2013
@dotjs dotjs Fire `onResourceReceived` callback when the resource error occured. e27d6ad
@mle-ii
mle-ii commented Jun 28, 2013

I'm confused. The change log says it's in this release and in a future release. Is it in 1.9.1?
https://github.com/ariya/phantomjs/blob/master/ChangeLog

Also, please update the home page I didn't realize there was a newer version until today.

Thanks for fixing this, I can't wait to try out the latest additions to PhantomJS and CasperJS.

@ariya
Owner
ariya commented Jun 29, 2013

@mle-ii It's in both releases (the fix was backported). Also, if you want to track all releases and progress, subscribe to the mailing-list and/or follow https://twitter.com/phantomjs. We often don't have time to explain every details/update every corners, so please be (pro)active.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment