Closed Bug 1495261 Opened 3 years ago Closed 3 years ago

Watch for exceptions being thrown while stringifying old exceptions


(Core Graveyard :: Web Replay, enhancement)

Not set


(firefox64 fixed)

Tracking Status
firefox64 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)



(1 file)

Attached patch patchSplinter Review
There have been some Web Replay crashes (reported by :jlast, sample below) that happened because the ProcessRequest JS handler threw an exception, after which the caller (ProcessRequest in toolkit/recordreplay/ipc/JSControl.cpp) immediately crashes.

ProcessRequest has a big try/catch block surrounding its logic, so the only ways it should fail are if an uncatchable exception was thrown, or if the catch block itself threw an exception.  I don't think the former is possible, as we immediately abort if a recording/replaying process throws an OOM or over-recursion error.  The latter is definitely possible, as the thrown exception is converted to a string outside a try/catch block.

The attached patch puts a try/catch block around this stringification, which will hopefully take care of this.
Attachment #9013122 - Flags: review?(jimb)
Comment on attachment 9013122 [details] [diff] [review]

We might want to leave a comment in the code to the bug so that it is clear that this addresses a regression.
Comment on attachment 9013122 [details] [diff] [review]

Changing review per discussion with :jlast.
Attachment #9013122 - Flags: review?(jimb) → review?(lsmyth)
Comment on attachment 9013122 [details] [diff] [review]

This looks good to me. My only thought was that it might be good to try to get `e.message` if `"" + e` fails, but without knowing what actually triggers this case, it's hard to know if it would be worth it, e.g.

    try {
        errorMessage = "" + e;
    } catch (e1) {
        try {
            errorMessage = e.message;
        } catch (e2) {
            errorMessage = "Unknown";
Attachment #9013122 - Flags: review?(lsmyth) → review+
Pushed by
Watch for exceptions being thrown while stringifying old exceptions, r=lsmyth.
I went with the simpler version of the patch, since, yeah, there isn't much information about what is going on here and what additional checks would be useful.
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1500530
No longer depends on: 1500530
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.