Pass FrameToken from MainThreadDebugger::consoleAPIMessage down to WebContentsDelegate::DidAddMessageToConsole and WebContentsObserver::OnDidAddMessageToConsole. [chromium/src : main]
Tomasz Malinowski (Gerrit)
Tomasz Malinowski added 1 comment![]( "Open in Gerrit")
This is proposal to pass information about frame which send log (FrameToken) from MainThreadDebugger::consoleAPIMessage down to WebContentsDelegate::DidAddMessageToConsole and WebContentsObserver::OnDidAddMessageToConsole.
It is result of change in V8:
V8 changes - https://chromium-review.googlesource.com/c/v8/v8/+/6811579
V8 ticket - https://issues.chromium.org/u/1/issues/437026560
Related details
- Alex Moshchuk
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 2 comments![]( "Open in Gerrit")
Thanks for the CL. I'm curious to hear alexrudenko@'s thoughts on whether this is needed from a devtools perspective, and in the meantime a couple of comments from a content owner perspective.
const std::optional<std::u16string>& source_frame_token) {}How is this going to be used outside of //content? I don't see any uses of this, only the plumbing to make it available, but this goes against the guidelines for introducing content/public APIs, where each addition must be accompanied by code that uses it outside of //content (https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md).
The other concern I have with this is that the `source_frame_token` seems to come from the (possibly compromised) renderer, so we need to be careful with how it's going to be used, and possibly validate that it corresponds to a valid frame prior to plumbing it for use outside //content. What kind of token is this? For example, LocalFrameTokens can't be resolved to frames without a valid process ID (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=358-365;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f).
Related details
- Alex Rudenko
- Tomasz Malinowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
@szu...@chromium.org could you please review? I saw you reviewed corresponding v8 changes.
frame->Console().ReportMessageToClient(is not `frame` already referring to the source frame?
Related details
- Simon Zünd
- Tomasz Malinowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd added 2 comments![]( "Open in Gerrit")
The V8 change in and of itself was fine. Whether or not it's ok to pass up the frame token through all layers of abstraction (also w.r.t. to security) I'm not able to judge so I'll defer to Chromium owners here.
One thing to note thats not clear from the CL description: IIUC the frame token will not actually be used anywhere in Chrome/Chromium, but it's a third party browser that implements a custom `WebContentsDelegate`.
frame->Console().ReportMessageToClient(is not `frame` already referring to the source frame?
For same origin iframes, `frame` and `local_frame` will differ.
Related details
- Alex Rudenko
- Tomasz Malinowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
The V8 change in and of itself was fine. Whether or not it's ok to pass up the frame token through all layers of abstraction (also w.r.t. to security) I'm not able to judge so I'll defer to Chromium owners here.
One thing to note thats not clear from the CL description: IIUC the frame token will not actually be used anywhere in Chrome/Chromium, but it's a third party browser that implements a custom `WebContentsDelegate`.
If this is the case, this unfortunately goes against our guidelines for accepting this code change: see https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#code-guidelines under "Code in the Chromium project should be in service of other code in the Chromium project."
In similar cases in the past, we only proceeded with a change like this if we also found a valid use case in the codebase which meaningfully benefits Chromium.
Tomasz Malinowski (Gerrit)
Tomasz Malinowski added 1 comment![]( "Open in Gerrit")
const std::optional<std::u16string>& source_frame_token) {}How is this going to be used outside of //content? I don't see any uses of this, only the plumbing to make it available, but this goes against the guidelines for introducing content/public APIs, where each addition must be accompanied by code that uses it outside of //content (https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md).
The other concern I have with this is that the `source_frame_token` seems to come from the (possibly compromised) renderer, so we need to be careful with how it's going to be used, and possibly validate that it corresponds to a valid frame prior to plumbing it for use outside //content. What kind of token is this? For example, LocalFrameTokens can't be resolved to frames without a valid process ID (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=358-365;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f).
Q: How is this going to be used outside of //content?
A: It will be used to get proper RenderFrameHost and get more info about frame which send this log, e.g.: Frame name.
Q: What kind of token is this? For example, LocalFrameTokens can't be resolved to frames without a valid process ID.
A: Yes, you have right, That is why I plan to replace const std::optional<std::u16string>& source_frame_token to const std::optional<content::GlobalRenderFrameHostToken>& source_frame_global_token. This will resolve problem of finding correct frame on browser process side.
My questions:
About this policy: https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md
If I will not go outside //content , then it will be accepted to have it integrated in to chromium ? I was thinking to pass source_frame_global_token only down to RenderFrameHostDelegate.
Also I was inspired by const std::optional<std::u16string>& untrusted_stack_trace which is passed everywhere but used only it two places and mostly only in dev logs.
Related details
- Alex Moshchuk
Tomasz Malinowski (Gerrit)
Tomasz Malinowski added 1 comment![]( "Open in Gerrit")
const std::optional<std::u16string>& source_frame_token) {}Tomasz MalinowskiHow is this going to be used outside of //content? I don't see any uses of this, only the plumbing to make it available, but this goes against the guidelines for introducing content/public APIs, where each addition must be accompanied by code that uses it outside of //content (https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md).
The other concern I have with this is that the `source_frame_token` seems to come from the (possibly compromised) renderer, so we need to be careful with how it's going to be used, and possibly validate that it corresponds to a valid frame prior to plumbing it for use outside //content. What kind of token is this? For example, LocalFrameTokens can't be resolved to frames without a valid process ID (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=358-365;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f).
Q: How is this going to be used outside of //content?
A: It will be used to get proper RenderFrameHost and get more info about frame which send this log, e.g.: Frame name.Q: What kind of token is this? For example, LocalFrameTokens can't be resolved to frames without a valid process ID.
A: Yes, you have right, That is why I plan to replace const std::optional<std::u16string>& source_frame_token to const std::optional<content::GlobalRenderFrameHostToken>& source_frame_global_token. This will resolve problem of finding correct frame on browser process side.My questions:
About this policy: https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md
If I will not go outside //content , then it will be accepted to have it integrated in to chromium ? I was thinking to pass source_frame_global_token only down to RenderFrameHostDelegate.
Also I was inspired by const std::optional<std::u16string>& untrusted_stack_trace which is passed everywhere but used only it two places and mostly only in dev logs.
@ale...@chromium.org any thoughts about above comment ?
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
const std::optional<std::u16string>& source_frame_token) {}Tomasz MalinowskiHow is this going to be used outside of //content? I don't see any uses of this, only the plumbing to make it available, but this goes against the guidelines for introducing content/public APIs, where each addition must be accompanied by code that uses it outside of //content (https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md).
The other concern I have with this is that the `source_frame_token` seems to come from the (possibly compromised) renderer, so we need to be careful with how it's going to be used, and possibly validate that it corresponds to a valid frame prior to plumbing it for use outside //content. What kind of token is this? For example, LocalFrameTokens can't be resolved to frames without a valid process ID (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=358-365;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f).
Tomasz MalinowskiQ: How is this going to be used outside of //content?
A: It will be used to get proper RenderFrameHost and get more info about frame which send this log, e.g.: Frame name.Q: What kind of token is this? For example, LocalFrameTokens can't be resolved to frames without a valid process ID.
A: Yes, you have right, That is why I plan to replace const std::optional<std::u16string>& source_frame_token to const std::optional<content::GlobalRenderFrameHostToken>& source_frame_global_token. This will resolve problem of finding correct frame on browser process side.My questions:
About this policy: https://chromium.googlesource.com/chromium/src/+/HEAD/content/public/README.md
If I will not go outside //content , then it will be accepted to have it integrated in to chromium ? I was thinking to pass source_frame_global_token only down to RenderFrameHostDelegate.
Also I was inspired by const std::optional<std::u16string>& untrusted_stack_trace which is passed everywhere but used only it two places and mostly only in dev logs.
@ale...@chromium.org any thoughts about above comment ?
If I will not go outside //content , then it will be accepted to have it integrated in to chromium ? I was thinking to pass source_frame_global_token only down to RenderFrameHostDelegate.
In order for it to be accepted, I think it would still need to be used for an improvement in Chromium itself, according to the guidance I mentioned in https://chromium-review.googlesource.com/c/chromium/src/+/7019174/comments/cd8d3807_c36ca160. For example, the `untrusted_stack_trace` seems to [improve](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/devtools_frontend_host_impl.cc;l=186;drc=c1b102f35ec290df46da5093c2ab90d6616134c6) JS error reporting in DevTools (though I'm not familiar with its exact usage and defer to DevTools owners there). I don't think we can accept a plumbing-only patch without a concrete followup CL lined up that uses the new token. If those uses are going to be within content, then the plumbing would also need to stay within content.
Q: How is this going to be used outside of //content?
A: It will be used to get proper RenderFrameHost and get more info about frame which send this log, e.g.: Frame name.
I think this is still too vague - I think we'd want an implementation of a feature/improvement where this new info would be used in Chromium.
Also, as a separate issue, I wonder if the source frame might be gone by the time the token gets used, and if it would be more robust to capture things like the frame name at the time the message is generated.
Related details
- Tomasz Malinowski