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.

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

fix: prevent erroneous new main frame #10549

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

adamraine
Copy link
Contributor

@adamraine adamraine commented Jul 12, 2023

For reasons I am still investigating, Chrome will sometimes emit a random Page.frameNavigated event that looks like this:

{
  "id": "FC1767E7420AFDD8D5D7CD935DA4C788",
  "loaderId": "E4B73E277690F2C9755A2D0C950D2FA0",
  "url": ":",
  "domainAndRegistry": "",
  "securityOrigin": "://",
  "mimeType": "text/html",
  "adFrameStatus": {
    "adFrameType": "none"
  },
  "secureContextType": "InsecureScheme",
  "crossOriginIsolatedContextType": "NotIsolatedFeatureDisabled",
  "gatedAPIFeatures": []
}

The frame ID of this event is not the same frame id as the actual main frame of the tree, however it is set as the main frame since it doesn't have a parent id.

It happens when running Lighthouse in DevTools:
GoogleChrome/lighthouse#15073

This patch ensures that the main frame of a frame tree can only be set one time so this random event does not influence the frame tree.

@OrKoN OrKoN changed the title Prevent erroneous new main frame fix: prevent erroneous new main frame Jul 13, 2023
@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Jul 13, 2023

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@adamraine
Copy link
Contributor Author

@OrKoN are the CI failures related to this patch?

@OrKoN
Copy link
Collaborator

OrKoN commented Jul 13, 2023

@adamraine did you find a reason why this event is emitted?

@adamraine
Copy link
Contributor Author

Not yet, planning on doing more investigation today.

@OrKoN
Copy link
Collaborator

OrKoN commented Jul 13, 2023

@OrKoN are the CI failures related to this patch?

probably not, restarted the failing check.

@OrKoN OrKoN merged commit cb46413 into puppeteer:main Jul 13, 2023
31 checks passed
@release-please release-please bot mentioned this pull request Jul 13, 2023
@adamraine
Copy link
Contributor Author

Update on investigation, this appears to be coming from a fenced frame (notice the target subtype). Still not sure exactly how this event gets created though.

{
  "method": "Target.attachedToTarget",
  "params": {
    "sessionId": "221795A5659470D7AC6FCBF3F311D882",
    "targetInfo": {
      "targetId": "F5B7345C9BF9413F71A9B51D22304A82",
      "type": "iframe",
      "title": "",
      "url": "",
      "attached": true,
      "canAccessOpener": false,
      "browserContextId": "6310EBC13B0F02CB468081EC10C1A4B0",
      "subtype": "fenced"
    },
    "waitingForDebugger": true
  },
  "sessionId": "B5668ED04C68E8A4178A04D5EE1EB6E0"
}

Frame tree:

{
  "frame": {
    "id": "F5B7345C9BF9413F71A9B51D22304A82",
    "loaderId": "4BDF72DDE41DDD533EFDE652EB563B30",
    "url": ":",
    "domainAndRegistry": "",
    "securityOrigin": "://",
    "mimeType": "text/html",
    "adFrameStatus": {
      "adFrameType": "none"
    },
    "secureContextType": "InsecureScheme",
    "crossOriginIsolatedContextType": "NotIsolatedFeatureDisabled",
    "gatedAPIFeatures": []
  }
}

@OrKoN
Copy link
Collaborator

OrKoN commented Jul 14, 2023

@adamraine I believe the event might be created in the same place as for regular frames. The problem seems to be that the fenced frame does not report a parent id. It is probably because it is 'fenced' but for CDP purposes we might need to still report it. Would you mind filing a crbug so that we can discuss? cc @caseq

@release-please release-please bot mentioned this pull request Jul 14, 2023
@adamraine
Copy link
Contributor Author

@release-please release-please bot mentioned this pull request Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants