Closed Bug 1212244 Opened 4 years ago Closed 4 years ago

Same-process sendAsyncMessage should throw instead of crashing in case of OOM

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: crash)

Attachments

(1 file)

No description provided.
Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r?smaug
Attachment #8670718 - Flags: review?(bugs)
Comment on attachment 8670718 [details]
MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug

+
+  Telemetry::Accumulate(Telemetry::SAME_PROCESS_MESSAGE_DISPATCH_STATUS, mStatus == NS_OK?0:1);

Looks odd. We send tons of messages, so I wouldn't add telemetry code to that path.
And I don't know what useful information the telemetry even provides.
So, please remove that and the change to Histograms.json


Why we need any change to nsSameProcessAsyncMessageBase::ReceiveMessage?

Rather than adding mStatus member variable, it would be better to add static Create method which returns null if
mData.Copy(aData) fails.
Attachment #8670718 - Flags: review?(bugs) → review-
Well, the idea was to get back through Telemetry the info we had from the Crash reporter, since these cases will most likely map to datalosses and/or partial freezes. I believe that it is useful, don't you?
What would we do with that telemetry data?
Comment on attachment 8670718 [details]
MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug

Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r?smaug
Attachment #8670718 - Flags: review- → review?(bugs)
Comment on attachment 8670718 [details]
MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug

Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r?smaug

For the moment, same-process sendAsyncMessage attempts to copy the serialized data to a string and crashes with OOM if this fails. As shown in bug 1106264, this causes frequent Session Restore crashes. It is probably possible to entirely remove the copy, but there is a good chance that this would only move
the Session Restore crashes to the code that writes data to disk if we do not take precautions to clean up messages that are too long.

This patch converts the OOM into a XPConnect error and introduces telemetry that will let us help determine a threshold for the acceptable size of messages and track the actual resolution of the issue.
Attachment #8670718 - Flags: review?(bugs) → review+
Attachment #8670718 - Flags: review+ → review?(bugs)
Comment on attachment 8670718 [details]
MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug

Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r?smaug

For the moment, same-process sendAsyncMessage attempts to copy the serialized data to a string and crashes with OOM if this fails. As shown in bug 1106264, this causes frequent Session Restore crashes. It is probably possible to entirely remove the copy, but there is a good chance that this would only move
the Session Restore crashes to the code that writes data to disk if we do not take precautions to clean up messages that are too long.

This patch converts the OOM into a XPConnect error and introduces telemetry that will let us help determine a threshold for the acceptable size of messages and track the actual resolution of the issue.
Comment on attachment 8670718 [details]
MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug

Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug
Attachment #8670718 - Attachment description: MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r?smaug → MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug
Attachment #8670718 - Flags: review?(bugs)
Comment on attachment 8670718 [details]
MozReview Request: Bug 1212244 - Same-process sendAsyncMessage can now throw instead of OOM;r=smaug

https://reviewboard.mozilla.org/r/21441/#review19591
Attachment #8670718 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d1d48d269ce5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.