Fix a crash on clicking "Load unpacked extension" multiple times

Fix a crash on clicking "Load unpacked extension" multiple times

SelectFile() is getting called more than once for the same window
and causing NOTREACHED() failure on linux, since it tries to re-add
the observer.

To fix the failure, this patch adds a check that does an early-return
if the select file dialog is currently being shown to the specified
window.

BUG=432783
Committed: https://crrev.com/7ec6e063e36a66300570ab0adfd88c2e5acc6bd3
Cr-Commit-Position: refs/heads/master@{#308113}

[babu, 2014-12-02 20:57:15]

sudarsana.nagineni@intel.com changed reviewers:
+ finnur@chromium.org

[Finnur, 2014-12-03 10:12:48]

finnur@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Finnur, 2014-12-03 10:12:49]

Devlin should probably weigh in before I give a green light.

[Devlin, 2014-12-03 17:47:55]

https://codereview.chromium.org/772983002/diff/1/chrome/browser/ui/webui/exte...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/772983002/diff/1/chrome/browser/ui/webui/exte...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:102:
load_extension_dialog_ = ui::SelectFileDialog::Create(
hmm... I have a slight concern in that ui::SelectFileDialog is ref-counted, so
reassigning |load_extension_dialog_| doesn't mean the old object is destroyed. 
Coupled with the fact that we'd have the FileHelper as the observer for both, I
feel like this could let us get into some weird state.  Are you sure that
everything can handle the possibility of multiple file dialogs being
opened/notifying of files selected?

The alternative would be to just early-return if load_extension_dialog_ already
exists, and say that we can only open one dialog at a time (of course, this
would be better if there was a way of focusing that dialog, but it doesn't look
like we have a method for that yet).

[Devlin, 2014-12-03 17:59:01]

https://codereview.chromium.org/772983002/diff/1/chrome/browser/ui/webui/exte...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/772983002/diff/1/chrome/browser/ui/webui/exte...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:102:
load_extension_dialog_ = ui::SelectFileDialog::Create(
On 2014/12/03 17:47:55, Devlin wrote:
> hmm... I have a slight concern in that ui::SelectFileDialog is ref-counted, so
> reassigning |load_extension_dialog_| doesn't mean the old object is destroyed.

> Coupled with the fact that we'd have the FileHelper as the observer for both,
I
> feel like this could let us get into some weird state.  Are you sure that
> everything can handle the possibility of multiple file dialogs being
> opened/notifying of files selected?
> 
> The alternative would be to just early-return if load_extension_dialog_
already
> exists, and say that we can only open one dialog at a time (of course, this
> would be better if there was a way of focusing that dialog, but it doesn't
look
> like we have a method for that yet).

For instance, this causes a problem on line 94, where we inform the dialog that
the listener was destroyed.  If we remove our reference to the file dialog,
while the dialog is still alive, and then the FileHelper is destroyed, we could
end up crashing because we never unregister.

[babu, 2014-12-03 23:10:09]

On 2014/12/03 17:47:55, Devlin wrote:
>
https://codereview.chromium.org/772983002/diff/1/chrome/browser/ui/webui/exte...
> File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):
> 
>
https://codereview.chromium.org/772983002/diff/1/chrome/browser/ui/webui/exte...
> chrome/browser/ui/webui/extensions/extension_loader_handler.cc:102:
> load_extension_dialog_ = ui::SelectFileDialog::Create(
> hmm... I have a slight concern in that ui::SelectFileDialog is ref-counted, so
> reassigning |load_extension_dialog_| doesn't mean the old object is destroyed.

> Coupled with the fact that we'd have the FileHelper as the observer for both,
I
> feel like this could let us get into some weird state.  Are you sure that
> everything can handle the possibility of multiple file dialogs being
> opened/notifying of files selected?

Thanks for reviewing Devlin!

My understanding was that reassigning |load_extension_dialog_| will always
destroy the old object, and that's what I also noticed while testing this patch.


The following stacktrace also shows that the old object is getting destroyed
when the new one is created.

#0 0x7f2ac9c45059 base::debug::StackTrace::StackTrace()
#1 0x7f2ac9f4979e libgtk2ui::SelectFileDialogImplGTK::~SelectFileDialogImplGTK()
#2 0x7f2ac9f499ee libgtk2ui::SelectFileDialogImplGTK::~SelectFileDialogImplGTK()
#3 0x7f2ace702bfb base::RefCountedThreadSafe<>::DeleteInternal()
#4 0x7f2ace702b26 base::DefaultRefCountedThreadSafeTraits<>::Destruct()
#5 0x7f2ace7026ba base::RefCountedThreadSafe<>::Release()
#6 0x7f2ace702500 scoped_refptr<>::Release()
#7 0x7f2ace70254e scoped_refptr<>::operator=()
#8 0x7f2ace702288 scoped_refptr<>::operator=()
#9 0x7f2acfa17654 extensions::ExtensionLoaderHandler::FileHelper::ChooseFile()

[Devlin, 2014-12-03 23:26:39]

On 2014/12/03 23:10:09, babu wrote:
> My understanding was that reassigning |load_extension_dialog_| will always
> destroy the old object, and that's what I also noticed while testing this
patch.
>
> The following stacktrace also shows that the old object is getting destroyed
> when the new one is created.

Right, it does on linux because the only reference to the SelectFileDialog is
held by the FileHelper.  But, sadly, that isn't a guarantee.  We'd probably have
to check every OS's implementation to make sure that *none* of the SelectFile()
calls resulted in a ref being passed off somewhere, and even then, it could
change at anytime.  Refcounts can be kind of a pain in times like this. :/

Is there a significant reason to prefer creating a new dialog over just leaving
the old one open?

[babu, 2014-12-04 17:24:03]

On 2014/12/03 23:26:39, Devlin wrote:
> Right, it does on linux because the only reference to the SelectFileDialog is
> held by the FileHelper.  But, sadly, that isn't a guarantee.  We'd probably
have
> to check every OS's implementation to make sure that *none* of the
SelectFile()
> calls resulted in a ref being passed off somewhere, and even then, it could
> change at anytime.  Refcounts can be kind of a pain in times like this. :/
> 
> Is there a significant reason to prefer creating a new dialog over just
leaving
> the old one open?

The actual problem is that calling SelectFile() more than once is causing a
NOTREACHED() failure on linux, since it tries to re-add the observer. 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

The issue seems to be a linux platform specific, so I'm thinking an alternative
solution that does the early-return if the dialog is currently being shown to
the specified window.

--- a/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc
+++ b/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc
@@ -228,6 +228,10 @@ void SelectFileDialogImplGTK::SelectFileImpl(
   // and chooses 'Open Link in New Tab' when 'Ask where to save each file
   // before downloading.' preference is turned on. (http://crbug.com/29213)
   if (owning_window) {
+    if (IsRunning(owning_window)) {
+      LOG(ERROR) << "Select file dialog already in use!";
+      return;
+     }
     owning_window->AddObserver(this);
     parents_.insert(owning_window);
   }

[babu, 2014-12-04 17:28:56]

On 2014/12/04 17:24:03, babu wrote:
> On 2014/12/03 23:26:39, Devlin wrote:
> > Right, it does on linux because the only reference to the SelectFileDialog
is
> > held by the FileHelper.  But, sadly, that isn't a guarantee.  We'd probably
> have
> > to check every OS's implementation to make sure that *none* of the
> SelectFile()
> > calls resulted in a ref being passed off somewhere, and even then, it could
> > change at anytime.  Refcounts can be kind of a pain in times like this. :/
> > 
> > Is there a significant reason to prefer creating a new dialog over just
> leaving
> > the old one open?
> 
> The actual problem is that calling SelectFile() more than once is causing a
> NOTREACHED() failure on linux, since it tries to re-add the observer. 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> The issue seems to be a linux platform specific, so I'm thinking an
alternative
> solution that does the early-return if the dialog is currently being shown to
> the specified window.
> 
> --- a/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc
> +++ b/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc
> @@ -228,6 +228,10 @@ void SelectFileDialogImplGTK::SelectFileImpl(
>    // and chooses 'Open Link in New Tab' when 'Ask where to save each file
>    // before downloading.' preference is turned on. (http://crbug.com/29213)
>    if (owning_window) {
> +    if (IsRunning(owning_window)) {
> +      LOG(ERROR) << "Select file dialog already in use!";
> +      return;
> +     }
>      owning_window->AddObserver(this);
>      parents_.insert(owning_window);
>    }

Other option would be to add a check and prevent calling AddObserver() multiple
times. This will allow us to open a separate dialog on every click, but I don't
see a real use case of showing more than one select file dialog at a time for
the same window.

[Devlin, 2014-12-04 17:31:48]

I agree; I think the right solution is an early return.

[babu, 2014-12-04 19:50:55]

sudarsana.nagineni@intel.com changed reviewers:
+ estade@chromium.org

[babu, 2014-12-04 19:54:11]

+estade@ for owner review.

[Devlin, 2014-12-04 20:00:14]

https://codereview.chromium.org/772983002/diff/20001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/772983002/diff/20001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:233: return;
This isn't what I was referring to.  I meant an early return in the
LoaderHandler, since there's already a dialog up.  I don't think that making a
platform-specific change is the right solution to this.

[babu, 2014-12-04 21:01:21]

Devlin, PTAL. Thanks for your help.

[Devlin, 2014-12-04 21:39:10]

This looks much better. :)

https://codereview.chromium.org/772983002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/772983002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:110: LOG(ERROR)
<< "Select file dialog already in use!";
Generally, we don't like to log errors for things that can happen in the normal
course of action.  Since we should be able to handle the user clicking on the
load button twice, I'd remove this in favor of a comment
// File chooser dialog is already running; ignore the click.

[babu, 2014-12-04 22:06:05]

On 2014/12/04 21:39:10, Devlin wrote:
> This looks much better. :)
> 
>
https://codereview.chromium.org/772983002/diff/40001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):
> 
>
https://codereview.chromium.org/772983002/diff/40001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/extensions/extension_loader_handler.cc:110: LOG(ERROR)
> << "Select file dialog already in use!";
> Generally, we don't like to log errors for things that can happen in the
normal
> course of action.  Since we should be able to handle the user clicking on the
> load button twice, I'd remove this in favor of a comment
> // File chooser dialog is already running; ignore the click.

Done.

[Devlin, 2014-12-04 22:16:26]

lgtm (note you'll still need an OWNERS approval)

[Evan Stade, 2014-12-11 21:42:42]

The CQ bit was checked by estade@chromium.org

[babu, 2014-12-12 13:41:49]

Finnur, could you please have a look?

[Finnur, 2014-12-12 14:34:04]

LGTM (sorry, didn't see Devlin's hint about an OWNER; didn't actually even
realize he isn't an owner there).

Also, as a general note: I think I speak for all Chromium reviewers when I say:
In the future, feel free to ping us -way- before a week has passed without a
response.

[babu, 2014-12-12 16:24:09]

On 2014/12/12 14:34:04, Finnur wrote:
> LGTM (sorry, didn't see Devlin's hint about an OWNER; didn't actually even
> realize he isn't an owner there).
> 
> Also, as a general note: I think I speak for all Chromium reviewers when I
say:
> In the future, feel free to ping us -way- before a week has passed without a
> response.

Thanks, will do in the future :)

[babu, 2014-12-12 16:24:27]

The CQ bit was checked by sudarsana.nagineni@intel.com

[commit-bot: I haz the power, 2014-12-12 16:25:19]

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

[commit-bot: I haz the power, 2014-12-12 17:37:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 17:38:12]

Patchset 4 (id:??) landed as
https://crrev.com/7ec6e063e36a66300570ab0adfd88c2e5acc6bd3
Cr-Commit-Position: refs/heads/master@{#308113}