-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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 devtools iframe backend ready bug #54805
Fix devtools iframe backend ready bug #54805
Conversation
…lar DevTools' browser code (angular#53934)" (angular#54629)" This reverts commit dd9f9d7.
…evtools page (angular#53934)" (angular#54629)" This reverts commit 133319e.
ed9f9a2
to
96a3298
Compare
d7ab4e6
to
02471b7
Compare
There was a problem hiding this comment.
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
02471b7
to
8cefec5
Compare
8cefec5
to
d295ae0
Compare
topLevelContentScriptPort, | ||
childContentScriptPort, | ||
devtoolsPort, | ||
} of eachPermutationOfDevToolsInitializing()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
abdd5d9
to
00dc64e
Compare
21e729e
to
3632924
Compare
… 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.
3632924
to
38fa6fa
Compare
This PR was merged into the repository by commit 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
… 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
…lar DevTools' browser code (angular#53934)" (angular#54629)" (angular#54805) This reverts commit dd9f9d7. PR Close angular#54805
…evtools page (angular#53934)" (angular#54629)" (angular#54805) This reverts commit 133319e. PR Close angular#54805
… not detected error (angular#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 angular#54805
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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