fix(devtools): Enhance message bus with event tagging and spam protec… #62645
Conversation
8e83db9 to
20fa9d5
Compare
20fa9d5 to
b55f820
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two important defensive mechanisms for the message bus: event tagging to filter out irrelevant messages, and a spam protection mechanism to prevent the devtools from being overwhelmed by high-frequency event streams. The event tagging is implemented correctly across the different message bus types.
My main feedback is on the spam protection logic in content-script.ts. The current implementation has a delay in detection which could still allow a burst of messages to impact the extension. I've provided a suggestion for a more immediate protection strategy. Overall, this is a valuable improvement for the stability of the devtools.
b55f820 to
54b55af
Compare
…tion One common problem encountered by the devtools content script is that it accepted almost any message send over the message bus. Some websites like `auth.openai.com` were spamming the bus and DDOS the devtools app. This commit introduces 2 defensives strategies : * event tagging : Only Event sent by the devtools app will make it to its backend * Bailout out when the event throughput is too high (we're getting spammed). fixes angular#62471 angular#62450 angular#55854
54b55af to
e060593
Compare
There was a problem hiding this comment.
So happy to see this issue solved. Thanks for this @JeanMeche 🙏
|
|
||
| // This is where we listen for messages sent from the Angular DevTools extension to the content script. | ||
| // We have some logic to handle spam protection and event tagging. | ||
| fromEvent<MessageEvent>(window, 'message') |
There was a problem hiding this comment.
Consider: Move the spam protection piece to a different function which invokes a callback and then pass through proxyEventFromWindowToDevToolsExtension to decouple the two pieces.
|
|
||
| // This is where we listen for messages sent from the Angular DevTools extension to the content script. | ||
| // We have some logic to handle spam protection and event tagging. | ||
| fromEvent<MessageEvent>(window, 'message') |
There was a problem hiding this comment.
Question: How necessary is the "spam prevention" part of this?
If the page is emitting a bunch of messages and we ignore them, then that feels WAI from a correctness perspective and should be relatively lightweight from a performance perspective. I'm surprised ignoring messages would have a significant enough performance cost to be observable to users. If they're sending so many messages that the act of ignoring them is a performance problem, then that sounds like either a browser issue or the amount of messages in flight should dominate the performance profile and Angular's contribution of ignoring the content should be relatively insignificant.
Is the issue that we're unnecessarily proxying user-messages to DevTools via a "heavyweight" API like chrome.runtime.sendMessage? If so, would just updating from if (event.source === window && event.data) to if (event.source === window && event.data && event.data.__NG_DEVTOOLS_EVENT__) have the same impact for a lot less effort? Do we really need to unsubscribe from the handler in the case it's too noisy?
…tion
One common problem encountered by the devtools content script is that it accepted almost any message send over the message bus. Some websites like
auth.openai.comwere spamming the bus and DDOS the devtools app.This commit introduces 2 defensives strategies :
fixes #62471 #62450 #55854