Show only one profile corrupted dialog, support --noerrdialogs

Show only one profile corrupted dialog, support --noerrdialogs

Make sure that at most one profile corruption error dialog is shown at
any given time. Currently I ocasionally get six dialogs with an
identical error message when I start Chrome while the profile is locked.
Although, the ShowProfileErrorDialog method is called with two different
messages (corrupt profile vs incompatible profile), I have only seen one
of them showing up at any time, so the boolean guard is applied to any
dialog generated by ShowProfileErrorDialog.

R=pkasting@chromium.org
BUG=446732, 447791
TEST=Manually:
1. Start Chrome to initialize the profile directory.
2. Close Chrome.
3. Mark database as read-only: chmod a-r "/tmp/profile/Default/Web Data"
4. Start Chrome again.
5. Verify that only one dialog is shown (instead of six).
6. Close Chrome.
7. Start Chrome, with the --noerrdialogs flag.
8. Verify that no error dialogs are displayed at all (instead of six).

Committed: https://crrev.com/6d12da2b4cf576e0b9f2d571701e86a0cbac65d7
Cr-Commit-Position: refs/heads/master@{#314318}

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

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
File chrome/browser/ui/profile_error_dialog.cc (right):

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:16: bool
is_showing_profile_error_dialog = false;
Nit: A static bool inside the method would be clearer IMO.

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:25: switches::kNoErrorDialogs))
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?

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:28: base::AutoReset<bool>
resetter(&is_showing_profile_error_dialog, true);
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]

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
File chrome/browser/ui/profile_error_dialog.cc (right):

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:16: bool
is_showing_profile_error_dialog = false;
On 2015/01/26 19:52:46, Peter Kasting wrote:
> Nit: A static bool inside the method would be clearer IMO.

Done.

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:25: switches::kNoErrorDialogs))
On 2015/01/26 19:52:46, Peter Kasting wrote:
> 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?

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.

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:28: base::AutoReset<bool>
resetter(&is_showing_profile_error_dialog, true);
On 2015/01/26 19:52:46, Peter Kasting wrote:
> 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.

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]

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
File chrome/browser/ui/profile_error_dialog.cc (right):

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:25: switches::kNoErrorDialogs))
On 2015/01/26 23:26:21, robwu wrote:
> On 2015/01/26 19:52:46, Peter Kasting wrote:
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:28: base::AutoReset<bool>
resetter(&is_showing_profile_error_dialog, true);
On 2015/01/26 23:26:21, robwu wrote:
> On 2015/01/26 19:52:46, Peter Kasting wrote:
> > 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.
> 
> If you follow the calls, then you'll indeed see a nested message loop:
> https://cs.chromium.org/SimpleMessageBoxViews::RunDialogAndGetResult

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]

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
File chrome/browser/ui/profile_error_dialog.cc (right):

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:25: switches::kNoErrorDialogs))
On 2015/01/27 00:20:32, Peter Kasting wrote:
> On 2015/01/26 23:26:21, robwu wrote:
> > On 2015/01/26 19:52:46, Peter Kasting wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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).

https://codereview.chromium.org/873253002/diff/1/chrome/browser/ui/profile_er...
chrome/browser/ui/profile_error_dialog.cc:28: base::AutoReset<bool>
resetter(&is_showing_profile_error_dialog, true);
On 2015/01/27 00:20:32, Peter Kasting wrote:
> On 2015/01/26 23:26:21, robwu wrote:
> > On 2015/01/26 19:52:46, Peter Kasting wrote:
> > > 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.
> > 
> > If you follow the calls, then you'll indeed see a nested message loop:
> > https://cs.chromium.org/SimpleMessageBoxViews::RunDialogAndGetResult
> 
> 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.

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.

[Peter Kasting, 2015-01-27 22:25:02]

On 2015/01/27 22:03:47, robwu wrote:
> 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.

LGTM.

It might be good to not only prevent nested boxes, but prevent repeats of any
profile error message -- after we've shown "your profile could not be opened"
once, it's just annoying to show it again even in a non-recursive scenario.

But this is an improvement on the current situation at least.

[robwu, 2015-02-03 10:58:22]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-02-03 10:59:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873253002/20001

[commit-bot: I haz the power, 2015-02-03 12:34:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-03 12:35:14]

Patchset 2 (id:??) landed as
https://crrev.com/6d12da2b4cf576e0b9f2d571701e86a0cbac65d7
Cr-Commit-Position: refs/heads/master@{#314318}