Watch for exceptions being thrown while stringifying old exceptions

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

9 months ago
Posted 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.

https://crash-stats.mozilla.com/report/index/f98fff8e-fd44-4937-ae6d-119ea0180929

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]
patch

We might want to leave a comment in the code to the bug so that it is clear that this addresses a regression.
Assignee

Comment 2

9 months ago
Comment on attachment 9013122 [details] [diff] [review]
patch

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

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+

Comment 4

9 months ago
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db73000c424b
Watch for exceptions being thrown while stringifying old exceptions, r=lsmyth.
Assignee

Comment 5

9 months ago
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.

Comment 6

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db73000c424b
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1500530

Updated

8 months ago
No longer depends on: 1500530
You need to log in before you can comment on or make changes to this bug.