[event-timing] Simplify Event Timing entry tracking during RAII stack [chromium/src : main]
Michal Mocny (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 4 comments![]( "Open in Gerrit")
I'll come to my computer later tonight to read the relevant portion of window_performance.cc quite a bit more carefully to understand this in detail, but figured I'd scribble what I have now. :-)
// event_ might potentially be null if this is std::move()-ed.This pre-existing comment feels a little scary, is this an indication that stuff gets processed out of the expected RAII order? Could it be that the new, more efficient code is more reliant on the order than the original (the search in the other file that you're getting rid of, basically)? (speculating / ignore as you see fit please, I think I don't understand it well enough yet to really tell)
Maybe a check of sorts to guard against underflow in the next line?
E.g. CHECK(entry_nesting_level_ > 0);
DomWindow()->GetFrame()->GetChromeClient().NotifyPresentationTime(This is still in the same method, but I think you removed the check from l. 651 (base version) so it could crash? Or is checking for |entry| sufficient to prevent a null dereference?
Related details
- Michal Mocny
- Scott Haseley
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. |
Scott Haseley (Gerrit)
Scott Haseley added 3 comments![]( "Open in Gerrit")
Mostly LG. This is a lot cleaner.
Persistent<PerformanceEventTiming> entry_;This (and the others) really should be stored as raw pointers since this is stack allocated. I think for `event_` that would require clearing state on move() though.
if (!DomWindow() || !DomWindow()->GetFrame()) {What would happen if running the event handler detaches the window?
Related details
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +1 |
2 comments
// event_ might potentially be null if this is std::move()-ed.This pre-existing comment feels a little scary, is this an indication that stuff gets processed out of the expected RAII order? Could it be that the new, more efficient code is more reliant on the order than the original (the search in the other file that you're getting rid of, basically)? (speculating / ignore as you see fit please, I think I don't understand it well enough yet to really tell)
I dont actually think this ever does or could get moved. But I dont want to remove this without understanding the historical context.
DomWindow()->GetFrame()->GetChromeClient().NotifyPresentationTime(This is still in the same method, but I think you removed the check from l. 651 (base version) so it could crash? Or is checking for |entry| sufficient to prevent a null dereference?
I thought it was, but I will add it back to be extra careful.
In general I wouldn't think this could change in the middle of event dispatch (and still have the event finish dispatch), but I've been surprised before! (i.e. devtools debugging pauses or something)
Related details
- Johannes Henkel
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. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// event_ might potentially be null if this is std::move()-ed.This pre-existing comment feels a little scary, is this an indication that stuff gets processed out of the expected RAII order? Could it be that the new, more efficient code is more reliant on the order than the original (the search in the other file that you're getting rid of, basically)? (speculating / ignore as you see fit please, I think I don't understand it well enough yet to really tell)
No I think it's just a product of this being moveable (during creation). This is common for moveable RAII classes, and you'd have to make it non-moveable to avoid this. (btw it should be "this will be null if this has been moved", not potentially).
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
DomWindow()->GetFrame()->GetChromeClient().NotifyPresentationTime(This is still in the same method, but I think you removed the check from l. 651 (base version) so it could crash? Or is checking for |entry| sufficient to prevent a null dereference?
I thought it was, but I will add it back to be extra careful.
In general I wouldn't think this could change in the middle of event dispatch (and still have the event finish dispatch), but I've been surprised before! (i.e. devtools debugging pauses or something)
Along the lines of what I asked in the other comment, I'm pretty sure you can construct an example where an iframe causes itself to be removed in an event handler.
Related details
- Johannes Henkel
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
DomWindow()->GetFrame()->GetChromeClient().NotifyPresentationTime(Michal MocnyThis is still in the same method, but I think you removed the check from l. 651 (base version) so it could crash? Or is checking for |entry| sufficient to prevent a null dereference?
Scott HaseleyI thought it was, but I will add it back to be extra careful.
In general I wouldn't think this could change in the middle of event dispatch (and still have the event finish dispatch), but I've been surprised before! (i.e. devtools debugging pauses or something)
Along the lines of what I asked in the other comment, I'm pretty sure you can construct an example where an iframe causes itself to be removed in an event handler.
- Johannes Henkel
- Scott Haseley
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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
// event_ might potentially be null if this is std::move()-ed.This pre-existing comment feels a little scary, is this an indication that stuff gets processed out of the expected RAII order? Could it be that the new, more efficient code is more reliant on the order than the original (the search in the other file that you're getting rid of, basically)? (speculating / ignore as you see fit please, I think I don't understand it well enough yet to really tell)
No I think it's just a product of this being moveable (during creation). This is common for moveable RAII classes, and you'd have to make it non-moveable to avoid this. (btw it should be "this will be null if this has been moved", not potentially).
Interesting. So, by changing the header to
EventTiming(EventTiming&&) = delete;
EventTiming& operator=(EventTiming&&) = delete;
I confirmed that TryCreate does in fact cause something like std::move to happen, as the object is returned in the std::optional - it won't compile with the delete version above.
Below some speculation / theory:
I'm also thinking that maybe the destructor runs twice, once as the thingy that was stack-allocated in TryCreate gets deleted (the remnants of what was moved), and then in the caller (there are multiple), once the std::optional<EventTiming> goes out of scope.
And, the check in l. 125 below may accomplish to filter out the bogus constructor invocation, because the Persistent<Foo> is smart enough to clear out event_ from the stack-allocated instance?
On the usage of the Persistent<Foo> field: I have the feeling that it's actually correct, because the stack is a garbage collection root, and we don't want these objects to get garbage collected before the EventTiming instance gets deleted?
Perhaps we could do away with the TryCreate returning an std::optional<EventTiming> with the hidden move stuff, and instead have that logic in a more pedestrian instance method. E.g., void Init(...) with the same parameters as TryCreate?
Related details
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
// event_ might potentially be null if this is std::move()-ed.Scott HaseleyThis pre-existing comment feels a little scary, is this an indication that stuff gets processed out of the expected RAII order? Could it be that the new, more efficient code is more reliant on the order than the original (the search in the other file that you're getting rid of, basically)? (speculating / ignore as you see fit please, I think I don't understand it well enough yet to really tell)
Johannes HenkelNo I think it's just a product of this being moveable (during creation). This is common for moveable RAII classes, and you'd have to make it non-moveable to avoid this. (btw it should be "this will be null if this has been moved", not potentially).
Interesting. So, by changing the header to
EventTiming(EventTiming&&) = delete;
EventTiming& operator=(EventTiming&&) = delete;I confirmed that TryCreate does in fact cause something like std::move to happen, as the object is returned in the std::optional - it won't compile with the delete version above.
Below some speculation / theory:
I'm also thinking that maybe the destructor runs twice, once as the thingy that was stack-allocated in TryCreate gets deleted (the remnants of what was moved), and then in the caller (there are multiple), once the std::optional<EventTiming> goes out of scope.
And, the check in l. 125 below may accomplish to filter out the bogus constructor invocation, because the Persistent<Foo> is smart enough to clear out event_ from the stack-allocated instance?
On the usage of the Persistent<Foo> field: I have the feeling that it's actually correct, because the stack is a garbage collection root, and we don't want these objects to get garbage collected before the EventTiming instance gets deleted?
Perhaps we could do away with the TryCreate returning an std::optional<EventTiming> with the hidden move stuff, and instead have that logic in a more pedestrian instance method. E.g., void Init(...) with the same parameters as TryCreate?
I think I will not change this established pattern, but will update the semanstics a bit and clean the small new bug introduced.
(The idea of move semantics leaving an empty shell of an object behind that gets destructed is pretty standard modern C++, and I prefer assigning optionals to creating Init() methods, which just have the opposite problem: you could destruct a non-initialized object that has an empty event_)
Related details
- Johannes Henkel
- Scott Haseley
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. |
Michal Mocny (Gerrit)
Michal Mocny added 4 comments![]( "Open in Gerrit")
Persistent<PerformanceEventTiming> entry_;This (and the others) really should be stored as raw pointers since this is stack allocated. I think for `event_` that would require clearing state on move() though.
There was recently some back and forth on this best practice, I thought. If its alright with you, I will leave this for this patch and we can investigate?
if (!DomWindow() || !DomWindow()->GetFrame()) {What would happen if running the event handler detaches the window?
Done
Maybe a check of sorts to guard against underflow in the next line?
E.g. CHECK(entry_nesting_level_ > 0);
Done
DomWindow()->GetFrame()->GetChromeClient().NotifyPresentationTime(This is still in the same method, but I think you removed the check from l. 651 (base version) so it could crash? Or is checking for |entry| sufficient to prevent a null dereference?
Scott HaseleyI thought it was, but I will add it back to be extra careful.
In general I wouldn't think this could change in the middle of event dispatch (and still have the event finish dispatch), but I've been surprised before! (i.e. devtools debugging pauses or something)
Michal MocnyAlong the lines of what I asked in the other comment, I'm pretty sure you can construct an example where an iframe causes itself to be removed in an event handler.
lol, test failures confirm it!
I've added back the guard, but also done so slightly more robustly: if the event starts to get processed and the window is detached, we would previously leave the event in a half measured state.
This is somewhat fine, the event timing has no window to get reported to, and presumably the performance object is going to get cleaned up too.
But the state of the event timing measurements gets into a broken place and if ever there were more event timings things may blow up, so I "unwind the work of the event stack" and explicitly assign a fallback time for this case.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
cl passed the run, and ive addressed feedback. ptal next week!
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny abandoned this change.
Michal Mocny abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// event_ might potentially be null if this is std::move()-ed.This pre-existing comment feels a little scary, is this an indication that stuff gets processed out of the expected RAII order? Could it be that the new, more efficient code is more reliant on the order than the original (the search in the other file that you're getting rid of, basically)? (speculating / ignore as you see fit please, I think I don't understand it well enough yet to really tell)
Johannes HenkelNo I think it's just a product of this being moveable (during creation). This is common for moveable RAII classes, and you'd have to make it non-moveable to avoid this. (btw it should be "this will be null if this has been moved", not potentially).
Michal MocnyInteresting. So, by changing the header to
EventTiming(EventTiming&&) = delete;
EventTiming& operator=(EventTiming&&) = delete;I confirmed that TryCreate does in fact cause something like std::move to happen, as the object is returned in the std::optional - it won't compile with the delete version above.
Below some speculation / theory:
I'm also thinking that maybe the destructor runs twice, once as the thingy that was stack-allocated in TryCreate gets deleted (the remnants of what was moved), and then in the caller (there are multiple), once the std::optional<EventTiming> goes out of scope.
And, the check in l. 125 below may accomplish to filter out the bogus constructor invocation, because the Persistent<Foo> is smart enough to clear out event_ from the stack-allocated instance?
On the usage of the Persistent<Foo> field: I have the feeling that it's actually correct, because the stack is a garbage collection root, and we don't want these objects to get garbage collected before the EventTiming instance gets deleted?
Perhaps we could do away with the TryCreate returning an std::optional<EventTiming> with the hidden move stuff, and instead have that logic in a more pedestrian instance method. E.g., void Init(...) with the same parameters as TryCreate?
I think I will not change this established pattern, but will update the semanstics a bit and clean the small new bug introduced.
(The idea of move semantics leaving an empty shell of an object behind that gets destructed is pretty standard modern C++, and I prefer assigning optionals to creating Init() methods, which just have the opposite problem: you could destruct a non-initialized object that has an empty event_)
On the usage of the Persistent<Foo> field: I have the feeling that it's actually correct, because the stack is a garbage collection root, and we don't want these objects to get garbage collected before the EventTiming instance gets deleted?
That's not true: Blink does conservative stack scanning to avoid GCing objects with a raw pointer on the stack. Please see https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#stack_allocated for the part specifically about using raw pointers in STACK_ALLOCATED objects and https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#raw-pointers for the general case. You should only need Persistent<> when the object doesn't live on the stack, or (as recently learned) if you need to keep it alive past when it's used on the stack, neither or which I believe apply here.
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
3 comments
Persistent<PerformanceEventTiming> entry_;I won't block this since this is the established pattern here, but I think this is wrong. This class was changed from heap allocated (where persistents were needed) to STACK_ALLOCATED here: https://chromium-review.googlesource.com/c/chromium/src/+/5627563/16/third_party/blink/renderer/core/timing/event_timing.h), and I think this was just missed in review.
From https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#STACK_ALLOCATED: "Any fields holding garbage-collected objects should use raw pointers or references."
And since the values are all used in the destructor, there's no risk of scoping issues like we saw with the tracing thing recently.
If we don't fix this here, can we follow up and fix this?
for (base::TimeTicks LoadTimingInfo::* ts :Is this from JJ doing too much formatting? Fine to keep it, but the blame will be a bit wonky.
Related details
- Johannes Henkel
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
Persistent<PerformanceEventTiming> entry_;I won't block this since this is the established pattern here, but I think this is wrong. This class was changed from heap allocated (where persistents were needed) to STACK_ALLOCATED here: https://chromium-review.googlesource.com/c/chromium/src/+/5627563/16/third_party/blink/renderer/core/timing/event_timing.h), and I think this was just missed in review.
From https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#STACK_ALLOCATED: "Any fields holding garbage-collected objects should use raw pointers or references."
And since the values are all used in the destructor, there's no risk of scoping issues like we saw with the tracing thing recently.
If we don't fix this here, can we follow up and fix this?
I will fix in this patch, then!
for (base::TimeTicks LoadTimingInfo::* ts :Is this from JJ doing too much formatting? Fine to keep it, but the blame will be a bit wonky.
It is. `jj fix` is the equivalent of `git cl format` but has a known bug where it formats the whole file. Uploads via JJ will automatically run a fix.
But I've since learned that I can upload with `jj upload --no-fix`, and can still rely on `git cl format` manually.
I will do so!
(not a huge deal for this patch, but future revisions have larger deltas)
Related details
- Johannes Henkel
- Scott Haseley
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. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
Michal MocnyI won't block this since this is the established pattern here, but I think this is wrong. This class was changed from heap allocated (where persistents were needed) to STACK_ALLOCATED here: https://chromium-review.googlesource.com/c/chromium/src/+/5627563/16/third_party/blink/renderer/core/timing/event_timing.h), and I think this was just missed in review.
From https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#STACK_ALLOCATED: "Any fields holding garbage-collected objects should use raw pointers or references."
And since the values are all used in the destructor, there's no risk of scoping issues like we saw with the tracing thing recently.
If we don't fix this here, can we follow up and fix this?
I will fix in this patch, then!
Done
Michal MocnyIs this from JJ doing too much formatting? Fine to keep it, but the blame will be a bit wonky.
It is. `jj fix` is the equivalent of `git cl format` but has a known bug where it formats the whole file. Uploads via JJ will automatically run a fix.
But I've since learned that I can upload with `jj upload --no-fix`, and can still rely on `git cl format` manually.
I will do so!
(not a huge deal for this patch, but future revisions have larger deltas)
Done
Related details
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
if (event_ && performance_) {I think this stuff being null relied on behavior from the Persistent template, so now you'd have to clear these in a custom implementation of one of these or set a bool or similar to indicate that the destructor shouldn't run?
EventTiming(EventTiming&&) = default;
EventTiming& operator=(EventTiming&&) = default;
Related details
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
I think this stuff being null relied on behavior from the Persistent template, so now you'd have to clear these in a custom implementation of one of these or set a bool or similar to indicate that the destructor shouldn't run?
EventTiming(EventTiming&&) = default;
EventTiming& operator=(EventTiming&&) = default;
Good eye! Scott also pinged me about this and new CL already uploaded to fix it, but appreciate it.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
I think this entry is accidental, adding some empty file.
Related details
- Michal Mocny
- Scott Haseley
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +1 |
1 comment
I think this entry is accidental, adding some empty file.
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
LGTM
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel voted Code-Review+1![]( "Open in Gerrit")
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[event-timing] Simplify Event Timing entry tracking during RAII stack
EventTiming creates a performance entry at ProcessingStart, and relies
on a RAII stack based object to track the start->end of event dispatch
processing times. At ProcessingEnd, we need to match to the original
event timing performance entry, in order to update timings.
Previously, we would search through the list of event timings to assume
we found the right match, and would do another similar search to check
if we have nested scopes.
This change simplifies that by just storing a reference in the RAII
object we are already guaranteed to have access to. This is part of a
larger cleanup which moves more work into the RAII scope of the Event.
Also adds better handling (rare) cases of a window becoming detached during
event dispatch of the event being measured.
- M third_party/blink/renderer/core/timing/event_timing.cc
- M third_party/blink/renderer/core/timing/event_timing.h
- M third_party/blink/renderer/core/timing/performance_event_timing.cc
- M third_party/blink/renderer/core/timing/performance_event_timing.h
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/core/timing/window_performance.h
- M third_party/blink/renderer/core/timing/window_performance_test.cc
Code-Review: +1 by Johannes Henkel, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |