Skip to content

Commit

Permalink
Bug 1536850 - Properly detect when the browser is shutting down to av…
Browse files Browse the repository at this point in the history
…oid taking content process minidumps too late r=Ehsan a=pascalc

This code was already present but it had two flaws: the first one was that the
variable used to prevent minidumps from being taken when shutting down was not
being initialized which caused it to be true most of the time and thus
prevented captures even in valid scenarios. The second was that we relied on
the "profile-before-change" change event to detect shutdown but this often
happens after we've already started closing tabs and shutting down processes
thus the minidump generation would often race against it.

Differential Revision: https://phabricator.services.mozilla.com/D24691
  • Loading branch information
gabrielesvelto committed Mar 29, 2019
1 parent 3d10766 commit db9d19a
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 5 deletions.
11 changes: 7 additions & 4 deletions dom/ipc/ContentParent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@
#include "nsFrameMessageManager.h"
#include "nsHashPropertyBag.h"
#include "nsIAlertsService.h"
#include "nsIAppStartup.h"
#include "nsIClipboard.h"
#include "nsICookie.h"
#include "nsContentPermissionHelper.h"
Expand Down Expand Up @@ -2956,8 +2957,6 @@ ContentParent::Observe(nsISupports* aSubject, const char* aTopic,
const char16_t* aData) {
if (mSubprocess && (!strcmp(aTopic, "profile-before-change") ||
!strcmp(aTopic, "xpcom-shutdown"))) {
mShuttingDown = true;

// Make sure that our process will get scheduled.
ProcessPriorityManager::SetProcessPriority(this,
PROCESS_PRIORITY_FOREGROUND);
Expand Down Expand Up @@ -3377,7 +3376,11 @@ void ContentParent::GeneratePairedMinidump(const char* aReason) {
// Something has gone wrong to get us here, so we generate a minidump
// of the parent and child for submission to the crash server unless we're
// already shutting down.
if (mCrashReporter && !mShuttingDown &&
nsCOMPtr<nsIAppStartup> appStartup = components::AppStartup::Service();
bool shuttingDown = false;
if (mCrashReporter &&
!(NS_SUCCEEDED(appStartup->GetShuttingDown(&shuttingDown)) &&
shuttingDown) &&
Preferences::GetBool("dom.ipc.tabs.createKillHardCrashReports", false)) {
// GeneratePairedMinidump creates two minidumps for us - the main
// one is for the content process we're about to kill, and the other
Expand Down Expand Up @@ -5735,7 +5738,7 @@ mozilla::ipc::IPCResult ContentParent::RecvAttachBrowsingContext(

if (!child) {
RefPtr<BrowsingContextGroup> group =
BrowsingContextGroup::Select(aInit.mParentId, aInit.mOpenerId);
BrowsingContextGroup::Select(aInit.mParentId, aInit.mOpenerId);
child = BrowsingContext::CreateFromIPC(std::move(aInit), group, this);
}

Expand Down
1 change: 0 additions & 1 deletion dom/ipc/ContentParent.h
Original file line number Diff line number Diff line change
Expand Up @@ -1229,7 +1229,6 @@ class ContentParent final : public PContentParent,

LifecycleState mLifecycleState;

bool mShuttingDown;
bool mIsForBrowser;

// Whether this process is recording or replaying its execution, and any
Expand Down

0 comments on commit db9d19a

Please sign in to comment.