Closed Bug 1265045 Opened 5 years ago Closed 5 years ago

Truncate error messages sent in ConsoleListener::Observe()

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s ? ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [MemShrink] btpp-active)

Attachments

(1 file)

Apparently this IPC message can be quite large sometimes. I found this crash where IPC is trying to grow the message to 1.6mb:

https://crash-stats.mozilla.com/report/index/d37d0674-c0f9-4fb7-911b-57cf92160414

According to telemetry, it is very rare for this message to get this large, but it would also be extremely easy to fix, so we should just do that. sourceName and sourceLine are already truncated, so it must one one of the other parts. Honestly, we should just define a function and truncate everything. A few extra branches isn't going to hurt us.
As I said, not a huge win, but it will take somebody 10 minutes to do.
Assignee: nobody → continuation
The only additional string that needs to be truncated is msg, from GetErrorMessage. The category is also a string, but it is some kind of wrapped C string, so truncated it will be harder, and every place that sets it just uses some small constant string like "CSP" so it should not be an issue.
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2204bc583a9e

I just pushed the try run, but this seems low risk, so I'll ask for review now.
Attachment #8741943 - Flags: review?(amarchesini)
Comment on attachment 8741943 [details] [diff] [review]
Truncate the result of GetErrorMessage() before sending it to the parent.

Review of attachment 8741943 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +425,5 @@
>  
>  NS_IMPL_ISUPPORTS(ConsoleListener, nsIConsoleListener)
>  
> +static void
> +TruncateString(nsString& aString)

nsAString... ? I don't think it changes too much actually.

@@ +427,5 @@
>  
> +static void
> +TruncateString(nsString& aString)
> +{
> +  if (aString.Length() > 1000) {

Can you move the comment here as well? Or a short version of it.
Attachment #8741943 - Flags: review?(amarchesini) → review+
Comment on attachment 8741943 [details] [diff] [review]
Truncate the result of GetErrorMessage() before sending it to the parent.

Review of attachment 8741943 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +441,5 @@
>    }
>  
>    nsCOMPtr<nsIScriptError> scriptError = do_QueryInterface(aMessage);
>    if (scriptError) {
>      nsString msg, sourceName, sourceLine;

nsAutoString... ?
(In reply to Andrea Marchesini (:baku) from comment #5)
> nsAutoString... ?

I filed bug 1265045 for that.

(In reply to Andrea Marchesini (:baku) from comment #4)
> nsAString... ? I don't think it changes too much actually.
Fixed.

> Can you move the comment here as well? Or a short version of it.
I moved the comment up. It also makes the diff nicer looking.
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink] btpp-active
https://hg.mozilla.org/mozilla-central/rev/f1f07fcd876c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8741943 [details] [diff] [review]
Truncate the result of GetErrorMessage() before sending it to the parent.

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: some OOM crashes, though probably not very common
[Describe test coverage new/current, TreeHerder]: this code probably runs frequently
[Risks and why]: low: this just makes one error message shorter (1000 characters)
[String/UUID change made/needed]: none
Attachment #8741943 - Flags: approval-mozilla-aurora?
Comment on attachment 8741943 [details] [diff] [review]
Truncate the result of GetErrorMessage() before sending it to the parent.

Another improvement for e10s OOM crashes situation, Aurora47+
Attachment #8741943 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.