Get TaskAttribution to work with MessagePort's postMessage. [chromium/src : main]
Yoav Weiss (Gerrit)
Attention is currently required from: Scott Haseley, Yoav Weiss, Yuki Shiino.
Patch set 1:Commit-Queue +1
1 comment:
Patchset:
Hey yukishiino@ and shaseley@!
This is an initial draft of a solution for https://bugs.chromium.org/p/chromium/issues/detail?id=1342012 A couple of points that are missing here:
- I'm not sure if/how we can make nullable uints in mojom, so the current solution is using a sentinel 0 as an indication that the task has no parent. That's not great. Ideally we'd solve this with a nullable, otherwise I'll make sure that no task can have a 0 ID..
- We sometimes know the sender's agent cluster's ID at the receiver side, but only when the message is locked to the same agent cluster. Here we need slightly different semantics: the message is not locked, but we still want to know the agent cluster ID. I left a comment RE a couple of possible solutions for that.
I'd love your thoughts on this solution. Assuming they are favorable, I'll also tackle the remaining parts (agent cluster limitation, window.postMessage, BroadcastChannel) in followups.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Scott Haseley, Yuki Shiino.
File third_party/blink/public/common/DEPS:
Patch Set #1, Line 26:
"+third_party/blink/renderer/platform/scheduler/public",Might be cleaner to just move task_id.h to blink/public/common
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Scott Haseley, Yuki Shiino.
1 comment:
File third_party/blink/public/common/DEPS:
Patch Set #1, Line 26:
"+third_party/blink/renderer/platform/scheduler/public",
Might be cleaner to just move task_id. […]
Done
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Michal Mocny (Gerrit)
Attention is currently required from: Scott Haseley, Yoav Weiss, Yuki Shiino.
2 comments:
Patchset:
I am not familiar enough with these concepts to review thoroughly. I took a first pass read-through anyway, and it makes sense to me!
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 314:
std::unique_ptr<scheduler::TaskAttributionTracker::TaskScope>This unqiue_ptr doesn't seem to actually be used here. I do see that CreateTaskScore() has side effects, but is it important that the destructor not be called until this function unwinds?
If it is important, would a comment be useful here? If it is not important, could we save the variable and just leave the return value of of CreateTaskScore unused?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Scott Haseley, Yoav Weiss.
3 comments:
Patchset:
Overall, looks good to me.
File third_party/blink/public/mojom/messaging/transferable_message.mojom:
Patch Set #3, Line 40:
// TODO(yoav): Should this be nullable? If so, how?I think the following makes sense and it's extensible (which I think follows the spirit of mojo / protobuf).
```
struct TaskAttribution {
uint32 id;
};struct TransferrableMessage {
TaskAttribution? task_attribution;
};
```
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 326:
DOMWrapperWorld::MainWorld()Why can we assume it's the main world? It's possible to be an isolated world, isn't it?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yuki Shiino.
2 comments:
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 314:
std::unique_ptr<scheduler::TaskAttributionTracker::TaskScope>This unqiue_ptr doesn't seem to actually be used here. […]
It's important to ensure the task scope remains valid for the function's scope (and up until the event was sync dispatched). I'll add a comment.
Patch Set #3, Line 326:
DOMWrapperWorld::MainWorld()Why can we assume it's the main world? It's possible to be an isolated world, isn't it?
Yeah, I think so. I'm failing to find a way to properly check if we're in main world, though. `context->GetCurrentWorld()` is returning a nullptr and I'm not finding a way around it..
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yoav Weiss, Yuki Shiino.
1 comment:
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 314:
std::unique_ptr<scheduler::TaskAttributionTracker::TaskScope>
It's important to ensure the task scope remains valid for the function's scope (and up until the eve […]
Done
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yoav Weiss.
1 comment:
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 326:
DOMWrapperWorld::MainWorld()Yeah, I think so. I'm failing to find a way to properly check if we're in main world, though. […]
Replied offline.
I started thinking that Task Attribution shouldn't be associated with any v8::Context in the first place. Since the purpose of Task Attribution is to track down an execution sequence, it should be associated with v8::Isolate instead of v8::Context, because v8::Isolate is an unit of execution sequence.
Just for example, v8::TryCatch is associated with v8::Isolate because exception handling is about execution sequence.
I think the best solution (for a long term) is to move the implementation of Task Attribution from v8::Context to v8::Isolate.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yuki Shiino.
Patch set 10:Commit-Queue +1
2 comments:
File third_party/blink/public/mojom/messaging/transferable_message.mojom:
Patch Set #3, Line 40:
// TODO(yoav): Should this be nullable? If so, how?
FYI, […]
Done
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 326:
DOMWrapperWorld::MainWorld()
Replied offline. […]
This makes sense to me, and tbh, would simplify some of the use cases (where an isolate is present but a ScriptState is not). I'm assuming this should be in a followup, and not block this specifically, right?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yuki Shiino.
1 comment:
File third_party/blink/common/messaging/transferable_message_mojom_traits.cc:
Patch Set #11, Line 31:
out->delegated_capability = data.delegated_capability();Here I'm not doing anything in particular for parent_task_id, as I'm assuming that it's handled by its own traits. Let me know if I'm wrong.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yuki Shiino.
1 comment:
File third_party/blink/common/messaging/transferable_message_mojom_traits.cc:
Patch Set #11, Line 10:
#include "third_party/blink/public/mojom/messaging/transferable_message.mojom.h"This is spurious, but leaving it here for now just to keep this file in the diff (to address the comment below)
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yoav Weiss.
Patch set 11:Code-Review +1
4 comments:
Patchset:
LGTM with comments.
File third_party/blink/public/common/messaging/transferable_message_mojom_traits.h:
Patch Set #11, Line 61:
input.parent_task_id.value()))absl::make_optional?
File third_party/blink/renderer/core/messaging/blink_transferable_message_mojom_traits.h:
Patch Set #11, Line 73:
input.parent_task_id.value()))Ditto. absl::make_optinoal?
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 326:
DOMWrapperWorld::MainWorld()
This makes sense to me, and tbh, would simplify some of the use cases (where an isolate is present b […]
I think we'd need the followings at least to land this patch.
1. File a bug and add a TODO(crbug.com/xxx) comment to the bug. It's very good to have a fix plan for a long term in the bug.
2. Explain why this is acceptable as a short-term fix.Especially,
2-1. Task Attribution uses the ScriptState (hence the v8::Context) only in order to store the tracking id in the v8::Context, and
2-2. The ScriptState (hence the v8::Context) is never accessible from author scripts.Plus, we'd better to explain possible side effect (disclaimer).
3. It's possible that, when used with isolated worlds, cross talk among the main world and isolated worlds may be possible. As a result, the tracking ID can be wrong in such a case. However, Task Attribution is used as FYI for web developers and possible cross talk (wrong tracking ID) won't be a security issue nor severe issue.I think it's important to note/comment that a) it's wrong to assume the main world, and b) this wrong assumption causes only minor issues.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yoav Weiss.
5 comments:
File third_party/blink/common/messaging/task_attribution_id_mojom_traits.cc:
Patch Set #11, Line 11:
TaskAttributionIdDataViewnit: Ditto.
Patch Set #11, Line 13:
TaskAttributionIdDataViewnit: Ditto.
File third_party/blink/common/messaging/transferable_message_mojom_traits.cc:
Patch Set #11, Line 31:
out->delegated_capability = data.delegated_capability();Here I'm not doing anything in particular for parent_task_id, as I'm assuming that it's handled by i […]
I believe that you need to handle it for yourself.
File third_party/blink/public/common/messaging/task_attribution_id_mojom_traits.h:
Patch Set #11, Line 15:
TaskAttributionIdDataViewnit: TaskAttributionId::DataView would be preferable, I guess.
Patch Set #11, Line 21:
TaskAttributionIdDataViewnit: Ditto.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yuki Shiino.
3 comments:
File third_party/blink/public/common/messaging/transferable_message_mojom_traits.h:
Patch Set #11, Line 61:
input.parent_task_id.value()))
absl::make_optional? […]
Done
File third_party/blink/renderer/core/messaging/blink_transferable_message_mojom_traits.h:
Patch Set #11, Line 73:
input.parent_task_id.value()))
Ditto. […]
Done
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #3, Line 326:
DOMWrapperWorld::MainWorld()
I think we'd need the followings at least to land this patch. […]
Done
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yoav Weiss, Yuki Shiino.
Patch set 12:Commit-Queue +1
3 comments:
File third_party/blink/common/messaging/task_attribution_id_mojom_traits.cc:
Patch Set #11, Line 11:
TaskAttributionIdDataViewnit: Ditto.
Done
Patch Set #11, Line 13:
TaskAttributionIdDataViewnit: Ditto.
Done
File third_party/blink/public/common/messaging/task_attribution_id_mojom_traits.h:
Patch Set #11, Line 15:
TaskAttributionIdDataViewnit: TaskAttributionId::DataView would be preferable, I guess.
While I'm successfully using TaskAttributionId::DataView in the cc file, I'm failing to get this to build with it here.. I'd appreciate some advice 😊
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yuki Shiino.
1 comment:
File third_party/blink/public/common/messaging/task_attribution_id_mojom_traits.h:
Patch Set #11, Line 15:
TaskAttributionIdDataView
While I'm successfully using TaskAttributionId::DataView in the cc file, I'm failing to get this to […]
NM, I think I'm hitting some circular dependency where this relies on task_attribution_id.mojom.h and it relies on this h file. I'll figure out a way out of it..
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley.
1 comment:
File third_party/blink/public/common/messaging/task_attribution_id_mojom_traits.h:
Patch Set #11, Line 15:
TaskAttributionIdDataViewWhile I'm successfully using TaskAttributionId::DataView in the cc file, I'm failing to get this to […]
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h;drc=8b7f0ea51b5ccca6d33939bb0662155ba259243c;l=19
uses T::DataView, and it's defined at:
https://source.chromium.org/chromium/chromium/src/+/main:out/Debug/gen/third_party/blink/public/mojom/messaging/cloneable_message.mojom.h;drc=aeadb03c8553c39e88d5d11d10f706d42f06a1d7;l=63
so, third_party/blink/public/mojom/messaging/cloneable_message.mojom.h must be included.Your case should be the same, I think. And it looks that third_party/blink/public/mojom/messaging/task_attribution_id.mojom.h is included in PS13?
What error did you observe? Maybe because you're missing BLINK_COMMON_EXPORT?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Kouhei Ueno (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yoav Weiss.
Patch set 13:Code-Review +1
1 comment:
File third_party/blink/public/common/scheduler/task_attribution_id.h:
Patch Set #13, Line 38:
TaskAttributionIdType value_;Optional nit: = {0};
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Michal Mocny, Scott Haseley, Yuki Shiino.
2 comments:
File third_party/blink/public/common/messaging/task_attribution_id_mojom_traits.h:
Patch Set #11, Line 15:
TaskAttributionIdDataView
https://source.chromium. […]
This was related to the fact that task_attribution_id_mojom_traits.h was dependent on task_attribution_id_mojom.h and vice versa, create a circular dependency (that was broken by the #defines at the beginning of the h files, but ended up not defining TaskAttributionId::DataView). Now it's all good 😊
Patch Set #11, Line 21:
TaskAttributionIdDataViewnit: Ditto.
Done
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Kouhei Ueno, Michal Mocny, Scott Haseley, Yuki Shiino.
Patch set 14:Commit-Queue +1
2 comments:
File third_party/blink/common/messaging/transferable_message_mojom_traits.cc:
Patch Set #11, Line 31:
out->delegated_capability = data.delegated_capability();
I believe that you need to handle it for yourself.
Thanks! Handled 😊
File third_party/blink/public/common/scheduler/task_attribution_id.h:
Patch Set #13, Line 38:
TaskAttributionIdType value_;Optional nit: = {0};
Done! Also simplified the added constructor as a result.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Kouhei Ueno, Michal Mocny, Scott Haseley, Yuki Shiino.
1 comment:
File third_party/blink/common/messaging/transferable_message_mojom_traits.cc:
Patch Set #11, Line 10:
#include "third_party/blink/public/mojom/messaging/transferable_message.mojom.h"This is spurious, but leaving it here for now just to keep this file in the diff (to address the com […]
Resolving
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Mike West, Scott Haseley.
1 comment:
Patchset:
Hey Mike! Can you take a look at the mojo parts of this CL? (which are admittedly, a lot of the CL..)
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Dave Tapuska (Gerrit)
Attention is currently required from: Mike West, Scott Haseley, Yoav Weiss.
1 comment:
File third_party/blink/public/mojom/messaging/transferable_message.mojom:
Patch Set #15, Line 41:
TaskAttributionId? parent_task_id;Won't this just cause confusion? The TaskAttributionId is scoped per renderer process so passing it across a mojom boundary seems confusing because when it hits another renderer it would be referencing a completely different task id.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley.
1 comment:
File third_party/blink/public/mojom/messaging/transferable_message.mojom:
Patch Set #15, Line 41:
TaskAttributionId? parent_task_id;
Won't this just cause confusion? The TaskAttributionId is scoped per renderer process so passing it […]
This IPC is used both between different renderers as well as on the same renderer. Frameworks are postMessaging themselves in order to schedule tasks, which is the case I'm trying to tackle here.
There's a TODO(crbug.com/1351643) in message_port.cc that is aimed to tackle this exact problem, and filter such task IDs if they are coming from a different agent cluster.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Attention is currently required from: Dave Tapuska, Scott Haseley.
1 comment:
Patchset:
I agree with dtapuska@; it's not clear to me how this would work across an origin boundary. The test included in the CL only exercises code called in the same execution context. It would be ideal to take a look at workers, frames, and popups as well.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley.
2 comments:
Patchset:
I agree with dtapuska@; it's not clear to me how this would work across an origin boundary. […]
I'm planning to cover that as part of crbug.com/1351641
Would you consider this blocking?
File third_party/blink/public/mojom/messaging/transferable_message.mojom:
Patch Set #15, Line 41:
TaskAttributionId? parent_task_id;
This IPC is used both between different renderers as well as on the same renderer. […]
Apologies, that's crbug.com/1351641
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley.
2 comments:
Patchset:
I'm planning to cover that as part of crbug.com/1351641 […]
Added filtering for messages from other agent clusters (plus iframe tests to verify it actually works).
Patchset:
mkwst@ - can you take another look?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley, Yoav Weiss.
Patch set 17:Code-Review +1
1 comment:
Patchset:
non-owner LGTM.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Dave Tapuska (Gerrit)
Attention is currently required from: Mike West, Scott Haseley, Yoav Weiss.
2 comments:
Patchset:
Would it be possible to clobber the parent_task_id either in the browser RenderFrameHostImpl::PostMessageEvent (so that we don't leak the task ID to another renderer) or in LocalFrame::PostMessageEvent where the mojo side of that is handled in the renderer? (I personally prefer the browser side).
Other than that this seems ok to me.
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #17, Line 135:
if (tracker && script_state->World().IsMainWorld()) {I feel a comment here why we don't do this for isolated worlds might be necessary.]
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley.
1 comment:
Patchset:
Would it be possible to clobber the parent_task_id either in the browser RenderFrameHostImpl::PostMe […]
I'm not sure I understand. Do you want me to run the agent cluster check in RenderFrameHostImpl::PostMessageEvent? Is there some way to get the destination agent cluster there?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Dave Tapuska (Gerrit)
Attention is currently required from: Mike West, Scott Haseley, Yoav Weiss.
1 comment:
Patchset:
I'm not sure I understand. […]
I was thinking:
RenderFrameHostImpl::PostMessageEvent(... blink::TransferrableMessage message) {
message.parent_task_id = absl::nullopt;
}
So we don't leak the parent_task_id from one renderer to another so it couldn't possibly be miused.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley, Yuki Shiino.
1 comment:
Patchset:
I was thinking: […]
OK, done!
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley, Yuki Shiino.
Patch set 18:Commit-Queue +1
1 comment:
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #17, Line 135:
if (tracker && script_state->World().IsMainWorld()) {
I feel a comment here why we don't do this for isolated worlds might be necessary. […]
Done
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Attention is currently required from: Dave Tapuska, Scott Haseley, Yoav Weiss, Yuki Shiino.
2 comments:
Patchset:
I think it's reasonable to try this approach. One question below:
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #19, Line 330:
// attributing continuations to that original task.Could you grab the world through `ExecutionContext::GetCurrentWorld()`?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Scott Haseley, Yuki Shiino.
1 comment:
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #19, Line 330:
// attributing continuations to that original task.Could you grab the world through `ExecutionContext::GetCurrentWorld()`?
Seems like `context->GetCurrentWorld()` returns a null refptr here, at least in some critical cases.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Attention is currently required from: Dave Tapuska, Scott Haseley, Yoav Weiss, Yuki Shiino.
Patch set 19:Code-Review +1
1 comment:
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #19, Line 330:
// attributing continuations to that original task.
Seems like `context->GetCurrentWorld()` returns a null refptr here, at least in some critical cases.
Can you add a comment with some detail?
Otherwise LGTM.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Arthur Sonzogni, Dave Tapuska, Scott Haseley, Yuki Shiino.
2 comments:
Patchset:
Thanks for reviewing!!
arthursonzogni@ - Can you take a look at render_frame_host_impl.cc ?
File third_party/blink/renderer/core/messaging/message_port.cc:
Patch Set #19, Line 330:
// attributing continuations to that original task.
Can you add a comment with some detail? […]
Done
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Dave Tapuska, Yoav Weiss.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.cc:
// This is always called from either another renderer (through RemoteFrame) or
// from the embedder itself. As such, we nullify the parent task ID here, to
// prevent this information from leaking between renderers.
message.parent_task_id = absl::nullopt;Does it exists one case where we need to exchange this data in between the renderer(s) and the browsers?
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Arthur Sonzogni, Dave Tapuska.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.cc:
// This is always called from either another renderer (through RemoteFrame) or
// from the embedder itself. As such, we nullify the parent task ID here, to
// prevent this information from leaking between renderers.
message.parent_task_id = absl::nullopt;
Does it exists one case where we need to exchange this data in between the renderer(s) and the brows […]
The data is (at least currently) only relevant inside of a single renderer process, so at the moment I don't believe there is such a case
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Dave Tapuska, Yoav Weiss.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.cc:
// This is always called from either another renderer (through RemoteFrame) or
// from the embedder itself. As such, we nullify the parent task ID here, to
// prevent this information from leaking between renderers.
message.parent_task_id = absl::nullopt;
The data is (at least currently) only relevant inside of a single renderer process, so at the moment […]
If this is not needed. Would it be possible not to send it via mojo?
Otherwise, I would have to check every code sending blink::TransferableMessage from the browser process to verify if we should apply L12608 in a similar way:
https://source.chromium.org/search?q=blink::TransferableMessage%20file:content%2Fbrowser&sq=&ss=chromium
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Attention is currently required from: Arthur Sonzogni, Dave Tapuska, Mike West.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.cc:
// This is always called from either another renderer (through RemoteFrame) or
// from the embedder itself. As such, we nullify the parent task ID here, to
// prevent this information from leaking between renderers.
message.parent_task_id = absl::nullopt;
If this is not needed. Would it be possible not to send it via mojo? […]
I nullified the various places where we don't care about the parent task ID: https://chromium-review.googlesource.com/c/chromium/src/+/3815445/20..21
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Dave Tapuska, Mike West, Yoav Weiss.
Patch set 21:Code-Review +1
1 comment:
Patchset:
content/ LGTM.
To make this somehow "future proof", it would have been nice to never send this attribute toward a different process. One possibility would have been not make it null during deserialization when the process is the browser process. Unfortunately:
- There are no non hackish easy way to check what is the current process.
- It is not clear to me how useful leaking this attribute is for an attacker.
So I guess, the current patch is good enough ;-)
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Attention is currently required from: Dave Tapuska, Yoav Weiss.
Patchset:
LGTM. Again.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Dave Tapuska (Gerrit)
Attention is currently required from: Yoav Weiss.
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Yoav Weiss (Gerrit)
Patch set 21:Commit-Queue +2
1 comment:
Patchset:
Thanks all for reviewing!! 😊
To view, visit change 3815445. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Get TaskAttribution to work with MessagePort's postMessage.
TaskAttribution is not currently taking postMessage into account.
This CL addresses a part of that, where tasks that are being triggered
by a postMessage on a MessagePort, are being traced back to the
triggering task as their ancestor.
Bug: 1342012
Change-Id: Ib1efab2e6ac3868a6d14b88f602356f0673ebbda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3815445
Reviewed-by: Mike West <mk...@chromium.org>
Commit-Queue: Yoav Weiss <yoav...@chromium.org>
Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>
Reviewed-by: Dave Tapuska <dtap...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1050668}
---
M content/browser/android/app_web_message_port.cc
M content/browser/renderer_host/render_frame_host_impl.cc
M content/browser/service_worker/service_worker_context_wrapper.cc
M content/browser/service_worker/service_worker_object_host.cc
M content/browser/service_worker/service_worker_version.cc
M third_party/blink/common/BUILD.gn
A third_party/blink/common/messaging/task_attribution_id_mojom_traits.cc
M third_party/blink/common/messaging/transferable_message_mojom_traits.cc
A third_party/blink/public/common/messaging/task_attribution_id_mojom_traits.h
M third_party/blink/public/common/messaging/transferable_message.h
M third_party/blink/public/common/messaging/transferable_message_mojom_traits.h
M third_party/blink/public/common/scheduler/task_attribution_id.h
M third_party/blink/public/mojom/BUILD.gn
A third_party/blink/public/mojom/messaging/task_attribution_id.mojom
M third_party/blink/public/mojom/messaging/transferable_message.mojom
M third_party/blink/renderer/core/messaging/blink_transferable_message.cc
M third_party/blink/renderer/core/messaging/blink_transferable_message.h
M third_party/blink/renderer/core/messaging/blink_transferable_message_mojom_traits.h
M third_party/blink/renderer/core/messaging/message_port.cc
A third_party/blink/web_tests/wpt_internal/task-tracking/messageport-postmessage-same-origin-iframe.html
A third_party/blink/web_tests/wpt_internal/task-tracking/resources/messageport-postmessage-iframe.html
A third_party/blink/web_tests/wpt_internal/task-tracking/resources/messageport-postmessage-iframe.html.headers
A third_party/blink/web_tests/wpt_internal/task-tracking/track-messageport-postmessage-cross-origin-iframe.html
A third_party/blink/web_tests/wpt_internal/task-tracking/track-postmessage.html
24 files changed, 310 insertions(+), 5 deletions(-)