Bug 1433529: Fixed TODOs in parse_sdp, r=dminor draft
authorJohannes Willbold <j.willbold@mozilla.com>
Fri, 29 Jun 2018 15:03:58 -0700
changeset 815710 f34cf389829772b4ca84d7ba895a8b71bd620f64
parent 812548 a009b5249a4b78a889fdc5ffcf55ad51715cc686
push id115615
push userbmo:johannes.willbold@rub.de
push dateMon, 09 Jul 2018 18:23:50 +0000
reviewersdminor
bugs1433529
milestone63.0a1
Bug 1433529: Fixed TODOs in parse_sdp, r=dminor Changed parse_sdp to use StringView instad of raw pointers Fixed all TODOs by using the existing StringView::into trait instead of doing a manual string conversion. MozReview-Commit-ID: 5m7rLAu8vwS
media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp
media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs
--- a/media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
+++ b/media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
@@ -251,17 +251,17 @@ nsresult u32_vec_get(const U32Vec* vec, 
 size_t u16_vec_len(const U16Vec* vec);
 nsresult u16_vec_get(const U16Vec* vec, size_t index, uint16_t* ret);
 
 size_t u8_vec_len(const U8Vec* vec);
 nsresult u8_vec_get(const U8Vec* vec, size_t index, uint8_t* ret);
 
 void sdp_free_string(char* string);
 
-nsresult parse_sdp(const char* sdp, uint32_t length, bool fail_on_warning,
+nsresult parse_sdp(StringView sdp, bool fail_on_warning,
                    RustSdpSession** ret, RustSdpError** err);
 RustSdpSession* sdp_new_reference(RustSdpSession* aSess);
 void sdp_free_session(RustSdpSession* ret);
 size_t sdp_get_error_line_num(const RustSdpError* err);
 StringView sdp_get_error_message(const RustSdpError* err);
 void sdp_free_error(RustSdpError* err);
 
 RustSdpOrigin sdp_get_origin(const RustSdpSession* aSess);
--- a/media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp
+++ b/media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp
@@ -17,32 +17,25 @@
 
 namespace mozilla
 {
 
 UniquePtr<Sdp>
 RsdparsaSdpParser::Parse(const std::string &sdpText)
 {
   ClearParseErrors();
-  const char* rawString = sdpText.c_str();
   RustSdpSession* result;
   RustSdpError* err;
-  nsresult rv = parse_sdp(rawString, sdpText.length() + 1, false,
-                          &result, &err);
+  StringView sdpTextView{sdpText.c_str(), sdpText.length()};
+  nsresult rv = parse_sdp(sdpTextView, false, &result, &err);
   if (rv != NS_OK) {
-    // TODO: err should eventually never be null if rv != NS_OK
-    // see Bug 1433529
-    if (err != nullptr) {
-      size_t line = sdp_get_error_line_num(err);
-      std::string errMsg = convertStringView(sdp_get_error_message(err));
-      sdp_free_error(err);
-      AddParseError(line, errMsg);
-    } else {
-      AddParseError(0, "Unhandled Rsdparsa parsing error");
-    }
+    size_t line = sdp_get_error_line_num(err);
+    std::string errMsg = convertStringView(sdp_get_error_message(err));
+    sdp_free_error(err);
+    AddParseError(line, errMsg);
     return nullptr;
   }
 
   if(err) {
     size_t line = sdp_get_error_line_num(err);
     std::string warningMsg = convertStringView(sdp_get_error_message(err));
     sdp_free_error(err);
     AddParseWarnings(line, warningMsg);
--- a/media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs
@@ -1,15 +1,14 @@
 extern crate rsdparsa;
 extern crate libc;
 #[macro_use] extern crate log;
 extern crate nserror;
 
-use std::ffi::CStr;
-use std::{str, slice, ptr};
+use std::ptr;
 use std::os::raw::c_char;
 use std::error::Error;
 
 use libc::size_t;
 
 use std::rc::Rc;
 
 use nserror::{nsresult, NS_OK, NS_ERROR_INVALID_ARG};
@@ -23,44 +22,33 @@ pub mod network;
 pub mod attribute;
 pub mod media_section;
 
 pub use types::{StringView, NULL_STRING};
 use network::{RustSdpOrigin, origin_view_helper, RustSdpConnection,
               get_bandwidth};
 
 #[no_mangle]
-pub unsafe extern "C" fn parse_sdp(sdp: *const u8, length: u32,
+pub unsafe extern "C" fn parse_sdp(sdp: StringView,
                                    fail_on_warning: bool,
                                    session: *mut *const SdpSession,
                                    error: *mut *const SdpParserError) -> nsresult {
-    // Bug 1433529 tracks fixing the TODOs in this function.
-    // TODO: Do I need to add explicit lifetime here?
-    // https://gankro.github.io/blah/only-in-rust/#honorable-mention-variance
-    let sdp_slice: &[u8] = slice::from_raw_parts(sdp, length as usize);
-    let sdp_c_str = match CStr::from_bytes_with_nul(sdp_slice) {
+    let sdp_str = match sdp.into() {
         Ok(string) => string,
-        Err(_) => {
+        Err(boxed_error) => {
             *session = ptr::null();
-            *error = ptr::null(); // TODO: Give more useful return value here
-            debug!("Error converting string");
+            *error = Box::into_raw(Box::new(SdpParserError::Sequence {
+                message: (*boxed_error).description().to_string(),
+                line_number: 0,
+            }));
             return NS_ERROR_INVALID_ARG;
         }
     };
-    let sdp_buf: &[u8] = sdp_c_str.to_bytes();
-    let sdp_str_slice: &str = match str::from_utf8(sdp_buf) {
-        Ok(string) => string,
-        Err(_) => {
-            *session = ptr::null();
-            *error = ptr::null(); // TODO: Give more useful return value here
-            debug!("Error converting string to utf8");
-            return NS_ERROR_INVALID_ARG;
-        }
-    };
-    let parser_result = rsdparsa::parse_sdp(sdp_str_slice, fail_on_warning);
+
+    let parser_result = rsdparsa::parse_sdp(&sdp_str, fail_on_warning);
     match parser_result {
         Ok(parsed) => {
             *error = match parsed.warnings.len(){
                 0 => ptr::null(),
                 _ => Box::into_raw(Box::new(parsed.warnings[0].clone())),
             };
             *session = Rc::into_raw(Rc::new(parsed));
             NS_OK