Skip to content

Fix devtools iframe backend ready bug#54805

Closed
AleksanderBodurri wants to merge 3 commits into
angular:mainfrom
AleksanderBodurri:fix-devtools-iframe-backend-ready-bug
Closed

Fix devtools iframe backend ready bug#54805
AleksanderBodurri wants to merge 3 commits into
angular:mainfrom
AleksanderBodurri:fix-devtools-iframe-backend-ready-bug

Conversation

@AleksanderBodurri

@AleksanderBodurri AleksanderBodurri commented Mar 11, 2024

Copy link
Copy Markdown
Member

Previously, a race condition could cause DevTools to enter a state where it could not detect an application after a reload. This was caused by a sequencing issue between the content script connection, the devtools panel connection and an event "backendReady" that lets DevTools know when a particular frame is ready to be inspected.

Two commits that caused this behaviour were reverted, along with the features they implemented.

These two commits have been re-reverted, and one new commit has been introduced fixing the issue. This new commit replaces the previously stored backendReady boolean with a promise, so that the devtools panel can eventually run a callback to connect to a content script when that content script emits it's backendReady message.

Related #53953

dgp1130
dgp1130 reviewed Mar 11, 2024
View reviewed changes
Comment thread devtools/projects/shell-browser/src/app/tab_manager.ts Outdated
Comment thread devtools/projects/shell-browser/src/app/tab_manager.ts Outdated
Comment thread devtools/projects/shell-browser/src/app/tab_manager.ts Outdated
@AleksanderBodurri
AleksanderBodurri force-pushed the fix-devtools-iframe-backend-ready-bug branch from ed9f9a2 to 96a3298 Compare March 12, 2024 05:56
@pullapprove
pullapprove Bot requested a review from josephperrott March 12, 2024 05:56
@AleksanderBodurri
AleksanderBodurri force-pushed the fix-devtools-iframe-backend-ready-bug branch 2 times, most recently from d7ab4e6 to 02471b7 Compare March 12, 2024 06:16
josephperrott
josephperrott approved these changes Mar 12, 2024
View reviewed changes

@josephperrott josephperrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: dev-infra

@AleksanderBodurri
AleksanderBodurri force-pushed the fix-devtools-iframe-backend-ready-bug branch from 02471b7 to 8cefec5 Compare March 12, 2024 15:48
dgp1130
dgp1130 reviewed Mar 12, 2024
View reviewed changes
Comment thread devtools/projects/shell-browser/src/app/tab_manager.ts Outdated
Comment thread devtools/tsconfig.spec.json Outdated
Comment thread devtools/tsconfig.spec.json Outdated
Comment thread devtools/projects/shell-browser/src/app/tab_manager_spec.ts Outdated
@AleksanderBodurri
AleksanderBodurri force-pushed the fix-devtools-iframe-backend-ready-bug branch from 8cefec5 to d295ae0 Compare March 13, 2024 00:13
@angular-robot
angular-robot Bot requested a review from josephperrott March 13, 2024 00:13
dgp1130
dgp1130 reviewed Mar 13, 2024
View reviewed changes
Comment thread devtools/projects/shell-browser/src/app/tab_manager_spec.ts Outdated
Comment thread devtools/projects/shell-browser/src/app/tab_manager_spec.ts Outdated
Comment thread devtools/projects/shell-browser/src/app/tab_manager_spec.ts Outdated
Comment thread devtools/projects/shell-browser/src/app/tab_manager_spec.ts Outdated

@dgp1130 dgp1130 Mar 13, 2024

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.

Consider: We don't strictly need to do this for every test as long as we have reasonable confidence that we're correctly handling all the initialization possibilities. It might be possible to do this only for a subset of tests and still get the same confidence. Up to you which tests that is, or if we really need to do it for all of the tests.

@AleksanderBodurri AleksanderBodurri Mar 15, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought about this too 🤔 I think it's fine to run this for every test case in this file since we expect this part of the code base to be pretty stable. At worst it does little harm and at best it could catch non-obvious sequencing edge cases in the future. By having it run for all (or most) of the cases in this file we're also conveying that the output state of TabManager should be identical regardless of the ordering of events that it processes

@AleksanderBodurri
AleksanderBodurri force-pushed the fix-devtools-iframe-backend-ready-bug branch 3 times, most recently from abdd5d9 to 00dc64e Compare March 15, 2024 05:22
dgp1130
dgp1130 reviewed Mar 15, 2024
View reviewed changes
Comment thread devtools/projects/shell-browser/src/app/tab_manager.ts Outdated
Comment thread devtools/tsconfig.json Outdated
@AleksanderBodurri
AleksanderBodurri force-pushed the fix-devtools-iframe-backend-ready-bug branch 3 times, most recently from 21e729e to 3632924 Compare March 19, 2024 17:51
@AleksanderBodurri
fix(devtools): issue where backendReady race condition causes Angular…
38fa6fa 
… not detected error

Previously, a race condition could cause DevTools to enter a state where it can't detect an application on reload. This was caused by a sequencing issue between the content script connection, the devtools panel connection and an event "backendReady" that lets DevTools know when a particular frame is ready to be inspected.

This commit replaces the previously stored backendReady boolean with a promise, so that the devtools panel can eventually run a callback to connect to a content script when that content script emits it's backendReady message.
@AleksanderBodurri
AleksanderBodurri force-pushed the fix-devtools-iframe-backend-ready-bug branch from 3632924 to 38fa6fa Compare March 25, 2024 23:07
@ngbot ngbot Bot added this to the Backlog milestone Mar 26, 2024
@dgp1130 dgp1130 added the target: patch This PR is targeted for the next patch release label Mar 26, 2024
@dgp1130
dgp1130 removed the request for review from josephperrott March 26, 2024 16:08
@dgp1130 dgp1130 added the action: merge The PR is ready for merge by the caretaker label Mar 26, 2024
@dylhunn

dylhunn commented Mar 26, 2024

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit d15dca0.

dylhunn pushed a commit that referenced this pull request Mar 26, 2024
@AleksanderBodurri @dylhunn
Revert "Revert "refactor(devtools): implement iframe support for Angu…
821012d 
…lar DevTools' browser code (#53934)" (#54629)" (#54805)

This reverts commit dd9f9d7.

PR Close #54805
dylhunn pushed a commit that referenced this pull request Mar 26, 2024
@AleksanderBodurri @dylhunn
Revert "Revert "refactor(devtools): implement multiframe support in d…
cd00ec5 
…evtools page (#53934)" (#54629)" (#54805)

This reverts commit 133319e.

PR Close #54805
dylhunn pushed a commit that referenced this pull request Mar 26, 2024
@AleksanderBodurri @dylhunn
fix(devtools): issue where backendReady race condition causes Angular…
fa46844 
… not detected error (#54805)

Previously, a race condition could cause DevTools to enter a state where it can't detect an application on reload. This was caused by a sequencing issue between the content script connection, the devtools panel connection and an event "backendReady" that lets DevTools know when a particular frame is ready to be inspected.

This commit replaces the previously stored backendReady boolean with a promise, so that the devtools panel can eventually run a callback to connect to a content script when that content script emits it's backendReady message.

PR Close #54805
@dylhunn dylhunn closed this in 15b54ce Mar 26, 2024
dylhunn pushed a commit that referenced this pull request Mar 26, 2024
@AleksanderBodurri @dylhunn
Revert "Revert "refactor(devtools): implement multiframe support in d…
a3e6703 
…evtools page (#53934)" (#54629)" (#54805)

This reverts commit 133319e.

PR Close #54805
dylhunn pushed a commit that referenced this pull request Mar 26, 2024
@AleksanderBodurri @dylhunn
fix(devtools): issue where backendReady race condition causes Angular…
d15dca0 
… not detected error (#54805)

Previously, a race condition could cause DevTools to enter a state where it can't detect an application on reload. This was caused by a sequencing issue between the content script connection, the devtools panel connection and an event "backendReady" that lets DevTools know when a particular frame is ready to be inspected.

This commit replaces the previously stored backendReady boolean with a promise, so that the devtools panel can eventually run a callback to connect to a content script when that content script emits it's backendReady message.

PR Close #54805
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Apr 26, 2024

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@josephperrott josephperrott josephperrott approved these changes
@dgp1130 dgp1130 dgp1130 approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: devtools target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@AleksanderBodurri @dylhunn @josephperrott @dgp1130