WebUI: prevent WebContent to hold invalid pointer.

WebUI: prevent WebContent to hold an invalid pointer.

A ConstrainedWebDialogBase creates and own a WebContent. For various
reasons, it stores a pointer to itself inside the WebContent. The
problem was that the lifetime of the WebContent may exceed the one of
ConstrainedWebDialogBase when |release_contents_on_close_| is set to
false. The pointer becomes invalid.

BUG=704327
Review-Url: https://codereview.chromium.org/2798583002
Cr-Commit-Position: refs/heads/master@{#463318}
Committed: https://chromium.googlesource.com/chromium/src/+/e63206f4ef0c18bca991534852acd0193a2de94a

[arthursonzogni, 2017-04-04 10:35:29]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 10:35:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-04-04 10:36:40]

Description was changed from

==========
WebUI: prevent WebContent to hold invalid pointer.

A ConstrainedWebDialogBase creates and own a WebContent. For various
reasons, it stores a pointer to itself inside the WebContent. The
problem was that the lifetime of the WebContent may exceed the one of
ConstrainedWebDialogBase when |release_contents_on_close_| is set to
false. The pointer becomes invalid.

BUG=704327
==========

to

==========
WebUI: prevent WebContent to hold an invalid pointer.

A ConstrainedWebDialogBase creates and own a WebContent. For various
reasons, it stores a pointer to itself inside the WebContent. The
problem was that the lifetime of the WebContent may exceed the one of
ConstrainedWebDialogBase when |release_contents_on_close_| is set to
false. The pointer becomes invalid.

BUG=704327
==========

[commit-bot: I haz the power, 2017-04-04 11:16:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 11:16:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[arthursonzogni, 2017-04-04 12:58:29]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 12:58:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 13:39:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 13:39:26]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-04-04 14:21:51]

arthursonzogni@chromium.org changed reviewers:
+ thestig@chromium.org

[arthursonzogni, 2017-04-04 14:21:52]

Hi Lei (@thestig),

I am working on a crash with the print preview that happens when PlzNavigate is
enabled:
https://bugs.chromium.org/p/chromium/issues/detail?id=704327

That's why I have reported some issue the last few days:
https://bugs.chromium.org/p/chromium/issues/detail?id=706369
https://bugs.chromium.org/p/chromium/issues/detail?id=706319
https://bugs.chromium.org/p/chromium/issues/detail?id=704900
https://bugs.chromium.org/p/chromium/issues/detail?id=636642
Maybe some of them are related to this bug.

I think it is due to the ConstrainedWebDialogDelegateBase storing a pointer to
itself inside the WebContent. It is not a problem as long as the
ConstrainedWebDialogDelegateBase owns the WebContent.
The problem arise when ReleaseWebContentsOnDialogClose() is called. After this
point, the ConstrainedWebDialogDelegateBase doesn't own the WebContent anymore.
The pointer becomes invalid when the object is deleted.
The ReleaseWebContentsOnDialogClose() is used by the print preview dialog when
it switches from the chrome one(ctrl+p) to the native one(ctrl+shit+p). (See
OnHidePreviewDialog()). Do you understand why it has to be like this?

This CL prevents us to use an invalid pointer. Can you confirm that this is the
right thing to do?

The ConstrainedWebDialogUI will not be able to retrieve the
ConstrainedWebDialogDelegateBase after the ReleaseWebContentsOnDialogClose()
call.

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

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:52:
ConstrainedWebDialogUI::SetConstrainedDelegate(web_contents_.get(), this);
FYI: the |this| pointer is given to the WebContent here.

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

avi@chromium.org changed reviewers:
+ avi@chromium.org

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

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:62:
ignore_result(web_contents_.release());
(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.

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:87:
ConstrainedWebDialogUI::ClearConstrainedDelegate(web_contents_.get());
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]

Thanks Avi.

I can't do what you suggest. Maybe we could store this pointer outside of the
UserData or change how the classes are designed to work together.
Let me know if you have any idea.

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:62:
ignore_result(web_contents_.release());
On 2017/04/04 14:49:46, Avi wrote:
> (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.

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();
```

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:87:
ConstrainedWebDialogUI::ClearConstrainedDelegate(web_contents_.get());
On 2017/04/04 14:49:46, Avi wrote:
> 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?

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 08:27:56]

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:62:
ignore_result(web_contents_.release());
On 2017/04/04 15:17:52, arthursonzogni wrote:
> On 2017/04/04 14:49:46, Avi wrote:
> > (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.
> 
> 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()

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

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

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:87:
ConstrainedWebDialogUI::ClearConstrainedDelegate(web_contents_.get());
On 2017/04/04 15:17:52, arthursonzogni wrote:
> 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.

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?

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_ui.cc (right):

https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_ui.cc:120:
web_ui()->GetWebContents()->
Another idea is for ConstrainedWebDialogUI to be a WebContentsObserver, and
override WebContentsDestroyed(). Then you'll know to return nullptr here. Though
getting ClearConstrainedDelegate() to work right feels cleaner.

[arthursonzogni, 2017-04-06 09:17:22]

On 2017/04/06 08:27:56, Lei Zhang wrote:
>
https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):
> 
>
https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:62:
> ignore_result(web_contents_.release());
> On 2017/04/04 15:17:52, arthursonzogni wrote:
> > On 2017/04/04 14:49:46, Avi wrote:
> > > (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.
> > 
> > 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()
> 
> Can you check to see what thread it's being called on and what it's expecting?
> Shouldn't they both be UI thread?

It doesn't work only in
ConstrainedWebDialogBrowserTest.ReleaseWebContentsOnDialogClose
I misinterpreted the stacktrace. 
The full one is:
[76285:76285:0406/110206.695500:FATAL:lock_impl_posix.cc(65)] Check failed: rv
== 0 (22 vs. 0). Invalid argument
#0 0x2b85982ec6bb base::debug::StackTrace::StackTrace()
#1 0x2b85982eb3bc base::debug::StackTrace::StackTrace()
#2 0x2b85983549bf logging::LogMessage::~LogMessage()
#3 0x2b859846531d base::internal::LockImpl::Lock()
#4 0x2b85982a4703 base::Lock::Acquire()
#5 0x2b85982a43a3 base::AutoLock::AutoLock()
#6 0x2b85984297e4 base::SequenceCheckerImpl::CalledOnValidSequence()
#7 0x2b859845f9f3 base::SupportsUserData::RemoveUserData()
#8 0x000005b041a0 ConstrainedWebDialogUI::ClearConstrainedDelegate()

So it looks like the lock pthread_mutex_lock(...) has failed. I will take a
look.

[arthursonzogni, 2017-04-06 09:26:10]

On 2017/04/06 09:12:54, Lei Zhang wrote:
> 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?

Absolutely correct, except that the print preview dialog doesn't stay open. I
don't know why.

>
https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/constrained_web_dialog_ui.cc (right):
> 
>
https://codereview.chromium.org/2798583002/diff/20001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/constrained_web_dialog_ui.cc:120:
> web_ui()->GetWebContents()->
> Another idea is for ConstrainedWebDialogUI to be a WebContentsObserver, and
> override WebContentsDestroyed(). Then you'll know to return nullptr here.
Though
> getting ClearConstrainedDelegate() to work right feels cleaner.

I like this idea. I will give it a try. Thanks!

[arthursonzogni, 2017-04-06 09:36:34]

On 2017/04/06 09:26:10, arthursonzogni wrote:
> > chrome/browser/ui/webui/constrained_web_dialog_ui.cc:120:
> > web_ui()->GetWebContents()->
> > Another idea is for ConstrainedWebDialogUI to be a WebContentsObserver, and
> > override WebContentsDestroyed(). Then you'll know to return nullptr here.
> Though
> > getting ClearConstrainedDelegate() to work right feels cleaner.
> 
> I like this idea. I will give it a try. Thanks!
I don't understand very well. The problem is when the WebContents is still alive
after the ConstrainedWebDialogDelegateBase has been destroyed. A
ConstrainedWebDialogDelegateBaseObserver maybe :-) ?

I will continue to work on the solution of ClearConstrainedDelegate().

[Lei Zhang, 2017-04-06 11:07:43]

On 2017/04/06 09:36:34, arthursonzogni wrote:
> I don't understand very well. The problem is when the WebContents is still
alive
> after the ConstrainedWebDialogDelegateBase has been destroyed. A
> ConstrainedWebDialogDelegateBaseObserver maybe :-) ?

I looked at too many Constrained* classes, and when I got back to this CL, I
thought the WebContent was the one that got destroyed first. Whoops, ignore.

[arthursonzogni, 2017-04-06 12:57:05]

On 2017/04/06 11:07:43, Lei Zhang wrote:
> On 2017/04/06 09:36:34, arthursonzogni wrote:
> > I don't understand very well. The problem is when the WebContents is still
> alive
> > after the ConstrainedWebDialogDelegateBase has been destroyed. A
> > ConstrainedWebDialogDelegateBaseObserver maybe :-) ?
> 
> I looked at too many Constrained* classes, and when I got back to this CL, I
> thought the WebContent was the one that got destroyed first. Whoops, ignore.

I found the problem.
In this test, the WebContent is deleted before the
ConstrainedWebDialogDelegateBase (e.g. The opposite of the initial problem).
I have added an WebContentObserver to fix the issue.

I am not very convinced of the final result. They are no more invalid pointers
at least, which is good.
I have the impression of adding a bandage without doing the full refactoring
that looks necessary.

Please take a look. Thanks!

[arthursonzogni, 2017-04-06 12:57:20]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 12:57:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 13:21:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 13:21:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[arthursonzogni, 2017-04-06 13:34:01]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[arthursonzogni, 2017-04-06 13:34:06]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-04-06 13:34:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 14:18:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 14:18:27]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-04-06 22:48:20]

On 2017/04/06 12:57:05, arthursonzogni wrote:
> On 2017/04/06 11:07:43, Lei Zhang wrote:
> > On 2017/04/06 09:36:34, arthursonzogni wrote:
> > > I don't understand very well. The problem is when the WebContents is still
> > alive
> > > after the ConstrainedWebDialogDelegateBase has been destroyed. A
> > > ConstrainedWebDialogDelegateBaseObserver maybe :-) ?
> > 
> > I looked at too many Constrained* classes, and when I got back to this CL, I
> > thought the WebContent was the one that got destroyed first. Whoops, ignore.
> 
> I found the problem.
> In this test, the WebContent is deleted before the
> ConstrainedWebDialogDelegateBase (e.g. The opposite of the initial problem).
> I have added an WebContentObserver to fix the issue.

I found the repro and can confirm this CL fixes the problem. I find it strange
that my advice became useful. :)

> I am not very convinced of the final result. They are no more invalid pointers
> at least, which is good.
> I have the impression of adding a bandage without doing the full refactoring
> that looks necessary.

I still like to understand the order of events to explain why this only happens
with PlzNavigate. Did we just get lucky this whole time before PlzNavigate? Or
is PlzNavigate misbehaving in some way to trigger this?

[Lei Zhang, 2017-04-07 00:55:22]

https://codereview.chromium.org/2798583002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):

https://codereview.chromium.org/2798583002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:123:
web_contents_.release();
This release() makes the ownership of |web_contents_| questionable. I'll try to
write a CL to make it cleaner.

The failing test in patch set 1 tries to simulate what's happening in the
browser, and the WebContents should outlive the ConstrainedWebDialog. However,
some implementation detail in ConstrainedWebDialogDelegateViewViews means even
though the test called OnDialogCloseFromWebUI(), the dialog does not actually
get destroyed until later.

[Lei Zhang, 2017-04-07 01:20:54]

On 2017/04/07 00:55:22, Lei Zhang wrote:
>
https://codereview.chromium.org/2798583002/diff/60001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc (right):
> 
>
https://codereview.chromium.org/2798583002/diff/60001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:123:
> web_contents_.release();
> This release() makes the ownership of |web_contents_| questionable. I'll try
to
> write a CL to make it cleaner.

Something like https://codereview.chromium.org/2806603003 is functionally
equivalent to patch set 3. WDYT?

[arthursonzogni, 2017-04-07 08:27:25]

On 2017/04/07 01:20:54, Lei Zhang wrote:
> On 2017/04/07 00:55:22, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/2798583002/diff/60001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc
(right):
> > 
> >
>
https://codereview.chromium.org/2798583002/diff/60001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/constrained_web_dialog_delegate_base.cc:123:
> > web_contents_.release();
> > This release() makes the ownership of |web_contents_| questionable. I'll try
> to
> > write a CL to make it cleaner.
> 
> Something like https://codereview.chromium.org/2806603003 is functionally
> equivalent to patch set 3. WDYT?

I look at https://codereview.chromium.org/2806603003 and it looks good to me.

On 2017/04/06 22:48:20, Lei Zhang wrote:
> On 2017/04/06 12:57:05, arthursonzogni wrote:
> > On 2017/04/06 11:07:43, Lei Zhang wrote:
> > > On 2017/04/06 09:36:34, arthursonzogni wrote:
> > > > I don't understand very well. The problem is when the WebContents is
still
> > > alive
> > > > after the ConstrainedWebDialogDelegateBase has been destroyed. A
> > > > ConstrainedWebDialogDelegateBaseObserver maybe :-) ?
> > > 
> > > I looked at too many Constrained* classes, and when I got back to this CL,
I
> > > thought the WebContent was the one that got destroyed first. Whoops,
ignore.
> > 
> > I found the problem.
> > In this test, the WebContent is deleted before the
> > ConstrainedWebDialogDelegateBase (e.g. The opposite of the initial problem).
> > I have added an WebContentObserver to fix the issue.
> 
> I found the repro and can confirm this CL fixes the problem. I find it strange
> that my advice became useful. :)

How do you repro the problem? By using --kiosk-printing ?
I have tried several times but it doesn't crash on my computer(Linux). What OS
are you using?

I am happy to know that this CL fixes the problem.

[arthursonzogni, 2017-04-10 09:12:36]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 09:12:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-04-10 09:18:02]

Hi Lei,

Do you think this can be committed?

[commit-bot: I haz the power, 2017-04-10 09:27:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 09:27:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[arthursonzogni, 2017-04-10 09:42:37]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 09:43:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-04-10 09:52:28]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 09:53:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 10:37:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 10:37:28]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-04-10 11:04:10]

Patchset #5 (id:100001) has been deleted

[Lei Zhang, 2017-04-10 17:49:47]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2017-04-10 17:49:48]

lgtm

https://codereview.chromium.org/2798583002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h (right):

https://codereview.chromium.org/2798583002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/constrained_web_dialog_delegate_base.h:67: // Pointer to
|web_contents_| that remains valid until it is destroyed.
This references itself. Let's fix it later.

[commit-bot: I haz the power, 2017-04-10 17:50:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 17:56:46]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491846587117640,
"parent_rev": "8088cc1ba912e866ea7c5daef7a7b4beba74525c", "commit_rev":
"e63206f4ef0c18bca991534852acd0193a2de94a"}

[commit-bot: I haz the power, 2017-04-10 17:57:35]

Description was changed from

==========
WebUI: prevent WebContent to hold an invalid pointer.

A ConstrainedWebDialogBase creates and own a WebContent. For various
reasons, it stores a pointer to itself inside the WebContent. The
problem was that the lifetime of the WebContent may exceed the one of
ConstrainedWebDialogBase when |release_contents_on_close_| is set to
false. The pointer becomes invalid.

BUG=704327
==========

to

==========
WebUI: prevent WebContent to hold an invalid pointer.

A ConstrainedWebDialogBase creates and own a WebContent. For various
reasons, it stores a pointer to itself inside the WebContent. The
problem was that the lifetime of the WebContent may exceed the one of
ConstrainedWebDialogBase when |release_contents_on_close_| is set to
false. The pointer becomes invalid.

BUG=704327

Review-Url: https://codereview.chromium.org/2798583002
Cr-Commit-Position: refs/heads/master@{#463318}
Committed:
https://chromium.googlesource.com/chromium/src/+/e63206f4ef0c18bca991534852ac...
==========

[commit-bot: I haz the power, 2017-04-10 17:57:36]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e63206f4ef0c18bca991534852ac...

[arthursonzogni, 2017-04-11 08:37:26]

Thanks!