Skip to content

devtools: Implement frame scoped evaluation #42936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eerii merged 1 commit into from
Mar 3, 2026
Merged

devtools: Implement frame scoped evaluation #42936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions components/devtools/actors/console.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
use atomic_refcell::AtomicRefCell;
use base::generic_channel::{self, GenericSender};
use base::id::TEST_PIPELINE_ID;
use devtools_traits::EvaluateJSReply::{
use devtools_traits::EvaluateJSReplyValue::{
ActorValue, BooleanValue, NullValue, NumberValue, StringValue, VoidValue,
};
use devtools_traits::{
Expand Down Expand Up @@ -227,6 +227,7 @@ struct EvaluateJSReply {
timestamp: u64,
exception: Value,
exception_message: Value,
has_exception: bool,
helper_result: Value,
}

Expand All @@ -243,6 +244,7 @@ struct EvaluateJSEvent {
result_id: String,
exception: Value,
exception_message: Value,
has_exception: bool,
helper_result: Value,
}

Expand Down Expand Up @@ -319,6 +321,10 @@ impl ConsoleActor {
msg: &Map<String, Value>,
) -> Result<EvaluateJSReply, ()> {
let input = msg.get("text").unwrap().as_str().unwrap().to_owned();
let frame_actor_id = msg
.get("frameActor")
.and_then(|v| v.as_str())
.map(String::from);
let (chan, port) = generic_channel::channel().unwrap();
// FIXME: Redesign messages so we don't have to fake pipeline ids when
// communicating with workers.
Expand All @@ -327,11 +333,19 @@ impl ConsoleActor {
UniqueId::Worker(_) => TEST_PIPELINE_ID,
};
self.script_chan(registry)
.send(DevtoolScriptControlMsg::Eval(input.clone(), pipeline, chan))
.send(DevtoolScriptControlMsg::Eval(
input.clone(),
pipeline,
frame_actor_id,
chan,
))
.unwrap();

// TODO: Extract conversion into protocol module or some other useful place
let result = match port.recv().map_err(|_| ())? {
let eval_result = port.recv().map_err(|_| ())?;
let has_exception = eval_result.has_exception;

let result = match eval_result.value {
VoidValue => {
let mut m = Map::new();
m.insert("type".to_owned(), Value::String("undefined".to_owned()));
Expand Down Expand Up @@ -380,18 +394,17 @@ impl ConsoleActor {
},
};

// TODO: Catch and return exception values from JS evaluation
// TODO: This should have a has_exception field

@atbrakhi atbrakhi Mar 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to do what this comment said, to introduce a has_exception field in EvaluateJSReply. Introducing this resolved the reference error we see in screenshot of the previous comment.

let reply = EvaluateJSReply {
from: self.name(),
input,
result,
timestamp: get_time_stamp(),
exception: Value::Null,
exception_message: Value::Null,
has_exception,
helper_result: Value::Null,
};
std::result::Result::Ok(reply)
Ok(reply)
}

pub(crate) fn handle_console_resource(
Expand Down Expand Up @@ -532,6 +545,7 @@ impl Actor for ConsoleActor {
result_id,
exception: reply.exception,
exception_message: reply.exception_message,
has_exception: reply.has_exception,
helper_result: reply.helper_result,
};
// Send the data from evaluateJS along with a resultID
Expand Down
22 changes: 14 additions & 8 deletions components/script/devtools.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ use base::generic_channel::GenericSender;
use base::id::PipelineId;
use devtools_traits::{
AttrModification, AutoMargins, ComputedNodeLayout, CssDatabaseProperty, EvaluateJSReply,
EventListenerInfo, NodeInfo, NodeStyle, RuleModification, TimelineMarker, TimelineMarkerType,
EvaluateJSReplyValue, EventListenerInfo, NodeInfo, NodeStyle, RuleModification, TimelineMarker,
TimelineMarkerType,
};
use js::context::JSContext;
use js::conversions::jsstr_to_string;
Expand Down Expand Up @@ -129,7 +130,7 @@ pub(crate) fn handle_evaluate_js(
reply: GenericSender<EvaluateJSReply>,
cx: &mut JSContext,
) {
let result = unsafe {
let value = unsafe {
let mut realm = enter_auto_realm(cx, global);
let cx = &mut realm.current_realm();
rooted!(&in(cx) let mut rval = UndefinedValue());
Expand All @@ -144,33 +145,38 @@ pub(crate) fn handle_evaluate_js(
);

if rval.is_undefined() {
EvaluateJSReply::VoidValue
EvaluateJSReplyValue::VoidValue
} else if rval.is_boolean() {
EvaluateJSReply::BooleanValue(rval.to_boolean())
EvaluateJSReplyValue::BooleanValue(rval.to_boolean())
} else if rval.is_double() || rval.is_int32() {
EvaluateJSReply::NumberValue(
EvaluateJSReplyValue::NumberValue(
match FromJSValConvertible::from_jsval(cx.raw_cx(), rval.handle(), ()) {
Ok(ConversionResult::Success(v)) => v,
_ => unreachable!(),
},
)
} else if rval.is_string() {
let jsstr = std::ptr::NonNull::new(rval.to_string()).unwrap();
EvaluateJSReply::StringValue(jsstr_to_string(cx.raw_cx(), jsstr))
EvaluateJSReplyValue::StringValue(jsstr_to_string(cx.raw_cx(), jsstr))
} else if rval.is_null() {
EvaluateJSReply::NullValue
EvaluateJSReplyValue::NullValue
} else {
assert!(rval.is_object());

let jsstr = std::ptr::NonNull::new(ToString(cx.raw_cx(), rval.handle())).unwrap();
let class_name = jsstr_to_string(cx.raw_cx(), jsstr);

EvaluateJSReply::ActorValue {
EvaluateJSReplyValue::ActorValue {
class: class_name,
uuid: Uuid::new_v4().to_string(),
}
}
};

let result = EvaluateJSReply {
value,
has_exception: false,
};
reply.send(result).unwrap();
}

Expand Down
7 changes: 7 additions & 0 deletions components/script/dom/debuggerevalevent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub(crate) struct DebuggerEvalEvent {
code: DOMString,
pipeline_id: Dom<PipelineId>,
worker_id: Option<DOMString>,
frame_actor_id: Option<DOMString>,
}

impl DebuggerEvalEvent {
Expand All @@ -28,13 +29,15 @@ impl DebuggerEvalEvent {
code: DOMString,
pipeline_id: &PipelineId,
worker_id: Option<DOMString>,
frame_actor_id: Option<DOMString>,
can_gc: CanGc,
) -> DomRoot<Self> {
let result = Box::new(Self {
event: Event::new_inherited(),
code,
pipeline_id: Dom::from_ref(pipeline_id),
worker_id,
frame_actor_id,
});
let result = reflect_dom_object(result, debugger_global, can_gc);
result.event.init_event("eval".into(), false, false);
Expand All @@ -59,6 +62,10 @@ impl DebuggerEvalEventMethods<crate::DomTypeHolder> for DebuggerEvalEvent {
self.worker_id.clone()
}

fn GetFrameActorId(&self) -> Option<DOMString> {
self.frame_actor_id.clone()
}

fn IsTrusted(&self) -> bool {
self.event.IsTrusted()
}
Expand Down
78 changes: 42 additions & 36 deletions components/script/dom/debuggerglobalscope.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ use base::generic_channel::{GenericCallback, GenericSender, channel};
use base::id::{Index, PipelineId, PipelineNamespaceId};
use constellation_traits::ScriptToConstellationChan;
use devtools_traits::{
DevtoolScriptControlMsg, EvaluateJSReply, ScriptToDevtoolsControlMsg, SourceInfo, WorkerId,
DevtoolScriptControlMsg, EvaluateJSReply, EvaluateJSReplyValue, ScriptToDevtoolsControlMsg,
SourceInfo, WorkerId,
};
use dom_struct::dom_struct;
use embedder_traits::ScriptToEmbedderChan;
Expand Down Expand Up @@ -170,6 +171,7 @@ impl DebuggerGlobalScope {
code: DOMString,
debuggee_pipeline_id: PipelineId,
debuggee_worker_id: Option<WorkerId>,
frame_actor_id: Option<String>,
result_sender: GenericSender<EvaluateJSReply>,
) {
assert!(
Expand All @@ -185,6 +187,7 @@ impl DebuggerGlobalScope {
code,
&debuggee_pipeline_id,
debuggee_worker_id.map(|id| id.to_string().into()),
frame_actor_id.map(|id| id.into()),
can_gc,
));
assert!(
Expand Down Expand Up @@ -413,41 +416,44 @@ impl DebuggerGlobalScopeMethods<crate::DomTypeHolder> for DebuggerGlobalScope {
.take()
.expect("Guaranteed by Self::fire_eval()");

let reply = if result.completionType.str() == "terminated" {
EvaluateJSReply::VoidValue
} else {
match &*result.valueType.str() {
"undefined" => EvaluateJSReply::VoidValue,
"null" => EvaluateJSReply::NullValue,
"boolean" => {
EvaluateJSReply::BooleanValue(result.booleanValue.flatten().unwrap_or(false))
},
"number" => {
let num = result.numberValue.flatten().map(|f| *f).unwrap_or(0.0);
EvaluateJSReply::NumberValue(num)
},
"string" => EvaluateJSReply::StringValue(
result
.stringValue
.as_ref()
.and_then(|opt| opt.as_ref())
.map(|s| s.to_string())
.unwrap_or_default(),
),
"object" => {
let class = result
.objectClass
.as_ref()
.and_then(|opt| opt.as_ref())
.map(|s| s.to_string())
.unwrap_or_else(|| "Object".to_string());
EvaluateJSReply::ActorValue {
class,
uuid: uuid::Uuid::new_v4().to_string(),
}
},
_ => unreachable!(),
}
let has_exception = result.hasException.flatten().unwrap_or(false);

let value = match &*result.valueType.str() {
"undefined" => EvaluateJSReplyValue::VoidValue,
"null" => EvaluateJSReplyValue::NullValue,
"boolean" => {
EvaluateJSReplyValue::BooleanValue(result.booleanValue.flatten().unwrap_or(false))
},
"number" => {
let num = result.numberValue.flatten().map(|f| *f).unwrap_or(0.0);
EvaluateJSReplyValue::NumberValue(num)
},
"string" => EvaluateJSReplyValue::StringValue(
result
.stringValue
.as_ref()
.and_then(|opt| opt.as_ref())
.map(|s| s.to_string())
.unwrap_or_default(),
),
"object" => {
let class = result
.objectClass
.as_ref()
.and_then(|opt| opt.as_ref())
.map(|s| s.to_string())
.unwrap_or_else(|| "Object".to_string());
EvaluateJSReplyValue::ActorValue {
class,
uuid: uuid::Uuid::new_v4().to_string(),
}
},
_ => unreachable!(),
};

let reply = EvaluateJSReply {
value,
has_exception,
};

let _ = sender.send(reply);
Expand Down
12 changes: 9 additions & 3 deletions components/script/script_thread.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2249,9 +2249,15 @@ impl ScriptThread {
node_id.as_deref(),
)
},
DevtoolScriptControlMsg::Eval(code, id, reply) => {
self.debugger_global
.fire_eval(CanGc::from_cx(cx), code.into(), id, None, reply);
DevtoolScriptControlMsg::Eval(code, id, frame_actor_id, reply) => {
self.debugger_global.fire_eval(
CanGc::from_cx(cx),
code.into(),
id,
None,
frame_actor_id,
reply,
);
},
DevtoolScriptControlMsg::GetPossibleBreakpoints(spidermonkey_id, result_sender) => {
self.debugger_global.fire_get_possible_breakpoints(
Expand Down
2 changes: 2 additions & 0 deletions components/script_bindings/webidls/DebuggerEvalEvent.webidl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ interface DebuggerEvalEvent : Event {
readonly attribute DOMString code;
readonly attribute PipelineId pipelineId;
readonly attribute DOMString? workerId;
readonly attribute DOMString? frameActorId;
};

partial interface DebuggerGlobalScope {
Expand Down Expand Up @@ -36,4 +37,5 @@ dictionary EvalResultValue {
// A string naming the ECMAScript [[Class]] of the referent.
// <https://firefox-source-docs.mozilla.org/js/Debugger/Debugger.Object.html#accessor-properties-of-the-debugger-object-prototype>
DOMString? objectClass;
boolean? hasException;
};
16 changes: 13 additions & 3 deletions components/shared/devtools/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,8 @@ pub enum DomMutation {
}

/// Serialized JS return values
/// TODO: generalize this beyond the EvaluateJS message?
#[derive(Debug, Deserialize, Serialize)]
pub enum EvaluateJSReply {
pub enum EvaluateJSReplyValue {

@eerii eerii Mar 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very small nits, but maybe this can be simplified to EvaluateJSValue, or even JSValue if we are going to use it outside of eval (though that might be confused with the other JSValue).

Also, I'm not sure why clippy isn't complaining, but since all of the variants end with Value we should probably remove the suffix and have EvaluateJSValue::Void, and so on. The lint is enum_variant_names.

@atbrakhi atbrakhi Mar 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent lots of time on this renaming. My other option was also EvaluateJSValue but i didn't wanted to give the feel that this is all the Evaluate js value as we have a lot more then that. Hence I couldn't convince myself to rename it to EvaluateJSValue. But i will agree that i am also not a big fan of EvaluateJSReplyValue.

I think we will be touching this function more in coming weeks, how do you feel about landing it the way it is and renaming it later?...i am hoping to find something better

@eerii eerii Mar 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is a tricky naming choice indeed. I agree that we should think about it and rename it later :)

VoidValue,
NullValue,
BooleanValue(bool),
Expand All @@ -160,6 +159,12 @@ pub enum EvaluateJSReply {
ActorValue { class: String, uuid: String },
}

#[derive(Debug, Deserialize, Serialize)]
pub struct EvaluateJSReply {
pub value: EvaluateJSReplyValue,
pub has_exception: bool,
}

#[derive(Debug, Deserialize, Serialize)]
pub struct AttrInfo {
pub namespace: String,
Expand Down Expand Up @@ -333,7 +338,12 @@ pub enum DevtoolScriptControlMsg {
/// Highlight the given DOM node
HighlightDomNode(PipelineId, Option<String>),

Eval(String, PipelineId, GenericSender<EvaluateJSReply>),
Eval(
String,
PipelineId,
Option<String>,
GenericSender<EvaluateJSReply>,
),
GetPossibleBreakpoints(u32, GenericSender<Vec<RecommendedBreakpointLocation>>),
SetBreakpoint(u32, u32, u32),
ClearBreakpoint(u32, u32, u32),
Expand Down
Loading