Skip to content

fix: suppress the error if unexpected extraInfo events are detected - #7808

Merged
OrKoN merged 2 commits into
mainfrom
issue-7805
Nov 29, 2021
Merged

OrKoN merged 2 commits into
mainfrom
issue-7805

Conversation

@OrKoN

@OrKoN OrKoN commented Nov 28, 2021

Copy link
Copy Markdown
Collaborator

Issues: #7805

@OrKoN
OrKoN requested a review from josepharhar November 28, 2021 19:06
@google-cla google-cla Bot added the cla: yes label Nov 28, 2021
@OrKoN

OrKoN commented Nov 28, 2021

Copy link
Copy Markdown
Collaborator Author

Hey @josepharhar! Could you please take a look at this PR and the report #7805? I was able to reproduce the issue using the scenario from the report on Linux. It seems that a responseReceived event comes with hasExtraInfo = false but there is an extraInfo event in the cache:

// response
{
  requestId: '3140404.20',
  loaderId: 'ED744A4C49834781224817923ED12434',
  timestamp: 2394061.923203,
  type: 'Image',
  response: {
    url: 'https://googleads.g.doubleclick.net/adsid/google/si?gadsid=AORoGNTTfIduWIVdfg2--ZRhf5885XatMWRILAtAQH3mF9skPGi4NARWPw',
    status: 204,
    statusText: '',
    headers: {},
    mimeType: 'text/html',
    connectionReused: true,
    connectionId: 127,
    remoteIPAddress: '108.177.15.157',
    remotePort: 443,
    fromDiskCache: false,
    fromServiceWorker: false,
    fromPrefetchCache: false,
    encodedDataLength: 59,
    timing: {
      requestTime: 2394061.919692,
      proxyStart: 0.119,
      proxyEnd: 0.122,
      dnsStart: -1,
      dnsEnd: -1,
      connectStart: -1,
      connectEnd: -1,
      sslStart: -1,
      sslEnd: -1,
      workerStart: -1,
      workerReady: -1,
      workerFetchStart: -1,
      workerRespondWithSettled: -1,
      sendStart: 0.185,
      sendEnd: 0.286,
      pushStart: 0,
      pushEnd: 0,
      receiveHeadersEnd: 2.588
    },
    responseTime: 1638125871824.849,
    protocol: 'h2',
    securityState: 'unknown'
  },
  hasExtraInfo: false,
  frameId: '562B31F1971A9BEC48A040867814A468'
}
// extraInfo in the cache
{
  requestId: '3140404.20',
  blockedCookies: [],
  headers: {
    p3p: 'CP="This is not a P3P policy! See http://support.google.com/accounts/answer/151657 for more info."',
    'timing-allow-origin': '*',
    'cross-origin-resource-policy': 'cross-origin',
    'cache-control': 'private, max-age=15',
    'content-type': 'text/html; charset=UTF-8',
    'x-content-type-options': 'nosniff',
    date: 'Sun, 28 Nov 2021 18:57:51 GMT',
    server: 'cafe',
    'content-length': '0',
    'x-xss-protection': '0'
  },
  resourceIPAddressSpace: 'Public',
  statusCode: 204
}

@josepharhar josepharhar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it make sense to remove the thrown error instead? I'm having a hard time understanding exactly what the added if (!extraInfo) does...

Also, instead of throwing an error, is there any other way we can signal that something bad happened so we can fix it in chromium?

@OrKoN

OrKoN commented Nov 29, 2021

Copy link
Copy Markdown
Collaborator Author

we can swallow it and log using debugError, I guess, although I am not very familiar with how it's done in Puppeteer.

@OrKoN

OrKoN commented Nov 29, 2021

Copy link
Copy Markdown
Collaborator Author

I have updated the CL. So do you think that chromium is sending wrong events here?

@josepharhar

Copy link
Copy Markdown
Contributor

If hasExtraInfo is true and we aren't actually getting a responseReceivedExtraInfo event, then yes, it's a chromium bug. Is that the case? I can't tell from the bug report.

@OrKoN

OrKoN commented Nov 29, 2021

Copy link
Copy Markdown
Collaborator Author

@josepharhar see my comment above #7808 (comment) it's hasExtraInfo=false but there is an responseReceivedExtraInfo event.

Also, it seems to timeout if I ignore the error...

@OrKoN

OrKoN commented Nov 29, 2021

Copy link
Copy Markdown
Collaborator Author

Ah, sorry, my bad, correction: it's not timing out.

@josepharhar

Copy link
Copy Markdown
Contributor

it's hasExtraInfo=false but there is an responseReceivedExtraInfo event.

Ah I see. Yeah I think the right thing to do is ignore the ExtraInfo event when hasExtraInfo=false, which this patch currently does. If instead we were to use an ExtraInfo event if present, then we could get flaky behavior where it would pick up the ExtraInfo event sometimes but not other times.

Also, it seems to timeout if I ignore the error...

Ah, sorry, my bad, correction: it's not timing out.

I'm guessing this means we're good to merge this

@OrKoN OrKoN changed the title fix: handle extraInfo events even if event.hasExtraInfo === false fix: suppress the error if unexpected extraInfo events are detected Nov 29, 2021
@hanselfmu
hanselfmu self-requested a review November 29, 2021 19:44

@hanselfmu hanselfmu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants