Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(Network): explicitly throw on content request for redirect response #2352

Merged
merged 2 commits into from
Apr 11, 2018
Merged

fix(Network): explicitly throw on content request for redirect response #2352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df293ba
fix(Network): explicitly throw on content request for redirect response
aslushnikov Apr 11, 2018
b7831c3
address comments
aslushnikov Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions lib/NetworkManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ class NetworkManager extends EventEmitter {
const response = new Response(this._client, request, redirectStatus, redirectHeaders, fromDiskCache, fromServiceWorker, securityDetails);
request._response = response;
request._redirectChain.push(request);
response._bodyLoadedPromiseFulfill.call(null, new Error('Response body is unavailable for redirect responses'));
this._requestIdToRequest.delete(request._requestId);
this._interceptionIdToRequest.delete(request._interceptionId);
this._attemptedAuthentications.delete(request._interceptionId);
Expand Down Expand Up @@ -274,7 +275,7 @@ class NetworkManager extends EventEmitter {
// @see https://crbug.com/750469
if (!request)
return;
request._completePromiseFulfill.call(null);
request.response()._bodyLoadedPromiseFulfill.call(null);
this._requestIdToRequest.delete(request._requestId);
this._interceptionIdToRequest.delete(request._interceptionId);
this._attemptedAuthentications.delete(request._interceptionId);
Expand All @@ -291,7 +292,9 @@ class NetworkManager extends EventEmitter {
if (!request)
return;
request._failureText = event.errorText;
request._completePromiseFulfill.call(null);
const response = request.response();
if (response)
response._bodyLoadedPromiseFulfill.call(null);
this._requestIdToRequest.delete(request._requestId);
this._interceptionIdToRequest.delete(request._interceptionId);
this._attemptedAuthentications.delete(request._interceptionId);
Expand Down Expand Up @@ -319,9 +322,6 @@ class Request {
this._interceptionHandled = false;
this._response = null;
this._failureText = null;
this._completePromise = new Promise(fulfill => {
this._completePromiseFulfill = fulfill;
});

this._url = url;
this._resourceType = resourceType.toLowerCase();
Expand Down Expand Up @@ -523,6 +523,10 @@ class Response {
this._request = request;
this._contentPromise = null;

this._bodyLoadedPromise = new Promise(fulfill => {
this._bodyLoadedPromiseFulfill = fulfill;
});

this._status = status;
this._url = request.url();
this._fromDiskCache = fromDiskCache;
Expand Down Expand Up @@ -581,7 +585,9 @@ class Response {
*/
buffer() {
if (!this._contentPromise) {
this._contentPromise = this._request._completePromise.then(async() => {
this._contentPromise = this._bodyLoadedPromise.then(async error => {
if (error)
throw error;
const response = await this._client.send('Network.getResponseBody', {
requestId: this._request._requestId
});
Expand Down
11 changes: 11 additions & 0 deletions test/network.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,17 @@ module.exports.addTests = function({testRunner, expect}) {
expect(await response.text()).toBe('{"foo": "bar"}\n');
expect(await response.json()).toEqual({foo: 'bar'});
});
it('Page.Events.Response should throw when requesting body of redirected response', async({page, server}) => {
server.setRedirect('/foo.html', '/empty.html');
const response = await page.goto(server.PREFIX + '/foo.html');
const redirectChain = response.request().redirectChain();
expect(redirectChain.length).toBe(1);
const redirected = redirectChain[0].response();
expect(redirected.status()).toBe(302);
let error = null;
await redirected.text().catch(e => error = e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for next time let error = await redirected.text().catch(e => e)

expect(error.message).toContain('Response body is unavailable for redirect responses');
});
it('Page.Events.Response should not report body unless request is finished', async({page, server}) => {
await page.goto(server.EMPTY_PAGE);
// Setup server to trap request.
Expand Down