WebUI: prevent WebContent to hold invalid pointer.

chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h"
 
 #include <string>
 
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_preferences_util.h"
@@ skipping 31 matching lines @@
     web_contents_->SetDelegate(this);
   }
   content::RendererPreferences* prefs =
       web_contents_->GetMutableRendererPrefs();
   renderer_preferences_util::UpdateFromSystemSettings(
       prefs, Profile::FromBrowserContext(browser_context), web_contents_.get());
 
   web_contents_->GetRenderViewHost()->SyncRendererPrefs();
 
   // Set |this| as a delegate so the ConstrainedWebDialogUI can retrieve it.
   ConstrainedWebDialogUI::SetConstrainedDelegate(web_contents_.get(), this);

[arthursonzogni, 2017/04/04 14:32:15]

FYI: the |this| pointer is given to the WebContent here.

 
   web_contents_->GetController().LoadURL(delegate->GetDialogContentURL(),
                                          content::Referrer(),
                                          ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                          std::string());
 }
 
 ConstrainedWebDialogDelegateBase::~ConstrainedWebDialogDelegateBase() {
   if (release_contents_on_close_)
     ignore_result(web_contents_.release());

[Avi (use Gerrit), 2017/04/04 14:49:46]

(read the other comment first)

This if() block seems like the right place; if this object is going away and
we're just doing a release, then we need to clear our reference.

[arthursonzogni, 2017/04/04 15:17:52]

Yes, that is what I have done in the first patch.

The problem is that I am not allowed to call SupportsuserData::RemoveUserData()
here because it will trigger a DCHECK:
#3 0x000002eeba5a base::SequenceCheckerImpl::CalledOnValidSequence()
#4 0x000002efceac base::SupportsUserData::RemoveUserData()

The reason:
```
  // SupportsUserData is not thread-safe, and on debug build will assert it is
  // only used on one execution sequence. Calling this method allows the caller
  // to hand the SupportsUserData instance across execution sequences. Use only
  // if you are taking full control of the synchronization of that hand over.
  void DetachFromSequence();
```

[Lei Zhang, 2017/04/06 08:27:55]

Can you check to see what thread it's being called on and what it's expecting?
Shouldn't they both be UI thread?

 }
 
 const WebDialogDelegate*
     ConstrainedWebDialogDelegateBase::GetWebDialogDelegate() const {
   return web_dialog_delegate_.get();
 }
 
 WebDialogDelegate*
     ConstrainedWebDialogDelegateBase::GetWebDialogDelegate() {
   return web_dialog_delegate_.get();
 }
 
 void ConstrainedWebDialogDelegateBase::OnDialogCloseFromWebUI() {
   closed_via_webui_ = true;
   CloseContents(web_contents_.get());
 }
 
 bool ConstrainedWebDialogDelegateBase::closed_via_webui() const {
   return closed_via_webui_;
 }
 
 void ConstrainedWebDialogDelegateBase::ReleaseWebContentsOnDialogClose() {
+  // Remove reference to |this| in the WebContent since the lifetime of the
+  // WebContent may exceed the one of this object.
+  ConstrainedWebDialogUI::ClearConstrainedDelegate(web_contents_.get());

[Avi (use Gerrit), 2017/04/04 14:49:46]

This doesn't look right.

  // If called, on dialog closure, the dialog will release its WebContents
  // instead of destroying it. After which point, the caller will own the
  // released WebContents.
  virtual void ReleaseWebContentsOnDialogClose() = 0;

This is a note that *in the future* the close behavior should change. When this
function is called, the close isn't happening right then.

Can you move the clearing of the delegate to the actual close?

[arthursonzogni, 2017/04/04 15:17:52]

I can't. [See the answer above].
Clearing the pointer to the ConstrainedDelegate early will cause problem if
someone needs it between
* ConstrainedWebDialogDelegateBase::ReleaseWebContentsOnDialogClose() and
* ConstrainedWebDialogDelegateBase::~ConstrainedWebDialogDelegateBase()

It looks like there is no call to
ConstrainedWebDialogUI::GetConstrainedDelegate() that make the assumption that
it will return something. I hope @thestig would knows what problems may arise.

[Lei Zhang, 2017/04/06 09:12:54]

Please double check my work:

Starting from PrintPreviewUI::OnHidePreviewDialog() we have:

delegate->ReleaseWebContentsOnDialogClose();
background_printing_manager->OwnPrintPreviewDialog(preview_dialog);
OnClosePrintPreviewDialog();

and PrintPreviewUI::OnClosePrintPreviewDialog() tries to call
GetConstrainedDelegate() too.

With this patch set, that GetConstrainedDelegate() call will return nullptr, and
the following code doesn't run:

delegate->GetWebDialogDelegate()->OnDialogClosed(std::string());
delegate->OnDialogCloseFromWebUI();

OnDialogClosed() is overridden by PrintPreviewDialogDelegate::OnDialogClosed()
which is a no-op.

OnDialogCloseFromWebUI() goes to
ConstrainedWebDialogDelegateBase::OnDialogCloseFromWebUI() which tries to call
CloseContents(). CloseContents() is overridden by
ConstrainedWebDialogDelegateViews::CloseContents(). If that does not run, then
does that mean the print preview dialog stays open?

   release_contents_on_close_ = true;
 }
 
 gfx::NativeWindow ConstrainedWebDialogDelegateBase::GetNativeDialog() {
   NOTREACHED();
   return NULL;
 }
 
 WebContents* ConstrainedWebDialogDelegateBase::GetWebContents() {
   return web_contents_.get();
@@ skipping 16 matching lines @@
 
 gfx::Size ConstrainedWebDialogDelegateBase::GetPreferredSize() const {
   NOTREACHED();
   return gfx::Size();
 }
 
 void ConstrainedWebDialogDelegateBase::ResizeToGivenSize(
     const gfx::Size size) {
   NOTREACHED();
 }