Closed Bug 1263774 Opened 8 years ago Closed 8 years ago

No memory reports in content process crash reports

Categories

(Toolkit :: Crash Reporting, defect, P1)

Product:
Toolkit
Component:
Crash Reporting
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
e10s + ---
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: bytesized)

References

Details

(Whiteboard: [fce-active-legacy])

Attachments

(1 file)

Bug 1263774 - Include memory reports in content process crash reports
(deleted), text/x-review-board-request
mccr8
: review+
bugzilla
: review+
lizzard
: approval-mozilla-aurora+
Details 
Similar to bug 1236108, it appears that memory reports for content process crashes don't have memory reports. I did a super search on crash-stats for beta crashes where "contains memory report" exists, and "process type" is "content" but it turned up no reports. It is hard to say how useful they will be for investigating the e10s OOM problems, but they might be useful.
I should actually double check that these aren't present in Nightly with a similar crash-stats search, in case there's something that has fixed this already that just hasn't made it back to beta.
Flags: needinfo?(continuation)
There are two parts to this:

* Collecting the memory report has to be triggered. That has to happen in the chrome process, but since it's the content process that's running out of memory that will need to be proxied somehow.
* Associating the memory report with the content process crash. Currently this only happens in the main process crash path here: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#804
So adding the ContainsMemoryReport annotation as well as copying the memory report file around to the proper place. I'm not sure whether that's TakeMinidump or some other place where we stage minidumps to the correct pending location.

David, can somebody on your team help with this? I can mentor.
Flags: needinfo?(ddurst)
I couldn't remember how these got generated, so I looked. We have this nice SaveMemoryReportNearOOM function that apparently only works on Windows but checks the available virtual memory, and if it goes below some low-water mark (200MB) we write out a memory report:
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/xpcom/threads/nsThread.cpp#453

This is called from nsThread::ProcessNextEvent when we're on the main thread:
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/xpcom/threads/nsThread.cpp#936

That calls nsXULAppInfo::SaveMemoryReport:
https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1352

which winds up calling CrashReporter::SetMemoryReportFile:
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/toolkit/crashreporter/nsExceptionHandler.cpp#2681

which just saves the path in a global variable in nsExceptionHandler.cpp. So then in the in-process case, in MinidumpCallback, if that global is set we copy the file to the pending directory and set an annotation indicating that a memory report is present:
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/toolkit/crashreporter/nsExceptionHandler.cpp#845

However! Child process crashes are written by the parent process, and they hit a different callback, OnChildProcessDumpRequested, which doesn't do that:
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/toolkit/crashreporter/nsExceptionHandler.cpp#3101

There's also code in CrashReporterParent that handles adding some extra annotations after the minidump has been written (it gets called via the "child process exited" callback):
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/ipc/CrashReporterParent.cpp#114
(In reply to Andrew McCreight [:mccr8] from comment #1)
> I should actually double check that these aren't present in Nightly with a
> similar crash-stats search, in case there's something that has fixed this
> already that just hasn't made it back to beta.

I looked, and I didn't see any on Nightly either.
Flags: needinfo?(continuation)
I believe I can start working on this tomorrow.
Assignee: nobody → ksteuber
Flags: needinfo?(ddurst)
tracking-e10s: ? → +
Priority: -- → P1
FYI, I haven't found any evidence that this works in the parent process, either.
Depends on: 1266601
Ok, I take that back, I do see at least one report with a memory report. I think erahm's issue is that it is OOM crashing while generating the memory report or something.
No longer depends on: 1266601
Kirk, are you still looking at this? It would be pretty useful for triaging e10s OOMs.
Flags: needinfo?(ksteuber)
I am still looking at it. I will be uploading a patch shortly.
Flags: needinfo?(ksteuber)
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49305/diff/1-2/
Attachment #8746200 - Flags: review?(ted)
Attachment #8746200 - Flags: review?(continuation)
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

https://reviewboard.mozilla.org/r/49305/#review46507

Thanks for fixing this.

I didn't review the toolkit/ changes, as I assume Ted will look at that.

::: dom/ipc/PContent.ipdl:1195
(Diff revision 2)
>      async NotifyBenchmarkResult(nsString aCodecName, uint32_t aDecodeFPS);
>  
> +    /**
> +     * Tell the parent process that the child process is low on memory. This
> +     * allows the parent process to save a memory report that can potentially be
> +     * sent with a crash report from the content process

nit: missing a period at the end.

::: dom/ipc/PContent.ipdl:1197
(Diff revision 2)
> +    /**
> +     * Tell the parent process that the child process is low on memory. This
> +     * allows the parent process to save a memory report that can potentially be
> +     * sent with a crash report from the content process
> +     */
> +     async NotifyLowMemory();

Maybe something like NotifyRequestMemoryReport? This name sounds a bit general.

::: xpcom/threads/nsThread.h:82
(Diff revision 2)
>    void ShutdownComplete(struct nsThreadShutdownContext* aContext);
>  
>    void WaitForAllAsynchronousShutdowns();
>  
> +#ifdef MOZ_CRASHREPORTER
> +  static bool

nit: in a header file, the declaration should all be on one line.

::: xpcom/threads/nsThread.h:83
(Diff revision 2)
>  
>    void WaitForAllAsynchronousShutdowns();
>  
> +#ifdef MOZ_CRASHREPORTER
> +  static bool
> +  SaveMemoryReportNearOOM(bool aForceReport);

Please define an enum for the argument, like maybe kForceReport and kMaybeReport or something. Passing in a bare "true" or "false" is confusing when looking at a call sites.

::: xpcom/threads/nsThread.cpp:459
(Diff revision 2)
> -static bool SaveMemoryReportNearOOM()
> +// Memory usage will not be checked more than every 30 seconds or saved more
> +// than every 3 minutes
> +// If |aForceReport == true|, a report will be saved regardless of whether the
> +// process is low on memory or not. However, it will still not be saved if
> +// a report was saved less than 3 minutes ago.
> +bool nsThread::SaveMemoryReportNearOOM(bool aForceReport)

Why does this return a boolean if nobody checks it? You might as well just skip that. I guess this is existing code, so feel free to leave it alone.

nit: while you are here, please make the return type on a separate line than the rest of the function name.

::: xpcom/threads/nsThread.cpp:468
(Diff revision 2)
> +  const size_t kLowMemoryCheckSeconds = 30;
> +  const size_t kLowMemorySaveSeconds = 3 * 60;
> +
> +  static TimeStamp nextCheck = TimeStamp::NowLoRes()
> +    + TimeDuration::FromSeconds(kLowMemoryCheckSeconds);
> +  static bool justSavedReport = false;

How useful is justSavedReport, out of curiousity? It looks like it just delays things by a single check, however long that is.

::: xpcom/threads/nsThread.cpp:496
(Diff revision 2)
> -    }
> +      }
> -  }
> +    }
> +  }
>  #endif
>  
> -  if (needMemoryReport) {
> +  if (needMemoryReport || aForceReport) {

This will start collecting memory reports on non-Windows platforms, while we didn't before. Hopefully this won't cause any issues. You could add an else to the XP_WIN branch that sets needMemoryReport to true if you want to be conservative.

::: xpcom/threads/nsThread.cpp:498
(Diff revision 2)
> +  }
>  #endif
>  
> -  if (needMemoryReport) {
> +  if (needMemoryReport || aForceReport) {
> +    if (XRE_IsContentProcess()) {
> +      // The Content process cannot save a memory report itself. It needs the

nit: "Content" shouldn't be capitalized. The comment doesn't seem too useful but feel free to leave it if you want.
Attachment #8746200 - Flags: review?(continuation) → review+
https://reviewboard.mozilla.org/r/49305/#review46507

> Maybe something like NotifyRequestMemoryReport? This name sounds a bit general.

As discussed, `NotifyRequestMemoryReport` seems too close to the existing `PMemoryReportRequest`

> How useful is justSavedReport, out of curiousity? It looks like it just delays things by a single check, however long that is.

`justSavedReport` allows us to determine if a report was saved the last time `SaveMemoryReportNearOOM()` was run. Without this, we may have problems in this situation: There are multiple content processes running and free physical memory dips below 200MB. This causes all content processes to call into the parent process around the same time requesting a memory report. With `justSavedReport`, we can check if a report has been saved in the last 3 minutes so that multiple messages sent to the parent process result in just one memory report.

> This will start collecting memory reports on non-Windows platforms, while we didn't before. Hopefully this won't cause any issues. You could add an else to the XP_WIN branch that sets needMemoryReport to true if you want to be conservative.

Without further changes, this will continue to collect memory reports on Windows platforms only. `needMemoryReport` can only be set to `true` in Windows (as before). The only current line of code that forces a memory report is `ContentParent::RecvNotifyLowMemory()`, which is only called in `SaveMemoryReportNearOOM()` and only when `needMemoryReport == true`.
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49305/diff/2-3/
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

I'm sorry, I didn't find time to review this and I'm leaving for a week of vacation. bsmedberg can help you find a suitable reviewer (or review it himself), perhaps aklotz?
Attachment #8746200 - Flags: review?(ted)
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49305/diff/3-4/
Attachment #8746200 - Flags: review?(aklotz)
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

https://reviewboard.mozilla.org/r/49305/#review47655

Looks good, mostly nits here... I'd like to see another revision before giving the OK.

::: toolkit/crashreporter/nsExceptionHandler.cpp:2937
(Diff revision 4)
>  
>      WriteAnnotation(fd,
>                      nsDependentCString("UptimeTS"),
>                      nsDependentCString(uptimeTSString));
> +
> +    if (memoryReportPath) {

This code is positioned inside the if (writeCrashTime) block. I think you want it outside and below that block.

::: toolkit/crashreporter/nsExceptionHandler.cpp:3113
(Diff revision 4)
>      return false;
>    }
>  
> +  if (memoryReport) {
> +    nsAutoString leafName;
> +    nsAutoString extension;

Since you're just passing memoryReportExtension straight into Replace() without changes, skip this intermediate varaible and just pass it in directly.

In other words, leafName.Replace(leafName.Length() - 4, 4, CONVERT_XP_CHAR_TO_UTF16(memoryReportExtension));

::: toolkit/crashreporter/nsExceptionHandler.cpp:3116
(Diff revision 4)
> +  if (memoryReport) {
> +    nsAutoString leafName;
> +    nsAutoString extension;
> +    extension = CONVERT_XP_CHAR_TO_UTF16(memoryReportExtension);
> +    nsresult rv = dumpFile->GetLeafName(leafName);
> +    if (NS_FAILED(rv))

Curly braces around if block in new code, please.

::: toolkit/crashreporter/nsExceptionHandler.cpp:3169
(Diff revision 4)
>                                         ArrayLength(kSubprocessBlacklist)),
>                               getter_AddRefs(extraFile)))
>      return;
>  
> -  if (ShouldReport())
> -    MoveToPending(minidump, extraFile);
> +  if (ShouldReport()) {
> +    nsCOMPtr<nsIFile> memoryReport = nullptr;

Nit: nsCOMPtr automatically initializes to null, so you don't need to do so here.

::: toolkit/crashreporter/nsExceptionHandler.cpp:3172
(Diff revision 4)
>  
> -  if (ShouldReport())
> -    MoveToPending(minidump, extraFile);
> +  if (ShouldReport()) {
> +    nsCOMPtr<nsIFile> memoryReport = nullptr;
> +    if (memoryReportPath) {
> +      CreateFileFromPath(memoryReportPath, getter_AddRefs(memoryReport));
> +    }

MoveToPending is OK if !memoryReport because you have a check inside there. OTOH, it might be a good idea to add an assertion here to ensure that if memoryReportPath is valid then memoryReport is valid as well. Unless of course that isn't always the case... but if so, please add the assertion.

::: toolkit/crashreporter/nsExceptionHandler.cpp:3459
(Diff revision 4)
>    }
>  
>    FindPendingDir();
>  
>    // Move {dump,extra} to pending folder
> -  if (!MoveToPending(lastMinidumpFile, lastExtraFile)) {
> +  if (!MoveToPending(lastMinidumpFile, lastExtraFile, nullptr)) {

Is it possible for a memory report to be lying around in this case?

This code works just fine, but I think that if it is possible for a memory report file to also be left behind from a previous run, we should grab it.

::: xpcom/threads/nsThread.h:82
(Diff revision 4)
>    void ShutdownComplete(struct nsThreadShutdownContext* aContext);
>  
>    void WaitForAllAsynchronousShutdowns();
>  
> +#ifdef MOZ_CRASHREPORTER
> +  enum ShouldSaveMemoryReport {

I'd suggest using "enum class" instead of "enum" here so that we can get some C++11 type safety.

::: xpcom/threads/nsThread.cpp:464
(Diff revision 4)
> +  const size_t kLowMemoryCheckSeconds = 30;
> +  const size_t kLowMemorySaveSeconds = 3 * 60;
> +
> +  static TimeStamp nextCheck = TimeStamp::NowLoRes()
> +    + TimeDuration::FromSeconds(kLowMemoryCheckSeconds);
> +  static bool justSavedReport = false;

Please rename this to "recentlySavedReport" or something, I think that would be a bit more clear.

::: xpcom/threads/nsThread.cpp:468
(Diff revision 4)
> +    + TimeDuration::FromSeconds(kLowMemoryCheckSeconds);
> +  static bool justSavedReport = false;
> +
> +  // Are we checking again too soon?
> +  TimeStamp now = TimeStamp::NowLoRes();
> +  if (aShouldSave == kForceReport) {

You can simplify this logic if you write

if (aShouldSave == kForceReport && justSavedReport && now < nextCheck) {
  return false;
}

then you can move the code in the else case outside of the else block and eliminate the else.

Our coding style guide forbids else after a return, so please fix.

::: xpcom/threads/nsThread.cpp:478
(Diff revision 4)
> +    if (now < nextCheck) {
> +      return false;
> +    }
> +  }
>  
> +  bool needMemoryReport = false;

Initialize needMemoryReport to aShouldSave == kForceRequest...

::: xpcom/threads/nsThread.cpp:492
(Diff revision 4)
> -    }
> +      }
> -  }
> +    }
> +  }
>  #endif
>  
> -  if (needMemoryReport) {
> +  if (needMemoryReport || aShouldSave == kForceReport) {

...now since you've changed needMemoryReport to reflect the value of aShouldSave, you don't need the OR here anymore.

::: xpcom/threads/nsThread.cpp:495
(Diff revision 4)
>  #endif
>  
> -  if (needMemoryReport) {
> +  if (needMemoryReport || aShouldSave == kForceReport) {
> +    if (XRE_IsContentProcess()) {
> +      dom::ContentChild* cc = dom::ContentChild::GetSingleton();
> +      cc->SendNotifyLowMemory();

Wrap the SendNotifyLowMemory() call with if (cc) {} please.

::: xpcom/threads/nsThread.cpp:505
(Diff revision 4)
> -      cr->SaveMemoryReport();
> +        cr->SaveMemoryReport();
> -    }
> +      }
> -  }
> +    }
> +    justSavedReport = true;
> +    nextCheck = now + TimeDuration::FromSeconds(kLowMemorySaveSeconds);
> +  } else { // !needMemoryReport && !aForceReport

Since we're simplifying the condition of the if block, you can eliminate this comment.
Attachment #8746200 - Flags: review?(aklotz)
https://reviewboard.mozilla.org/r/49305/#review47655

> You can simplify this logic if you write
> 
> if (aShouldSave == kForceReport && justSavedReport && now < nextCheck) {
>   return false;
> }
> 
> then you can move the code in the else case outside of the else block and eliminate the else.
> 
> Our coding style guide forbids else after a return, so please fix.

Actually, you can simplify this whole thing if you take advantage of short-circuit evaluation by writing:

if ((aShouldSave != kForceReport || justSavedReport) && now < nextCheck) {
  return false;
}
https://reviewboard.mozilla.org/r/49305/#review47655

> Actually, you can simplify this whole thing if you take advantage of short-circuit evaluation by writing:
> 
> if ((aShouldSave != kForceReport || justSavedReport) && now < nextCheck) {
>   return false;
> }

Even better to eliminate some negation:

if ((aShouldSave == kMaybeReport || justSavedReport) && now < nextCheck) {
  return false;
}
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49305/diff/4-5/
Attachment #8746200 - Flags: review?(aklotz)
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

https://reviewboard.mozilla.org/r/49305/#review50034

Looks good, thanks for your patience! Just one major issue left and we should be good to go.

::: toolkit/crashreporter/nsExceptionHandler.cpp:177
(Diff revision 5)
>  #endif
>  
>  static const XP_CHAR childCrashAnnotationBaseName[] = XP_TEXT("GeckoChildCrash");
>  static const XP_CHAR extraFileExtension[] = XP_TEXT(".extra");
>  static const XP_CHAR memoryReportExtension[] = XP_TEXT(".memory.json.gz");
> +static nsCOMPtr<nsIFile> defaultMemoryReportFile;

Generally we avoid the use of complex types at global scope because the compiler emits calls to their constructors and destructors during startup and shutdown, which has undesirable effects in the large.

Let's change this to a xpstring* and use CreatePathFromFile and CreateFileFromPath to work with it. See the childProcessTmpDir variable as an example.

::: xpcom/threads/nsThread.h:82
(Diff revision 5)
>    void ShutdownComplete(struct nsThreadShutdownContext* aContext);
>  
>    void WaitForAllAsynchronousShutdowns();
>  
> +#ifdef MOZ_CRASHREPORTER
> +  enum class ShouldSaveMemoryReport {

Nit: Open curly brace on its own line.
Attachment #8746200 - Flags: review?(aklotz)
> Let's change this to a xpstring* and use CreatePathFromFile and CreateFileFromPath to work with it. See the childProcessTmpDir variable as an example.

There's also StaticRefPtr<>. This would lose the COMPtr checking, but that's fairly minor, I think.
(In reply to Andrew McCreight [:mccr8] from comment #22)
> > Let's change this to a xpstring* and use CreatePathFromFile and CreateFileFromPath to work with it. See the childProcessTmpDir variable as an example.
> 
> There's also StaticRefPtr<>. This would lose the COMPtr checking, but that's
> fairly minor, I think.

True. I suggested xpstring for consistency with other path-type variables that we already have in there.
As I explained to ksteuber on IRC: We also tend to avoid using refcounted types for long-lived variables in the crash reporter since most of them live past XPCOM shutdown and we don't want to trigger leak checking.
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49305/diff/5-6/
Attachment #8746200 - Flags: review?(aklotz)
Whiteboard: [fce-active]
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

https://reviewboard.mozilla.org/r/49305/#review51526

A few issues here, but they are minor enough that I'll give you r+ provided that you fix them and flag them as resolved in MozReview.

::: toolkit/crashreporter/nsExceptionHandler.cpp:2764
(Diff revision 6)
> +                                         getter_AddRefs(defaultMemoryReportFile));
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    defaultMemoryReportFile->AppendNative(NS_LITERAL_CSTRING("memory-report.json.gz"));
> +    defaultMemoryReportPath = CreatePathFromFile(defaultMemoryReportFile);

You should add a nullptr check for the result of this call and return an error code if it failed.

::: toolkit/crashreporter/nsExceptionHandler.cpp:3012
(Diff revision 6)
>                      nsDependentCString("UptimeTS"),
>                      nsDependentCString(uptimeTSString));
>    }
>  
> +  if (memoryReportPath) {
> +    WriteAnnotation(fd,

Please use `WriteLiteral(fd, "ContainsMemoryReport=1\n");` instead

::: toolkit/crashreporter/nsExceptionHandler.cpp:3193
(Diff revision 6)
> +      return false;
> +    }
> +    // Generate the correct memory report filename from the dumpFile's name
> +    leafName.Replace(leafName.Length() - 4, 4,
> +                     static_cast<nsString>(CONVERT_XP_CHAR_TO_UTF16(memoryReportExtension)));
> +    memoryReport->MoveTo(pendingDir, leafName);

Please return false if the `MoveTo` failed, similarly to the other `MoveTo` calls in this function.

::: toolkit/crashreporter/nsExceptionHandler.cpp:3532
(Diff revision 6)
>      return false;
>    }
>  
> +  nsCOMPtr<nsIFile> memoryReportFile;
> +  nsresult rv = GetDefaultMemoryReportFile(getter_AddRefs(memoryReportFile));
> +  if (NS_FAILED(rv) || NS_FAILED(memoryReportFile->Exists(&exists)) || !exists) {

Let's omit this `if` block. If `NS_FAILED(rv)` then memoryReportFile will remain null anyway. Generally we try to also avoid `Exists` and just deal appropriately with failure on the actual file operation (in this case, the `MoveTo` inside `MoveToPending`)
Attachment #8746200 - Flags: review?(aklotz) → review+
https://reviewboard.mozilla.org/r/49305/#review51526

> Let's omit this `if` block. If `NS_FAILED(rv)` then memoryReportFile will remain null anyway. Generally we try to also avoid `Exists` and just deal appropriately with failure on the actual file operation (in this case, the `MoveTo` inside `MoveToPending`)

I don't think that we want to do this. If the memory report does not exist (which it commonly will not), then `MoveToPending` will return `false` when it fails to move the nonexistant report. This will cause `CheckForLastRunCrash` to return `false` as well. Callers of `CheckForLastRunCrash` will interpret the `false` return value as "We did not crash on the last run" or "We failed to assemble crash data, so there is nothing to send", rather than "There was a crash, but it did not have a memory report".

If we do remove the `if` block, `MoveToPending`'s return value should probably be independant of its success moving the memory report. There actually might be an argument to be made for this. We probably do not want to prevent the crash report from being sent just because of a failure sending the memory report with it. It could potentially lead to some incorrect `ContainsMemoryReport=1` annotations, but maybe that is acceptable.

What do you think?
Flags: needinfo?(aklotz)
Makes sense. Go ahead and leave that in.
Flags: needinfo?(aklotz)
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49305/diff/6-7/
Attachment #8746200 - Attachment description: MozReview Request: Bug 1263774 - Include memory reports in content process crash reports → Bug 1263774 - Include memory reports in content process crash reports
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/982855e49ded
Include memory reports in content process crash reports. r=mccr8, r=aklotz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/982855e49ded
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
This has been in m-c for a few days and we have some content process crashes with memory reports:

https://crash-stats.mozilla.com/search/?product=Firefox&version=50.0a1&process_type=content&contains_memory_report=%21__null__&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

If the results look potentially useful, I'm ok with trying to uplift this to aurora. More info on OOM crashes sounds good to me. 
We could potentially ask for a second technical review, or extra help in interpreting potential fallout on aurora in the crash-stats or telemetry reports.   bsmedberg, what do you think about uplift at least to aurora?
status-firefox49: --- → affected
Flags: needinfo?(benjamin)
Reading the patch I think it's moderate-to-low risk but any problems would likely show up pretty quickly and obviously. It would be helpful to have this for beta 49 so I think we should uplift.
Flags: needinfo?(benjamin)
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

Approval Request Comment
[Feature/regressing bug #]: Memory reports sent with content process OOM crashes
[User impact if declined]: Diminished ability for developers to fix OOM crashes.
[Describe test coverage new/current, TreeHerder]: Original memory report testing located at toolkit/crashreporter/test/unit/test_crash_with_memory_report.js
[Risks and why]: Possible negative impact on browser performance or regresses in crash reporting.
[String/UUID change made/needed]: None.
Attachment #8746200 - Flags: approval-mozilla-aurora?
Comment on attachment 8746200 [details]
Bug 1263774 - Include memory reports in content process crash reports

More OOM info should be useful for future crash fixes. Please uplift to aurora.
Attachment #8746200 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [fce-active] → [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: