Truncate error messages sent in ConsoleListener::Observe()

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

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.
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
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
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+
You need to log in before you can comment on or make changes to this bug.