1495261 - Watch for exceptions being thrown while stringifying old exceptions

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Jason Laster [:jlast]]

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.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 9013122 [details] [diff] [review]
patch

Changing review per discussion with :jlast.

Comment 3

[Logan Smyth [:loganfsmyth]]

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";
        }
    }

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db73000c424b
Watch for exceptions being thrown while stringifying old exceptions, r=lsmyth.

Comment 5

[Brian Hackett [Laid off!]]

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

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/db73000c424b