Closed
Bug 1495261
Opened 2 years ago
Closed 2 years ago
Watch for exceptions being thrown while stringifying old exceptions
Categories
(Core Graveyard :: Web Replay, enhancement)
Core Graveyard
Web Replay
Tracking
(firefox64 fixed)
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: bhackett1024, Assigned: bhackett1024)
Details
Attachments
(1 file)
812 bytes,
patch
|
loganfsmyth
:
review+
|
Details | Diff | Splinter 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 1•2 years ago
|
||
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•2 years ago
|
||
Comment on attachment 9013122 [details] [diff] [review] patch Changing review per discussion with :jlast.
Attachment #9013122 -
Flags: review?(jimb) → review?(lsmyth)
Comment 3•2 years ago
|
||
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+
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•2 years 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•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/db73000c424b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•1 year ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•