DevTools: Display console messages and errors (#32727)

* feat: add streams to browsing context

* feat: console now works!

* feat: order console messages

* feat: add streams to new browsing contexts

* fix: apply suggestions

Co-authored-by: Martin Robinson <mrobinson@igalia.com>

---------

Co-authored-by: Martin Robinson <mrobinson@igalia.com>

components/devtools/actors/browsing_context.rs

@@ -12,7 +12,7 @@ use std::net::TcpStream;
 
 use base::id::{BrowsingContextId, PipelineId};
 use devtools_traits::DevtoolScriptControlMsg::{self, WantsLiveNotifications};
-use devtools_traits::{DevtoolsPageInfo, NavigationState};
+use devtools_traits::{ConsoleLog, DevtoolsPageInfo, NavigationState, PageError};
 use ipc_channel::ipc::IpcSender;
 use serde::Serialize;
 use serde_json::{Map, Value};
@@ -70,6 +70,36 @@ struct ResourceAvailableMsg {
     url: Option<String>,
 }
 
+#[derive(Serialize)]
+struct ConsoleMsg {
+    from: String,
+    #[serde(rename = "type")]
+    type_: String,
+    resources: Vec<ConsoleMessageResource>,
+}
+
+#[derive(Serialize)]
+#[serde(rename_all = "camelCase")]
+struct ConsoleMessageResource {
+    message: ConsoleLog,
+    resource_type: String,
+}
+
+#[derive(Serialize)]
+struct PageErrorMsg {
+    from: String,
+    #[serde(rename = "type")]
+    type_: String,
+    resources: Vec<PageErrorResource>,
+}
+
+#[derive(Serialize)]
+#[serde(rename_all = "camelCase")]
+struct PageErrorResource {
+    page_error: PageError,
+    resource_type: String,
+}
+
 #[derive(Serialize)]
 struct TabNavigated {
     from: String,
@@ -347,7 +377,7 @@ impl BrowsingContextActor {
                 from: self.name(),
                 type_: "resource-available-form".into(),
                 resources: vec![ResourceAvailableMsg {
-                    has_native_console_api: None,
+                    has_native_console_api: Some(true),
                     name: name.into(),
                     new_uri: None,
                     resource_type: "document-event".into(),
@@ -358,4 +388,34 @@ impl BrowsingContextActor {
             });
         }
     }
+
+    pub(crate) fn console_message(&self, message: ConsoleLog) {
+        let msg = ConsoleMsg {
+            from: self.name(),
+            type_: "resource-available-form".into(),
+            resources: vec![ConsoleMessageResource {
+                message,
+                resource_type: "console-message".into(),
+            }],
+        };
+
+        for stream in self.streams.borrow_mut().values_mut() {
+            let _ = stream.write_json_packet(&msg);
+        }
+    }
+
+    pub(crate) fn page_error(&self, page_error: PageError) {
+        let msg = PageErrorMsg {
+            from: self.name(),
+            type_: "resource-available-form".into(),
+            resources: vec![PageErrorResource {
+                page_error,
+                resource_type: "error-message".into(),
+            }],
+        };
+
+        for stream in self.streams.borrow_mut().values_mut() {
+            let _ = stream.write_json_packet(&msg);
+        }
+    }
 }

components/devtools/actors/console.rs

@@ -16,7 +16,7 @@ use devtools_traits::EvaluateJSReply::{
     ActorValue, BooleanValue, NullValue, NumberValue, StringValue, VoidValue,
 };
 use devtools_traits::{
-    CachedConsoleMessage, CachedConsoleMessageTypes, ConsoleAPI, ConsoleMessage,
+    CachedConsoleMessage, CachedConsoleMessageTypes, ConsoleLog, ConsoleMessage,
     DevtoolScriptControlMsg, LogLevel, PageError,
 };
 use ipc_channel::ipc::{self, IpcSender};
@@ -40,7 +40,7 @@ impl EncodableConsoleMessage for CachedConsoleMessage {
     fn encode(&self) -> serde_json::Result<String> {
         match *self {
             CachedConsoleMessage::PageError(ref a) => serde_json::to_string(a),
-            CachedConsoleMessage::ConsoleAPI(ref a) => serde_json::to_string(a),
+            CachedConsoleMessage::ConsoleLog(ref a) => serde_json::to_string(a),
         }
     }
 }
@@ -141,23 +141,6 @@ impl ConsoleActor {
         }
     }
 
-    fn streams_mut(&self, registry: &ActorRegistry, cb: impl Fn(&mut TcpStream)) {
-        match &self.root {
-            Root::BrowsingContext(bc) => registry
-                .find::<BrowsingContextActor>(bc)
-                .streams
-                .borrow_mut()
-                .values_mut()
-                .for_each(cb),
-            Root::DedicatedWorker(worker) => registry
-                .find::<WorkerActor>(worker)
-                .streams
-                .borrow_mut()
-                .values_mut()
-                .for_each(cb),
-        }
-    }
-
     fn current_unique_id(&self, registry: &ActorRegistry) -> UniqueId {
         match &self.root {
             Root::BrowsingContext(bc) => UniqueId::Pipeline(
@@ -177,7 +160,7 @@ impl ConsoleActor {
     ) -> Result<EvaluateJSReply, ()> {
         let input = msg.get("text").unwrap().as_str().unwrap().to_owned();
         let (chan, port) = ipc::channel().unwrap();
-        // FIXME: redesign messages so we don't have to fake pipeline ids when
+        // FIXME: Redesign messages so we don't have to fake pipeline ids when
         //        communicating with workers.
         let pipeline = match self.current_unique_id(registry) {
             UniqueId::Pipeline(p) => p,
@@ -191,7 +174,7 @@ impl ConsoleActor {
             ))
             .unwrap();
 
-        //TODO: extract conversion into protocol module or some other useful place
+        // TODO: Extract conversion into protocol module or some other useful place
         let result = match port.recv().map_err(|_| ())? {
             VoidValue => {
                 let mut m = Map::new();
@@ -227,7 +210,7 @@ impl ConsoleActor {
             },
             StringValue(s) => Value::String(s),
             ActorValue { class, uuid } => {
-                //TODO: make initial ActorValue message include these properties?
+                // TODO: Make initial ActorValue message include these properties?
                 let mut m = Map::new();
                 let actor = ObjectActor::register(registry, uuid);
 
@@ -241,12 +224,15 @@ impl ConsoleActor {
             },
         };
 
-        //TODO: catch and return exception values from JS evaluation
+        // TODO: Catch and return exception values from JS evaluation
         let reply = EvaluateJSReply {
             from: self.name(),
             input,
             result,
-            timestamp: 0,
+            timestamp: SystemTime::now()
+                .duration_since(UNIX_EPOCH)
+                .unwrap_or_default()
+                .as_millis() as u64,
             exception: Value::Null,
             exception_message: Value::Null,
             helper_result: Value::Null,
@@ -266,14 +252,11 @@ impl ConsoleActor {
             .or_default()
             .push(CachedConsoleMessage::PageError(page_error.clone()));
         if id == self.current_unique_id(registry) {
-            let msg = PageErrorMsg {
-                from: self.name(),
-                type_: "pageError".to_owned(),
-                page_error,
+            if let Root::BrowsingContext(bc) = &self.root {
+                registry
+                    .find::<BrowsingContextActor>(bc)
+                    .page_error(page_error)
             };
-            self.streams_mut(registry, |stream| {
-                let _ = stream.write_json_packet(&msg);
-            });
         }
     }
 
@@ -292,42 +275,30 @@ impl ConsoleActor {
             _ => "log",
         }
         .to_owned();
+
+        let console_api = ConsoleLog {
+            level: level.clone(),
+            filename: console_message.filename.clone(),
+            line_number: console_message.line_number as u32,
+            column_number: console_message.column_number as u32,
+            time_stamp: SystemTime::now()
+                .duration_since(UNIX_EPOCH)
+                .unwrap_or_default()
+                .as_millis() as u64,
+            arguments: vec![console_message.message.clone()],
+        };
+
         self.cached_events
             .borrow_mut()
             .entry(id.clone())
             .or_default()
-            .push(CachedConsoleMessage::ConsoleAPI(ConsoleAPI {
-                type_: "ConsoleAPI".to_owned(),
-                level: level.clone(),
-                filename: console_message.filename.clone(),
-                line_number: console_message.line_number as u32,
-                function_name: "".to_string(), //TODO
-                time_stamp: SystemTime::now()
-                    .duration_since(UNIX_EPOCH)
-                    .unwrap_or_default()
-                    .as_nanos() as u64,
-                private: false,
-                arguments: vec![console_message.message.clone()],
-            }));
+            .push(CachedConsoleMessage::ConsoleLog(console_api.clone()));
         if id == self.current_unique_id(registry) {
-            let msg = ConsoleAPICall {
-                from: self.name(),
-                type_: "consoleAPICall".to_owned(),
-                message: ConsoleMsg {
-                    level,
-                    timestamp: SystemTime::now()
-                        .duration_since(UNIX_EPOCH)
-                        .unwrap_or_default()
-                        .as_nanos() as u64,
-                    arguments: vec![console_message.message],
-                    filename: console_message.filename,
-                    line_number: console_message.line_number,
-                    column_number: console_message.column_number,
-                },
+            if let Root::BrowsingContext(bc) = &self.root {
+                registry
+                    .find::<BrowsingContextActor>(bc)
+                    .console_message(console_api)
             };
-            self.streams_mut(registry, |stream| {
-                let _ = stream.write_json_packet(&msg);
-            });
         }
     }
 }
@@ -385,7 +356,7 @@ impl Actor for ConsoleActor {
                         {
                             true
                         },
-                        CachedConsoleMessage::ConsoleAPI(_)
+                        CachedConsoleMessage::ConsoleLog(_)
                             if message_types.contains(CachedConsoleMessageTypes::CONSOLE_API) =>
                         {
                             true
@@ -506,31 +477,3 @@ impl Actor for ConsoleActor {
         })
     }
 }
-
-#[derive(Serialize)]
-struct ConsoleAPICall {
-    from: String,
-    #[serde(rename = "type")]
-    type_: String,
-    message: ConsoleMsg,
-}
-
-#[derive(Serialize)]
-#[serde(rename_all = "camelCase")]
-struct ConsoleMsg {
-    level: String,
-    timestamp: u64,
-    arguments: Vec<String>,
-    filename: String,
-    line_number: usize,
-    column_number: usize,
-}
-
-#[derive(Serialize)]
-#[serde(rename_all = "camelCase")]
-struct PageErrorMsg {
-    from: String,
-    #[serde(rename = "type")]
-    type_: String,
-    page_error: PageError,
-}

components/devtools/actors/object.rs

@@ -26,6 +26,7 @@ impl Actor for ObjectActor {
         _: &mut TcpStream,
         _: StreamId,
     ) -> Result<ActorMessageStatus, ()> {
+        // TODO: Handle enumSymbols for console object inspection
         Ok(ActorMessageStatus::Ignored)
     }
 }

components/devtools/lib.rs

@@ -191,13 +191,11 @@ fn run_server(
 
     let actors = registry.create_shareable();
 
-    let mut accepted_connections: Vec<TcpStream> = Vec::new();
-
-    let mut browsing_contexts: HashMap<BrowsingContextId, String> = HashMap::new();
-    let mut pipelines: HashMap<PipelineId, BrowsingContextId> = HashMap::new();
-    let mut actor_requests: HashMap<String, String> = HashMap::new();
-
-    let mut actor_workers: HashMap<WorkerId, String> = HashMap::new();
+    let mut accepted_connections = HashMap::new();
+    let mut browsing_contexts: HashMap<_, String> = HashMap::new();
+    let mut pipelines = HashMap::new();
+    let mut actor_requests = HashMap::new();
+    let mut actor_workers = HashMap::new();
 
     /// Process the input from a single devtools client until EOF.
     fn handle_client(actors: Arc<Mutex<ActorRegistry>>, mut stream: TcpStream, id: StreamId) {
@@ -289,6 +287,7 @@ fn run_server(
         pipelines: &mut HashMap<PipelineId, BrowsingContextId>,
         actor_workers: &mut HashMap<WorkerId, String>,
         page_info: DevtoolsPageInfo,
+        connections: &HashMap<StreamId, TcpStream>,
     ) {
         let mut actors = actors.lock().unwrap();
 
@@ -324,10 +323,9 @@ fn run_server(
             Root::DedicatedWorker(worker_name)
         } else {
             pipelines.insert(pipeline, browsing_context);
-            Root::BrowsingContext(
-                if let Some(actor) = browsing_contexts.get(&browsing_context) {
-                    actor.to_owned()
-                } else {
+            let name = browsing_contexts
+                .entry(browsing_context)
+                .or_insert_with(|| {
                     let browsing_context_actor = BrowsingContextActor::new(
                         console_name.clone(),
                         browsing_context,
@@ -337,11 +335,18 @@ fn run_server(
                         &mut actors,
                     );
                     let name = browsing_context_actor.name();
-                    browsing_contexts.insert(browsing_context, name.clone());
                     actors.register(Box::new(browsing_context_actor));
                     name
-                },
-            )
+                });
+
+            // Add existing streams to the new browsing context
+            let browsing_context = actors.find::<BrowsingContextActor>(&name);
+            let mut streams = browsing_context.streams.borrow_mut();
+            for (id, stream) in connections {
+                streams.insert(*id, stream.try_clone().unwrap());
+            }
+
+            Root::BrowsingContext(name.clone())
         };
 
         let console = ConsoleActor {
@@ -609,7 +614,15 @@ fn run_server(
                 let actors = actors.clone();
                 let id = next_id;
                 next_id = StreamId(id.0 + 1);
-                accepted_connections.push(stream.try_clone().unwrap());
+                accepted_connections.insert(id, stream.try_clone().unwrap());
+
+                // Inform every browsing context of the new stream
+                for name in browsing_contexts.values() {
+                    let actors = actors.lock().unwrap();
+                    let browsing_context = actors.find::<BrowsingContextActor>(name);
+                    let mut streams = browsing_context.streams.borrow_mut();
+                    streams.insert(id, stream.try_clone().unwrap());
+                }
                 thread::Builder::new()
                     .name("DevtoolsClientHandler".to_owned())
                     .spawn(move || handle_client(actors, stream.try_clone().unwrap(), id))
@@ -641,6 +654,7 @@ fn run_server(
                 &mut pipelines,
                 &mut actor_workers,
                 pageinfo,
+                &accepted_connections,
             ),
             DevtoolsControlMsg::FromScript(ScriptToDevtoolsControlMsg::Navigate(
                 browsing_context,
@@ -698,7 +712,7 @@ fn run_server(
             )) => {
                 // copy the accepted_connections vector
                 let mut connections = Vec::<TcpStream>::new();
-                for stream in &accepted_connections {
+                for stream in accepted_connections.values() {
                     connections.push(stream.try_clone().unwrap());
                 }
 
@@ -721,7 +735,7 @@ fn run_server(
             DevtoolsControlMsg::FromChrome(ChromeToDevtoolsControlMsg::ServerExitMsg) => break,
         }
     }
-    for connection in &mut accepted_connections {
+    for connection in accepted_connections.values_mut() {
         let _ = connection.shutdown(Shutdown::Both);
     }
 }

components/script/dom/globalscope.rs

@@ -11,7 +11,7 @@ use std::rc::Rc;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
 use std::thread::JoinHandle;
-use std::time::Instant;
+use std::time::{Instant, SystemTime, UNIX_EPOCH};
 use std::{mem, ptr};
 
 use base::id::{
@@ -2305,7 +2305,10 @@ impl GlobalScope {
                     line_number: 0,
                     column_number: 0,
                     category: "script".to_string(),
-                    time_stamp: 0, //TODO
+                    time_stamp: SystemTime::now()
+                        .duration_since(UNIX_EPOCH)
+                        .unwrap_or_default()
+                        .as_millis() as u64,
                     error: false,
                     warning: true,
                     exception: true,
@@ -2493,7 +2496,10 @@ impl GlobalScope {
                             line_number: error_info.lineno,
                             column_number: error_info.column,
                             category: "script".to_string(),
-                            time_stamp: 0, //TODO
+                            time_stamp: SystemTime::now()
+                                .duration_since(UNIX_EPOCH)
+                                .unwrap_or_default()
+                                .as_millis() as u64,
                             error: true,
                             warning: false,
                             exception: true,

components/shared/devtools/lib.rs

@@ -282,23 +282,21 @@ pub struct PageError {
     pub private: bool,
 }
 
-#[derive(Debug, Deserialize, Serialize)]
-pub struct ConsoleAPI {
-    #[serde(rename = "_type")]
-    pub type_: String,
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub struct ConsoleLog {
     pub level: String,
     pub filename: String,
     pub line_number: u32,
-    pub function_name: String,
+    pub column_number: u32,
     pub time_stamp: u64,
-    pub private: bool,
     pub arguments: Vec<String>,
+    // pub stacktrace: Vec<...>,
 }
 
 #[derive(Debug, Deserialize, Serialize)]
 pub enum CachedConsoleMessage {
     PageError(PageError),
-    ConsoleAPI(ConsoleAPI),
+    ConsoleLog(ConsoleLog),
 }
 
 #[derive(Debug, PartialEq)]