Closed
Bug 1265045
Opened 8 years ago
Closed 8 years ago
Truncate error messages sent in ConsoleListener::Observe()
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Whiteboard: [MemShrink] btpp-active)
Attachments
(1 file)
2.56 KB,
patch
|
baku
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
As I said, not a huge win, but it will take somebody 10 minutes to do.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 2•8 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•8 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 4•8 years ago
|
||
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 5•8 years ago
|
||
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•8 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•8 years ago
|
Whiteboard: [MemShrink]
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink] btpp-active
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1f07fcd876c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 9•8 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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e2c675bcdfa
You need to log in
before you can comment on or make changes to this bug.
Description
•