Skip to content

Commit

Permalink
core(target-manager): warn on errors while attaching to workers (#15740)
Browse files Browse the repository at this point in the history
  • Loading branch information
adamraine committed Jan 18, 2024
1 parent c427c54 commit e85c2ac
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
11 changes: 10 additions & 1 deletion core/gather/driver/target-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,11 @@ class TargetManager extends ProtocolEventEmitter {
async _onSessionAttached(cdpSession) {
const newSession = new ProtocolSession(cdpSession);

let targetType;

try {
const {targetInfo} = await newSession.sendCommand('Target.getTargetInfo');
const targetType = targetInfo.type;
targetType = targetInfo.type;

// TODO: should detach from target in this case?
// See pptr: https://github.com/puppeteer/puppeteer/blob/733cbecf487c71483bee8350e37030edb24bc021/src/common/Page.ts#L495-L526
Expand Down Expand Up @@ -168,6 +170,13 @@ class TargetManager extends ProtocolEventEmitter {
// Sometimes targets can be closed before we even have a chance to listen to their network activity.
if (/Target closed/.test(err.message)) return;

// Worker targets can be a bit fickle and we only enable them for diagnostic purposes.
// We shouldn't throw a fatal error if there were issues attaching to them.
if (targetType === 'worker') {
log.warn('target-manager', `Issue attaching to worker target: ${err}`);
return;
}

throw err;
} finally {
// Resume the target if it was paused, but if it's unnecessary, we don't care about the error.
Expand Down
17 changes: 17 additions & 0 deletions core/test/gather/driver/target-manager-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,23 @@ describe('TargetManager', () => {
expect(sendMock.findAllInvocations('Runtime.runIfWaitingForDebugger')).toHaveLength(4);
});

it('should ignore errors while attaching to worker targets', async () => {
targetInfo.type = 'worker';
sendMock
.mockResponse('Target.getTargetInfo', {targetInfo})
.mockResponse('Network.enable', () => {
throw new Error('Cannot use Network.enable');
})
.mockResponse('Target.setAutoAttach');
await targetManager.enable();

const invocations = sendMock.findAllInvocations('Target.setAutoAttach');
expect(invocations).toHaveLength(0);

// Should still be resumed.
expect(sendMock.findAllInvocations('Runtime.runIfWaitingForDebugger')).toHaveLength(1);
});

it('should ignore targets that are not frames or web workers', async () => {
targetInfo.type = 'service_worker';
sendMock
Expand Down

0 comments on commit e85c2ac

Please sign in to comment.