Show only one profile corrupted dialog, support --noerrdialogs

chrome/browser/ui/profile_error_dialog.cc

Index: chrome/browser/ui/profile_error_dialog.cc
diff --git a/chrome/browser/ui/profile_error_dialog.cc b/chrome/browser/ui/profile_error_dialog.cc
index 0dae4bfd5cb01b50f59d388387a3c658d99edfed..a987efca7eac99f9ee0bd9348bb0757e7cc523db 100644
--- a/chrome/browser/ui/profile_error_dialog.cc
+++ b/chrome/browser/ui/profile_error_dialog.cc
@@ -4,19 +4,32 @@
 
 #include "chrome/browser/ui/profile_error_dialog.h"
 
+#include "base/auto_reset.h"
+#include "base/base_switches.h"
+#include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "chrome/browser/ui/simple_message_box.h"
 #include "chrome/grit/chromium_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 
+namespace {
+bool is_showing_profile_error_dialog = false;

[Peter Kasting, 2015/01/26 19:52:46]

Nit: A static bool inside the method would be clearer IMO.

[robwu, 2015/01/26 23:26:21]

Done.

+}  // namespace
+
 void ShowProfileErrorDialog(ProfileErrorType type, int message_id) {
 #if defined(OS_ANDROID) || defined(OS_IOS)
   NOTIMPLEMENTED();
 #else
   UMA_HISTOGRAM_ENUMERATION("Profile.ProfileError", type, PROFILE_ERROR_END);
-  chrome::ShowMessageBox(NULL,
-                         l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
-                         l10n_util::GetStringUTF16(message_id),
-                         chrome::MESSAGE_BOX_TYPE_WARNING);
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kNoErrorDialogs))

[Peter Kasting, 2015/01/26 19:52:46]

I wonder if this should be in ShowMessageBox() instead?  Or do we use that for
non-error dialogs that we need to continue to display?

[robwu, 2015/01/26 23:26:21]

At least ShowWarningMessageBox in
chrome/browser/chromeos/file_manager/open_util.cc shows a warning message that
should really be shown to the user, so moving this condition down into
ShowMessageBox does not make sense.

[Peter Kasting, 2015/01/27 00:20:32]

Are you sure that should be shown when --noerrdialogs is true?  It seems like
that's an error dialog, and thus shouldn't be shown.

[robwu, 2015/01/27 22:03:47]

I'm not sure about that, but I'm definitely to not about hiding the dialog boxes
for all other callers. To be on the safe side, I only add this flag check to a
location that is known to cause issues (see linked bug).

+    return;
+  if (!is_showing_profile_error_dialog) {
+    base::AutoReset<bool> resetter(&is_showing_profile_error_dialog, true);

[Peter Kasting, 2015/01/26 19:52:46]

If this actually works to eliminate duplicate dialogs, it means we're spinning
nested message loops each time we enter this function.  That in turn seems very
bad -- nested loops can in general cause all kinds of corruption and crashes
because they can cause bascially anything on the current thread to become
reentrant unexpectedly.

I would have imagined that ShowMessageBox() below would simply return
immediately.  Is that not what's happening?  I'd kind of like to see the stack
trace here.

[robwu, 2015/01/26 23:26:21]

If you follow the calls, then you'll indeed see a nested message loop:
https://cs.chromium.org/SimpleMessageBoxViews::RunDialogAndGetResult

Here is the stack trace that shows two ShowProfileErrorDialog calls:

[8802:8802:0127/001658:ERROR:profile_error_dialog.cc(33)]
is_showing_profile_error_dialog==true, hello backtrace:
#0 0x7f18a0b08e7e base::debug::StackTrace::StackTrace()
#1 0x7f18a4c82de7 ShowProfileErrorDialog()
#2 0x7f189fe1cf61 (anonymous namespace)::ProfileErrorCallback()
#3 0x7f18a841ef14 base::internal::RunnableAdapter<>::Run()
#4 0x7f18a841eeba base::internal::InvokeHelper<>::MakeItSo()
#5 0x7f18a841ee61 base::internal::Invoker<>::Run()
#6 0x7f18a756be06 base::Callback<>::Run()
#7 0x7f18a756b14a WebDatabaseService::OnDatabaseLoadDone()
#8 0x7f18a756e2d9 base::internal::RunnableAdapter<>::Run()
#9 0x7f18a756e1d8 base::internal::InvokeHelper<>::MakeItSo()
#10 0x7f18a756e15c base::internal::Invoker<>::Run()
#11 0x7f189f9549ae base::Callback<>::Run()
#12 0x7f18a0c7add9 base::debug::TaskAnnotator::RunTask()
#13 0x7f18a0b98d13 base::MessageLoop::RunTask()
#14 0x7f18a0b98e6b base::MessageLoop::DeferOrRunPendingTask()
#15 0x7f18a0b99055 base::MessageLoop::DoWork()
#16 0x7f18a0c6b8ee base::MessagePumpGlib::Run()
#17 0x7f18a0b988a2 base::MessageLoop::RunHandler()
#18 0x7f18a0be0e54 base::RunLoop::Run()
#19 0x7f18a4d9be09 chrome::(anonymous
namespace)::SimpleMessageBoxViews::RunDialogAndGetResult()
#20 0x7f18a4d9b988 chrome::(anonymous namespace)::ShowMessageBoxImpl()
#21 0x7f18a4d9b796 chrome::ShowMessageBox()
#22 0x7f18a4c82d19 ShowProfileErrorDialog()
#23 0x7f189fe1cf61 (anonymous namespace)::ProfileErrorCallback()
#24 0x7f18a841ef14 base::internal::RunnableAdapter<>::Run()
#25 0x7f18a841eeba base::internal::InvokeHelper<>::MakeItSo()
#26 0x7f18a841ee61 base::internal::Invoker<>::Run()
#27 0x7f18a756be06 base::Callback<>::Run()
#28 0x7f18a756b14a WebDatabaseService::OnDatabaseLoadDone()

[Peter Kasting, 2015/01/27 00:20:32]

That seems very, very bad.  I would imagine this can cause arbitrary bugs and
crashes.

I think we need to nuke this method entirely and ask the UI leads what it should
be replaced with for its various callers, e.g. an infobar or a badge on the
chrome menu.  Something that doesn't spin a nested message loop.

[robwu, 2015/01/27 22:03:47]

Can you follow up with the right people? As a member of the UI team, you
probably know better who to talk with and what action to take.

Meanwhile, can this patch be accepted? Besides getting rid of the too many
dialogs, it will also cause less nested message loops to be spinned and thereby
reduce the odds of triggering a bug.

+    chrome::ShowMessageBox(NULL,
+                           l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
+                           l10n_util::GetStringUTF16(message_id),
+                           chrome::MESSAGE_BOX_TYPE_WARNING);
+  }
 #endif
 }