Skip to content

Commit

Permalink
fix: make Request.frame throw when page is not ready yet (#26616)
Browse files Browse the repository at this point in the history
Suggest to check `request.isNavigationRequest()` beforehand.

Fixes #24603.
  • Loading branch information
dgozman committed Aug 22, 2023
1 parent bb808ca commit 5646875
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 8 deletions.
48 changes: 44 additions & 4 deletions docs/src/api/class-request.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,41 @@ page.RequestFailed += (_, request) =>

Returns the [Frame] that initiated this request.

**Usage**

```js
const frameUrl = request.frame().url();
```

```java
String frameUrl = request.frame().url();
```

```py
frame_url = request.frame.url
```

```csharp
var frameUrl = request.Frame.Url;
```

**Details**

Note that in some cases the frame is not available, and this method will throw.
* When request originates in the Service Worker. You can use `request.serviceWorker()` to check that.
* When navigation request is issued before the corresponding frame is created. You can use [`method: Request.isNavigationRequest`] to check that.

Here is an example that handles all the cases:

```js
if (request.serviceWorker())
console.log(`request ${request.url()} from a service worker`);
else if (request.isNavigationRequest())
console.log(`request ${request.url()} is a navigation request`);
else
console.log(`request ${request.url()} from a frame ${request.frame().url()}`);
```

## method: Request.headers
* since: v1.8
- returns: <[Object]<[string], [string]>>
Expand Down Expand Up @@ -103,6 +138,9 @@ Name of the header.

Whether this request is driving frame's navigation.

Some navigation requests are issued before the corresponding frame is created, and therefore
do not have [`method: Request.frame`] available.

## method: Request.method
* since: v1.8
- returns: <[string]>
Expand Down Expand Up @@ -252,12 +290,14 @@ Returns the matching [Response] object, or `null` if the response was not receiv
* langs: js
- returns: <[null]|[Worker]>

:::note
This field is Chromium only. It's safe to call when using other browsers, but it will always be `null`.
:::

The Service [Worker] that is performing the request.

**Details**

This method is Chromium only. It's safe to call when using other browsers, but it will always be `null`.

Requests originated in a Service Worker do not have a [`method: Request.frame`] available.

## async method: Request.sizes
* since: v1.15
- returns: <[Object]>
Expand Down
13 changes: 11 additions & 2 deletions packages/playwright-core/src/client/network.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,15 @@ export class Request extends ChannelOwner<channels.RequestChannel> implements ap
assert(this.serviceWorker());
throw new Error('Service Worker requests do not have an associated frame.');
}
return Frame.from(this._initializer.frame);
const frame = Frame.from(this._initializer.frame);
if (!frame._page) {
throw new Error([
'Frame for this navigation request is not available, because the request',
'was issued before the frame is created. You can check whether the request',
'is a navigation request by calling isNavigationRequest() method.',
].join('\n'));
}
return frame;
}

serviceWorker(): Worker | null {
Expand Down Expand Up @@ -267,7 +275,8 @@ export class Request extends ChannelOwner<channels.RequestChannel> implements ap
}

_targetClosedScope(): LongStandingScope {
return this.serviceWorker()?._closedScope || this.frame()._page?._closedOrCrashedScope || new LongStandingScope();
const frame = Frame.fromNullable(this._initializer.frame);
return this.serviceWorker()?._closedScope || frame?._page?._closedOrCrashedScope || new LongStandingScope();
}
}

Expand Down
38 changes: 36 additions & 2 deletions packages/playwright-core/types/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18050,6 +18050,32 @@ export interface Request {

/**
* Returns the {@link Frame} that initiated this request.
*
* **Usage**
*
* ```js
* const frameUrl = request.frame().url();
* ```
*
* **Details**
*
* Note that in some cases the frame is not available, and this method will throw.
* - When request originates in the Service Worker. You can use `request.serviceWorker()` to check that.
* - When navigation request is issued before the corresponding frame is created. You can use
* [request.isNavigationRequest()](https://playwright.dev/docs/api/class-request#request-is-navigation-request) to
* check that.
*
* Here is an example that handles all the cases:
*
* ```js
* if (request.serviceWorker())
* console.log(`request ${request.url()} from a service worker`);
* else if (request.isNavigationRequest())
* console.log(`request ${request.url()} is a navigation request`);
* else
* console.log(`request ${request.url()} from a frame ${request.frame().url()}`);
* ```
*
*/
frame(): Frame;

Expand Down Expand Up @@ -18086,6 +18112,9 @@ export interface Request {

/**
* Whether this request is driving frame's navigation.
*
* Some navigation requests are issued before the corresponding frame is created, and therefore do not have
* [request.frame()](https://playwright.dev/docs/api/class-request#request-frame) available.
*/
isNavigationRequest(): boolean;

Expand Down Expand Up @@ -18166,9 +18195,14 @@ export interface Request {
response(): Promise<null|Response>;

/**
* **NOTE** This field is Chromium only. It's safe to call when using other browsers, but it will always be `null`.
*
* The Service {@link Worker} that is performing the request.
*
* **Details**
*
* This method is Chromium only. It's safe to call when using other browsers, but it will always be `null`.
*
* Requests originated in a Service Worker do not have a
* [request.frame()](https://playwright.dev/docs/api/class-request#request-frame) available.
*/
serviceWorker(): null|Worker;

Expand Down
23 changes: 23 additions & 0 deletions tests/page/page-network-request.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -425,3 +425,26 @@ it('should report all cookies in one header', async ({ page, server, isElectron,
const cookie = (await response.request().allHeaders())['cookie'];
expect(cookie).toBe('myCookie=myValue; myOtherCookie=myOtherValue');
});

it('should not allow to access frame on popup main request', async ({ page, server }) => {
await page.setContent(`<a target=_blank href="${server.EMPTY_PAGE}">click me</a>`);
const requestPromise = page.context().waitForEvent('request');
const popupPromise = page.context().waitForEvent('page');
const clicked = page.getByText('click me').click();
const request = await requestPromise;

expect(request.isNavigationRequest()).toBe(true);

let error;
try {
request.frame();
} catch (e) {
error = e;
}
expect(error.message).toContain('Frame for this navigation request is not available');

const response = await request.response();
await response.finished();
await popupPromise;
await clicked;
});

0 comments on commit 5646875

Please sign in to comment.