Made all "free" extern "C" APIs in simpleservo fallible by returning …

[angelortiz1007]

…null pointr or boolean value. ServoApp will need to be modified to check return values.

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #23514 (GitHub issue number if applicable)

• There are tests for these changes OR
• These changes do not require tests because ServoApp will test/verify._

---

This change is 

[jdm]

r? @jdm

[CYBAI]

nit: we can just use result.is_ok() (Same nit for other pattern matchings with return boolean below)

btw, maybe we don't need underscore prefix for result? 👀

[CYBAI]

maybe no need to return explicitly while it's in the end of function 👀

match result {
  Ok(_) => ptr
  Err(_) => std::ptr::null()
}

[angelortiz1007]

Thanks for the comments. Can I propose to do the following based on your comments?

1. remove the "_" from the variable "_result".
2. in the match{} statements remove the explicit "returns"
3. After 1&2, I commit changes. I can squash the changes with the previous commit as well.
4. run mach test-tidy and mach fmt if needed.

[CYBAI]

Thanks for the comments. Can I propose to do the following based on your comments?

1. remove the "_" from the variable "_result".

2. in the match{} statements remove the explicit "returns"

3. After 1&2, I commit changes.  I can squash the changes with the previous commit as well.

4. run mach test-tidy and mach fmt if needed.

Yes! Sure! Actually, 1 & 2 are what I thought :D!

And 3 & 4 also sounds great to me! Thanks!

[jdm]

Good start!

[jdm]

Rather than using a mutable variable, we can do the following:

let result = panic::catch_unwind(AssertUnwindSafe(|| {
    let v = simpleservo::servo_version();
    let text = CString::new(v).expect("Can't create string");
    let ptr = text.as_ptr();
    mem::forget(text)
    ptr
}));

match result {
    Ok(ptr) => ptr,
    Err(_) => std::ptr::null(),
}

The return value of catch_unwind contains the return value of the closure that is invoked by catch_unwind, so we don't need to do any special communication with the environment outside of the closure to extract it.

[angelortiz1007]

Thank you. Good suggestion and better understanding.

[jdm]

Given how frequently we repeat this pattern of catch_unwind followed by checking its return value, we can make a helper to reduce some duplication:

fn catch_any_panic<F: FnOnce() + UnwindSafe>(function: F) -> bool {
    panic::catch_unwind(function).is_ok()
}

Then we can write the calling code like:

extern "C" fn perform_updates() -> bool {
    catch_any_panic(|| {
        debug!("perform_updates");
        call(|s| s.perform_updates());
    })
}

This will make it easier to maintain in the future if we want to add additional behaviour for logging information about panics, for example.

[jdm]

I modeled this idea after this code we wrote a few years ago for a similar purpose.

[angelortiz1007]

OK. I will make the changes. I was under the impression/misunderstanding on .is_ok() will return true when there is no panic, but was wondering how you return false if no explicit statement to do so.

[jdm]

Result::is_ok() returns true if the value is Ok, and false if the value is Err. panic::catch_unwind returns Result<???> where ??? is the return type of the closure that it invokes, so Ok if there is no panic and Err otherwise. Therefore, catch_any_panic returns true if no panic occurred inside the closure passed to panic::catch_unwind.

[angelortiz1007]

Can the generic function "catch_any_panic()" further be generalized to return a type based on the function signature?

I typed this up, doesn't build, but feel I'm close.

fn catch_any_panic<F: FnOnce() + UnwindSafe>(function: F) -> Result<T, E> {
let result = panic::catch_unwind(function);

match result {
    OK(T) => T,
    Err(E) => E,
}

}

[jdm]

It could (that's what this code does), but then we end up having all of the callers that need a boolean return value have to do extra processing. It's not clear to me what the value of this change would be.

[angelortiz1007]

I was thinking about the function pub extern "C" fn servo_version() -> *const c_char {} which returns a *const c_char. I was trying to fit the catch_any_panic() to servo_version() too.

[angelortiz1007]

I've got all the suggestions incorporated into my repository. I will run a few tests, add, commit and push.

My question can be deferred.

[jdm]

@bors-servo r+
Thanks!

[bors-servo]

📌 Commit ef4d579 has been approved by jdm

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit dde4f71 has been approved by jdm

[bors-servo]

⌛ Testing commit dde4f71 with merge 55983c3...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

• #23920

[bors-servo]

⌛ Testing commit dde4f71 with merge d93b652...

[bors-servo]

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing d93b652 to master...