3P Script Detector Share Between Iframes [chromium/src : main]
Ian Clelland (Gerrit)
Ian Clelland added 1 comment![]( "Open in Gerrit")
frame->DomWindow());If `frame` is null here, this will also crash; also, it's only used to get the DOMWindow. Would it make sense to pass in a `LocalDOMWindow*` instead?
Related details
- Aoyuan Zuo
- David Bokan
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aoyuan Zuo (Gerrit)
Aoyuan Zuo added 1 comment![]( "Open in Gerrit")
If `frame` is null here, this will also crash; also, it's only used to get the DOMWindow. Would it make sense to pass in a `LocalDOMWindow*` instead?
- David Bokan
- Ian Clelland
- Noam Rosenthal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
- Aoyuan Zuo
- David Bokan
- Ian Clelland
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 4 comments![]( "Open in Gerrit")
void DidBeginMainFrame(LocalDOMWindow* window);nit: `local_root_window`
if (frame->IsMainFrame()) {Is `RecordLongAnimationFrameUKMAndTrace` meant only to be called for the main frame? If so, we're not excluding it properly in `WebFrameWidgetImpl::DidBeginMainFrame` - will comment there...
LocalFrame* root_frame = LocalRootImpl()->GetFrame();This isn't actually the root frame, it's the local root. I.e. if you're in an OOPIF process, this will be the first ancestor frame who's parent is in a remote process.
I wonder if we really do mean "root frame" here since we check `frame->IsMainFrame` when calling from AnimationFrameTimingMonitor. (If so you should check `root_frame->IsOuterMostMainFrame()`)
animation_frame_timing_monitor_->DidBeginMainFrame(root_frame->DomWindow());Can we CHECK and make this a ref argument? I'd expect a frame producing animation frames to be attached.
Related details
- Aoyuan Zuo
- Ian Clelland
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
3 comments
if (frame->IsMainFrame()) {Is `RecordLongAnimationFrameUKMAndTrace` meant only to be called for the main frame? If so, we're not excluding it properly in `WebFrameWidgetImpl::DidBeginMainFrame` - will comment there...
Done
LocalFrame* root_frame = LocalRootImpl()->GetFrame();This isn't actually the root frame, it's the local root. I.e. if you're in an OOPIF process, this will be the first ancestor frame who's parent is in a remote process.
I wonder if we really do mean "root frame" here since we check `frame->IsMainFrame` when calling from AnimationFrameTimingMonitor. (If so you should check `root_frame->IsOuterMostMainFrame()`)
Summarizing offline chat (correct me if I missed something): currently we'll have different behavior or recording animation frame timings depending on if an iframe is in an OOPIF vs. in-process. (though...an argument could be made that that causes them to have separate compositors and separate timings...this is a decision I can't evaluate). But that's not really relevant to the crash so can be looked at in a follow up.
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | +1 |
1 comment
if (frame->IsMainFrame()) {Is `RecordLongAnimationFrameUKMAndTrace` meant only to be called for the main frame? If so, we're not excluding it properly in `WebFrameWidgetImpl::DidBeginMainFrame` - will comment there...
Sorry, but please revert this :)
We already check for MainFrame in WebFrameWidgetImpl::MainFrameUkmRecorder().
So we enable tracing for subframes, but we would only have a UKM recorder for the main frame.
Related details
- Aoyuan Zuo
- David Bokan
- Ian Clelland
David Bokan (Gerrit)
David Bokan added 1 comment![]( "Open in Gerrit")
if (frame->IsMainFrame()) {Noam RosenthalIs `RecordLongAnimationFrameUKMAndTrace` meant only to be called for the main frame? If so, we're not excluding it properly in `WebFrameWidgetImpl::DidBeginMainFrame` - will comment there...
Sorry, but please revert this :)
We already check for MainFrame in WebFrameWidgetImpl::MainFrameUkmRecorder().
So we enable tracing for subframes, but we would only have a UKM recorder for the main frame.
Ah, I see now - we'll early out from RecordLongAnimationFrameUKMAndTrace if we're in a remote frame because we can't access the MainFrameUkmRecorder (but after enabling tracing) - for my own curiosity - we intentionally record long frame metrics within local roots?
Related details
- Aoyuan Zuo
- Ian Clelland
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Clelland (Gerrit)
Ian Clelland voted Code-Review+1![]( "Open in Gerrit")
- Aoyuan Zuo
- Noam Rosenthal
Aoyuan Zuo (Gerrit)
Aoyuan Zuo added 4 comments![]( "Open in Gerrit")
LGTM % nit and question
Ty!
void DidBeginMainFrame(LocalDOMWindow* window);Aoyuan Zuonit: `local_root_window`
Done
Noam RosenthalIs `RecordLongAnimationFrameUKMAndTrace` meant only to be called for the main frame? If so, we're not excluding it properly in `WebFrameWidgetImpl::DidBeginMainFrame` - will comment there...
Sorry, but please revert this :)
We already check for MainFrame in WebFrameWidgetImpl::MainFrameUkmRecorder().
So we enable tracing for subframes, but we would only have a UKM recorder for the main frame.
Done
animation_frame_timing_monitor_->DidBeginMainFrame(root_frame->DomWindow());Can we CHECK and make this a ref argument? I'd expect a frame producing animation frames to be attached.
- Noam Rosenthal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | +1 |
- Aoyuan Zuo
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
2 comments
just one nit but otherwise still lgtm
RecordLongAnimationFrameUKMAndTrace(*timing_info, *frame->DomWindow());Will frame always be a local root? I can't tell from the call sites. If not, rename the param in `RecordLongANimationFrameUKMAndTrace`. If yes, add a CHECK in that method that the passed in window's frame is the local root.
Related details
- Aoyuan Zuo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aoyuan Zuo (Gerrit)
Aoyuan Zuo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aoyuan Zuo
| Commit-Queue | +1 |
1 comment
RecordLongAnimationFrameUKMAndTrace(*timing_info, *frame->DomWindow());Will frame always be a local root? I can't tell from the call sites. If not, rename the param in `RecordLongANimationFrameUKMAndTrace`. If yes, add a CHECK in that method that the passed in window's frame is the local root.
I think you're right, can't be sure to be local root, revert back to just name it window. Ty!
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aoyuan Zuo (Gerrit)
Aoyuan Zuo voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/frame/animation_frame_timing_monitor.h
Insertions: 1, Deletions: 1.
@@ -125,7 +125,7 @@
bool PushScriptEntryPoint(ScriptState*);
void RecordLongAnimationFrameUKMAndTrace(const AnimationFrameTimingInfo&,
- LocalDOMWindow& local_root_window);
+ LocalDOMWindow& window);
void ApplyTaskDuration(base::TimeDelta task_duration);
std::optional<PendingScriptInfo> pending_script_info_;
```
```
The name of the file: third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
Insertions: 3, Deletions: 3.
@@ -12,7 +12,6 @@
#include "services/metrics/public/cpp/ukm_source_id.h"
#include "third_party/blink/public/mojom/script/script_type.mojom-blink-forward.h"
#include "third_party/blink/renderer/bindings/core/v8/js_based_event_listener.h"
-#include "third_party/blink/renderer/bindings/core/v8/js_event_handler_for_content_attribute.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
#include "third_party/blink/renderer/core/core_probe_sink.h"
#include "third_party/blink/renderer/core/execution_context/agent.h"
@@ -280,7 +279,7 @@
void AnimationFrameTimingMonitor::RecordLongAnimationFrameUKMAndTrace(
const AnimationFrameTimingInfo& info,
- LocalDOMWindow& local_root_window) {
+ LocalDOMWindow& window) {
// Record all animation frames to traces, but only long ones to UKM.
RecordLongAnimationFrameTrace(info, this);
if (info.Duration() < kLongAnimationFrameDuration) {
@@ -317,8 +316,8 @@
total_forced_style_and_layout_duration += script->StyleDuration();
total_forced_style_and_layout_duration += script->LayoutDuration();
ThirdPartyScriptDetector::Technology third_party_technology =
- ThirdPartyScriptDetector::From(local_root_window)
- .Detect(script->GetSourceLocation().url);
+ ThirdPartyScriptDetector::From(window).Detect(
+ script->GetSourceLocation().url);
switch (script->GetInvokerType()) {
case ScriptTimingInfo::InvokerType::kClassicScript:
case ScriptTimingInfo::InvokerType::kModuleScript:
@@ -500,9 +499,7 @@
? ScriptTimingInfo::InvokerType::kModuleScript
: ScriptTimingInfo::InvokerType::kClassicScript,
.start_time = probe_data.CaptureStartTime(),
- .source_location = {
- .url = url,
- .char_position = static_cast<int>(probe_data.char_index)}};
+ .source_location = {.url = url, .char_position = 0}};
if (probe_data.sanitize) {
pending_script_info_->execution_start_time =
pending_script_info_->start_time;
@@ -635,21 +632,9 @@
}
v8::HandleScope scope(probe_data.script_state->GetIsolate());
- if (probe_data.listener->IsEventHandlerForContentAttribute()) {
- auto* handler =
- static_cast<JSEventHandlerForContentAttribute*>(probe_data.listener);
- StringBuilder builder;
- builder.Append("on");
- builder.Append(probe_data.event->type());
- info->SetSourceLocation(ScriptTimingInfo::ScriptSourceLocation{
- .url = ExecutionContext::From(probe_data.script_state)->Url(),
- .function_name = builder.ReleaseString(),
- .char_position = static_cast<int>(handler->GetTextPosition().offset_)});
- } else {
- info->SetSourceLocation(CaptureScriptSourceLocation(
- probe_data.script_state->GetIsolate(),
- probe_data.listener->GetListenerObject(*target)));
- }
+ info->SetSourceLocation(CaptureScriptSourceLocation(
+ probe_data.script_state->GetIsolate(),
+ probe_data.listener->GetListenerObject(*target)));
}
void AnimationFrameTimingMonitor::Will(
```
Change information
3P Script Detector Share Between Iframes
* Previous undesired outcome: Script window can be null and dereference
nullptr results in crash.
* Root cause: We suspect script from iframe could have their dom window
detached by the time animation_frame_timing_monitor doing 3p script
detection after BeginMainFrame.
* This CL address it by: use local root frame's window instead to
retrieve 3p script detector; also added a check to early exit just in
case.
- M third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
- M third_party/blink/renderer/core/frame/animation_frame_timing_monitor.h
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
Code-Review: +1 by David Bokan, +1 by Ian Clelland, +1 by Noam Rosenthal
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aoyuan Zuo (Gerrit)
Aoyuan Zuo abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |