Skip to content

Commit

Permalink
Remove Crash Keys sent to the renderer for hung renderers.
Browse files Browse the repository at this point in the history
Now that we've addressed the crashes we can remove any debugging code
we added.

BUG=615090,795955

Change-Id: I1f0a0fdd895f4c7d1b5ac8d2b7b1f464a91e8e47
Reviewed-on: https://chromium-review.googlesource.com/834390
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525500}
  • Loading branch information
dtapuska authored and Commit Bot committed Dec 20, 2017
1 parent fa911ad commit c6146c5
Show file tree
Hide file tree
Showing 43 changed files with 71 additions and 332 deletions.
49 changes: 2 additions & 47 deletions chrome/browser/hang_monitor/hang_crash_dump_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,57 +19,12 @@ static const int kTerminateTimeoutMS = 2000;
// How long do we wait for the crash to be generated (in ms).
static const int kGenerateDumpTimeoutMS = 10000;

enum NoCrashKeyReason {
kNoCrashKeyReasonVirtualAlloc = 1,
kNoCrashKeyReasonWriteProcessMemory,
kNoCrashKeyReasonNoKeys
};

} // namespace

void CrashDumpAndTerminateHungChildProcess(
HANDLE hprocess,
const base::StringPairs& additional_child_crash_keys) {
void CrashDumpAndTerminateHungChildProcess(HANDLE hprocess) {
// Before terminating the process we try collecting a dump. Which
// a transient thread in the child process will do for us.
DWORD crash_key_failure = 0;
void* remote_memory = nullptr;
bool send_remote_memory = false;
std::vector<std::string> keys;
for (const auto& crash_key : additional_child_crash_keys) {
DCHECK(base::debug::LookupCrashKey(crash_key.first));
std::string serialized_key = crash_key.first;
serialized_key.append(":");
serialized_key.append(crash_key.second);
keys.push_back(serialized_key);
}
std::string serialized_keys = base::JoinString(keys, ",");

if (!serialized_keys.empty()) {
size_t data_length = serialized_keys.length() + 1;
remote_memory = VirtualAllocEx(hprocess, nullptr, data_length,
MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
if (remote_memory) {
send_remote_memory =
!!WriteProcessMemory(hprocess, remote_memory, serialized_keys.c_str(),
data_length, nullptr);
crash_key_failure =
MAKELPARAM(kNoCrashKeyReasonWriteProcessMemory, GetLastError());
} else {
crash_key_failure =
MAKELPARAM(kNoCrashKeyReasonVirtualAlloc, GetLastError());
}
} else {
crash_key_failure = MAKELPARAM(kNoCrashKeyReasonNoKeys, 0);
}

HANDLE remote_thread = nullptr;
if (send_remote_memory) {
remote_thread = InjectDumpForHungInput_ExportThunk(hprocess, remote_memory);
} else {
remote_thread = InjectDumpForHungInputNoCrashKeys_ExportThunk(
hprocess, crash_key_failure);
}
HANDLE remote_thread = InjectDumpForHungInput_ExportThunk(hprocess);
DCHECK(remote_thread) << "Failed creating remote thread: error "
<< GetLastError();
if (remote_thread) {
Expand Down
8 changes: 2 additions & 6 deletions chrome/browser/hang_monitor/hang_crash_dump_win.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@
#include "base/strings/string_split.h"

// Causes the given child process to generate a crash dump and terminates the
// process. |additional_serialized_crash_keys| are additional key/value string
// pairs that will be logged in the child crash report. The crash keys provided
// must be preregistered before calling this method.
void CrashDumpAndTerminateHungChildProcess(
HANDLE hprocess,
const base::StringPairs& additional_crash_keys);
// process.
void CrashDumpAndTerminateHungChildProcess(HANDLE hprocess);

#endif // CHROME_BROWSER_HANG_MONITOR_HANG_CRASH_DUMP_WIN_H_
7 changes: 2 additions & 5 deletions chrome/browser/ui/browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1672,17 +1672,14 @@ void Browser::WebContentsCreated(WebContents* source_contents,
task_manager::WebContentsTags::CreateForTabContents(new_contents);
}

void Browser::RendererUnresponsive(
WebContents* source,
const content::WebContentsUnresponsiveState& unresponsive_state) {
void Browser::RendererUnresponsive(WebContents* source) {
// Ignore hangs if a tab is blocked.
int index = tab_strip_model_->GetIndexOfWebContents(source);
DCHECK_NE(TabStripModel::kNoTab, index);
if (tab_strip_model_->IsTabBlocked(index))
return;

TabDialogs::FromWebContents(source)->ShowHungRendererDialog(
unresponsive_state);
TabDialogs::FromWebContents(source)->ShowHungRendererDialog();
}

void Browser::RendererResponsive(WebContents* source) {
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ui/browser.h
Original file line number Diff line number Diff line change
Expand Up @@ -618,9 +618,7 @@ class Browser : public TabStripModelObserver,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) override;
void RendererUnresponsive(
content::WebContents* source,
const content::WebContentsUnresponsiveState& unresponsive_state) override;
void RendererUnresponsive(content::WebContents* source) override;
void RendererResponsive(content::WebContents* source) override;
void DidNavigateMainFramePostCommit(
content::WebContents* web_contents) override;
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/cocoa/tab_dialogs_cocoa.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ class TabDialogsCocoa : public TabDialogs {
// TabDialogs:
gfx::NativeView GetDialogParentView() const override;
void ShowCollectedCookies() override;
void ShowHungRendererDialog(
const content::WebContentsUnresponsiveState& unresponsive_state) override;
void ShowHungRendererDialog() override;
void HideHungRendererDialog() override;
bool IsShowingHungRendererDialog() override;
void ShowProfileSigninConfirmation(
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@
new CollectedCookiesMac(web_contents_);
}

void TabDialogsCocoa::ShowHungRendererDialog(
const content::WebContentsUnresponsiveState& unresponsive_state) {
void TabDialogsCocoa::ShowHungRendererDialog() {
[HungRendererController showForWebContents:web_contents_];
}

Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/hung_plugin_tab_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ void KillPluginOnIOThread(int child_id) {
const content::ChildProcessData& data = iter.GetData();
if (data.id == child_id) {
#if defined(OS_WIN)
base::StringPairs crash_keys = {{"hung-reason", "plugin"}};
CrashDumpAndTerminateHungChildProcess(data.handle, crash_keys);
CrashDumpAndTerminateHungChildProcess(data.handle);
#else
base::Process process =
base::Process::DeprecatedGetProcessFromHandle(data.handle);
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ui/hung_renderer_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/test/test_browser_dialog.h"
#include "chrome/test/base/ui_test_utils.h"
#include "content/public/browser/web_contents_unresponsive_state.h"
#include "content/public/test/browser_test_utils.h"
#include "net/dns/mock_host_resolver.h"
#include "ui/base/ui_features.h"
Expand Down Expand Up @@ -39,8 +38,7 @@ IN_PROC_BROWSER_TEST_F(HungRendererNavigationBrowserTest,
browser(), embedded_test_server()->GetURL("a.com", "/title1.html"));
content::WebContents* active_web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
TabDialogs::FromWebContents(active_web_contents)
->ShowHungRendererDialog(content::WebContentsUnresponsiveState());
TabDialogs::FromWebContents(active_web_contents)->ShowHungRendererDialog();
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL("b.com", "/title2.html"));
// Expect that the dialog has been dismissed.
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ui/tab_dialogs.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ class Profile;

namespace content {
class WebContents;
struct WebContentsUnresponsiveState;
}

namespace ui {
Expand All @@ -43,8 +42,7 @@ class TabDialogs : public base::SupportsUserData::Data {
virtual void ShowCollectedCookies() = 0;

// Shows or hides the hung renderer dialog.
virtual void ShowHungRendererDialog(
const content::WebContentsUnresponsiveState& unresponsive_state) = 0;
virtual void ShowHungRendererDialog() = 0;
virtual void HideHungRendererDialog() = 0;
virtual bool IsShowingHungRendererDialog() = 0;

Expand Down
26 changes: 4 additions & 22 deletions chrome/browser/ui/views/hung_renderer_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
#endif

using content::WebContents;
using content::WebContentsUnresponsiveState;

HungRendererDialogView* HungRendererDialogView::g_instance_ = NULL;

Expand Down Expand Up @@ -212,9 +211,7 @@ HungRendererDialogView* HungRendererDialogView::GetInstance() {
}

// static
void HungRendererDialogView::Show(
WebContents* contents,
const WebContentsUnresponsiveState& unresponsive_state) {
void HungRendererDialogView::Show(WebContents* contents) {
if (logging::DialogsAreSuppressed())
return;

Expand All @@ -228,7 +225,7 @@ void HungRendererDialogView::Show(
return;
#endif
HungRendererDialogView* view = HungRendererDialogView::Create(window);
view->ShowForWebContents(contents, unresponsive_state);
view->ShowForWebContents(contents);
}

// static
Expand All @@ -255,9 +252,7 @@ HungRendererDialogView::~HungRendererDialogView() {
hung_pages_table_->SetModel(NULL);
}

void HungRendererDialogView::ShowForWebContents(
WebContents* contents,
const content::WebContentsUnresponsiveState& unresponsive_state) {
void HungRendererDialogView::ShowForWebContents(WebContents* contents) {
DCHECK(contents && GetWidget());

// Don't show the warning unless the foreground window is the frame, or this
Expand Down Expand Up @@ -306,7 +301,6 @@ void HungRendererDialogView::ShowForWebContents(
hung_pages_table_model_->RowCount()));
Layout();

unresponsive_state_ = unresponsive_state;
// Make Widget ask for the window title again.
GetWidget()->UpdateWindowTitle();

Expand Down Expand Up @@ -370,20 +364,8 @@ bool HungRendererDialogView::Cancel() {
hung_pages_table_model_->GetRenderProcessHost();
if (rph) {
#if defined(OS_WIN)
base::StringPairs crash_keys;

crash_keys.push_back(std::make_pair(
"hung-outstanding-acks",
base::IntToString(unresponsive_state_.outstanding_ack_count)));
crash_keys.push_back(std::make_pair(
"hung-outstanding-event-type",
base::IntToString(unresponsive_state_.outstanding_event_type)));
crash_keys.push_back(
std::make_pair("hung-last-event-type",
base::IntToString(unresponsive_state_.last_event_type)));

// Try to generate a crash report for the hung process.
CrashDumpAndTerminateHungChildProcess(rph->GetHandle(), crash_keys);
CrashDumpAndTerminateHungChildProcess(rph->GetHandle());
#else
rph->Shutdown(content::RESULT_CODE_HUNG, false);
#endif
Expand Down
14 changes: 2 additions & 12 deletions chrome/browser/ui/views/hung_renderer_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@

#include "base/macros.h"
#include "components/favicon/content/content_favicon_driver.h"
#include "content/public/browser/web_contents_delegate.h"
#include "content/public/browser/web_contents_observer.h"
#include "content/public/browser/web_contents_unresponsive_state.h"
#include "ui/base/models/table_model.h"
#include "ui/views/controls/button/button.h"
#include "ui/views/controls/table/table_view.h"
Expand Down Expand Up @@ -110,17 +108,13 @@ class HungRendererDialogView : public views::DialogDelegateView,
static HungRendererDialogView* GetInstance();

// Shows or hides the hung renderer dialog for the given WebContents.
static void Show(
content::WebContents* contents,
const content::WebContentsUnresponsiveState& unresponsive_state);
static void Show(content::WebContents* contents);
static void Hide(content::WebContents* contents);

// Returns true if the frame is in the foreground.
static bool IsFrameActive(content::WebContents* contents);

virtual void ShowForWebContents(
content::WebContents* contents,
const content::WebContentsUnresponsiveState& unresponsive_state);
virtual void ShowForWebContents(content::WebContents* contents);
virtual void EndForWebContents(content::WebContents* contents);

// views::DialogDelegateView overrides:
Expand Down Expand Up @@ -170,10 +164,6 @@ class HungRendererDialogView : public views::DialogDelegateView,
// Whether or not we've created controls for ourself.
bool initialized_;

// A copy of the unresponsive state which ShowForWebContents was
// called with.
content::WebContentsUnresponsiveState unresponsive_state_;

DISALLOW_COPY_AND_ASSIGN(HungRendererDialogView);
};

Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/ui/views/tab_dialogs_views.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ void TabDialogsViews::ShowCollectedCookies() {
new CollectedCookiesViews(web_contents_);
}

void TabDialogsViews::ShowHungRendererDialog(
const content::WebContentsUnresponsiveState& unresponsive_state) {
HungRendererDialogView::Show(web_contents_, unresponsive_state);
void TabDialogsViews::ShowHungRendererDialog() {
HungRendererDialogView::Show(web_contents_);
}

void TabDialogsViews::HideHungRendererDialog() {
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/views/tab_dialogs_views.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ class TabDialogsViews : public TabDialogs {
// TabDialogs:
gfx::NativeView GetDialogParentView() const override;
void ShowCollectedCookies() override;
void ShowHungRendererDialog(
const content::WebContentsUnresponsiveState& unresponsive_state) override;
void ShowHungRendererDialog() override;
void HideHungRendererDialog() override;
bool IsShowingHungRendererDialog() override;
void ShowProfileSigninConfirmation(
Expand Down
1 change: 0 additions & 1 deletion chrome_elf/chrome_elf_x64.def
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ EXPORTS
ClearCrashKeyValue_ExportThunk
CrashForException_ExportThunk
GetCrashReports_ExportThunk
InjectDumpForHungInputNoCrashKeys_ExportThunk
InjectDumpForHungInput_ExportThunk
RequestSingleCrashUpload_ExportThunk
SetCrashKeyValueEx_ExportThunk
Expand Down
1 change: 0 additions & 1 deletion chrome_elf/chrome_elf_x86.def
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ EXPORTS
ClearCrashKeyValue_ExportThunk
CrashForException_ExportThunk
GetCrashReports_ExportThunk
InjectDumpForHungInputNoCrashKeys_ExportThunk
InjectDumpForHungInput_ExportThunk
RequestSingleCrashUpload_ExportThunk
SetCrashKeyValueEx_ExportThunk
Expand Down
12 changes: 1 addition & 11 deletions components/crash/content/app/breakpad_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,23 +129,13 @@ DWORD WINAPI DumpProcessWithoutCrashThread(void*) {
} // namespace

extern "C" HANDLE __declspec(dllexport) __cdecl InjectDumpForHungInput(
HANDLE process,
void* serialized_crash_keys) {
HANDLE process) {
// |serialized_crash_keys| is not propagated in breakpad but is in crashpad
// since breakpad is deprecated.
return CreateRemoteThread(process, NULL, 0, DumpProcessWithoutCrashThread,
0, 0, NULL);
}

extern "C" HANDLE __declspec(
dllexport) __cdecl InjectDumpForHungInputNoCrashKeys(HANDLE process,
int reason) {
// |reason| is not propagated in breakpad but is in crashpad since breakpad
// is deprecated.
return CreateRemoteThread(process, NULL, 0, DumpProcessWithoutCrashThread, 0,
0, NULL);
}

// Returns a string containing a list of all modifiers for the loaded profile.
std::wstring GetProfileType() {
std::wstring profile_type;
Expand Down
8 changes: 1 addition & 7 deletions components/crash/content/app/crash_export_stubs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,7 @@ void SetCrashKeyValueEx_ExportThunk(const char* key,

void ClearCrashKeyValueEx_ExportThunk(const char* key, size_t key_len) {}

HANDLE InjectDumpForHungInput_ExportThunk(HANDLE process,
void* serialized_crash_keys) {
return nullptr;
}

HANDLE InjectDumpForHungInputNoCrashKeys_ExportThunk(HANDLE process,
int reason) {
HANDLE InjectDumpForHungInput_ExportThunk(HANDLE process) {
return nullptr;
}

Expand Down
15 changes: 3 additions & 12 deletions components/crash/content/app/crash_export_thunks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,11 @@ void ClearCrashKeyValueEx_ExportThunk(const char* key, size_t key_len) {
crash_reporter::ClearCrashKey(base::StringPiece(key, key_len));
}

HANDLE InjectDumpForHungInput_ExportThunk(HANDLE process,
void* serialized_crash_keys) {
HANDLE InjectDumpForHungInput_ExportThunk(HANDLE process) {
return CreateRemoteThread(
process, nullptr, 0,
crash_reporter::internal::DumpProcessForHungInputThread,
serialized_crash_keys, 0, nullptr);
}

HANDLE InjectDumpForHungInputNoCrashKeys_ExportThunk(HANDLE process,
int reason) {
return CreateRemoteThread(
process, nullptr, 0,
crash_reporter::internal::DumpProcessForHungInputNoCrashKeysThread,
reinterpret_cast<void*>(reason), 0, nullptr);
crash_reporter::internal::DumpProcessForHungInputThread, nullptr, 0,
nullptr);
}

#if defined(ARCH_CPU_X86_64)
Expand Down
Loading

0 comments on commit c6146c5

Please sign in to comment.