Bug 1433529: Fixed TODOs in parse_sdp, r=dminor
authorJohannes Willbold <j.willbold@mozilla.com>
Fri, 29 Jun 2018 15:03:58 -0700
changeset 425690 607e9d3256b0d6679015565dd07335551181b709
parent 425674 bb08bc2f29568bb81840e45115107dd2fcd22c67
child 425691 ec17ed2e91b91afd97aa0db99be5d24b2500d6c2
push id34262
push usercsabou@mozilla.com
push dateTue, 10 Jul 2018 21:51:50 +0000
treeherdermozilla-central@70f901964f97 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdminor
bugs1433529
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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