Truncate error messages sent in ConsoleListener::Observe()

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(e10s?, firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

(Whiteboard: [MemShrink] btpp-active)

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
As I said, not a huge win, but it will take somebody 10 minutes to do.
status-firefox46: --- → wontfix
status-firefox47: --- → affected
tracking-e10s: --- → ?
(Assignee)

Updated

3 years ago
Assignee: nobody → continuation
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8741943 [details] [diff] [review]
Truncate the result of GetErrorMessage() before sending it to the parent.

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... ?
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Updated

3 years ago
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink] btpp-active

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1f07fcd876c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 9

3 years ago
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+

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e2c675bcdfa
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.