Fix crash in MediaCaptureDevicesDispatcher::ProcessScreenCaptureAccessRequest.

Fix crash in MediaCaptureDevicesDispatcher::ProcessScreenCaptureAccessRequest.

The web contents may be destroyed while awaiting user approval from the modal
message box.

BUG=326690
TEST=trybot, verify crash does not happen on device.
R=fischman@chromium.org, sergeyu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239900

[hshi1, 2013-12-10 01:37:12]

PTAL - this prevents a crash due to the web contents being destroyed while we
wait in chrome::ShowMessageBox for user approval.

It still feels hacky to use a modal dialog as it just invites for trouble. In
the long term we should really re-design the UI, for example use the infobar.
Thoughts?

[hshi1, 2013-12-10 17:58:25]

+fischman@ for advice as well

I still don't feel comfortable with this situation even after the fix. The
pointer |web_contents| could be deleted at any time and in theory we could still
hit use-after-free problems.

[hshi1, 2013-12-10 18:24:25]

On 2013/12/10 17:58:25, hshi1 wrote:
> +fischman@ for advice as well
> 
> I still don't feel comfortable with this situation even after the fix. The
> pointer |web_contents| could be deleted at any time and in theory we could
still
> hit use-after-free problems.

Follow-up: I added some more debug logs to figure out the exact sequence of
events.

(1) We enter MediaCaptureDevicesDispatcher::ProcessScreenCaptureAccessRequest on
the UI thread with a valid |web_contents| pointer;
(2) While in the function, chrome::ShowMessageBox() is called to show a modal
dialog. On aura this is implemented in simple_message_box_views.cc:
ShowMessageBoxImpl(). This dialog starts a nested message loop inside the UI
thread's main message loop;
(3) While waiting for user approval on the modal dialog, the destructor of
|web_contents| is called on the UI thread's main message loop;
(4) Finally the modal dialog is closed and control returns to the remainder of
MediaCaptureDevicesDispatcher::ProcessScreenCaptureAccessRequest, but at this
time |web_contents| is already invalid.

So the problem here is that ShowMessageBox() runs in nested message loop. Even
though all (1)~(4) occur on the UI thread, it is possible for the d'tor of
|web_contents| to run in (3).

I believe this fix is adequate because it address the special case of
ShowMessageBox() - as long as we initialize the string |notification_text|
before ShowMessageBox, we would not crash because |web_contents| will not be
dereferenced afterwards.

[Ami GONE FROM CHROMIUM, 2013-12-10 18:45:51]

On 2013/12/10 17:58:25, hshi1 wrote:
> +fischman@ for advice as well
> 
> I still don't feel comfortable with this situation even after the fix. The
> pointer |web_contents| could be deleted at any time and in theory we could
still
> hit use-after-free problems.

I'm not familiar with UI code at all, but looking at the callstack in the linked
BUG, ISTM the problem is far higher up the chain than this file.  In particular
it looks like MediaStreamUIProxy::Core::RequestAccess is calling 
render_delegate->RequestMediaAccessPermission() on a render_delegate that's
already invalid.

What does ASAN have to say about your repro?

[Ami GONE FROM CHROMIUM, 2013-12-10 18:54:10]

On Tue, Dec 10, 2013 at 10:24 AM, <hshi@chromium.org> wrote:

> I believe this fix is adequate because it address the special case of
> ShowMessageBox() - as long as we initialize the string |notification_text|
> before ShowMessageBox, we would not crash because |web_contents| will not
> be
> dereferenced afterwards.
>

If your analysis is correct then your fix looks brittle to me, b/c there's
nothing that indicates that web_contents is bogus after SMB() returns.
Better options in increasing order of awesome:
- explicate by clearing (web_contents=NULL) before the call to SMB, along
with a comment about why you're doing it.
- split the containing function and make the second piece not even accept
web_contents as a param, with a comment explaining why.
- make web_contents not deletable inside a nested loop.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hshi1, 2013-12-10 19:00:43]

On 2013/12/10 18:45:51, Ami Fischman wrote:
> On 2013/12/10 17:58:25, hshi1 wrote:
> > +fischman@ for advice as well
> > 
> > I still don't feel comfortable with this situation even after the fix. The
> > pointer |web_contents| could be deleted at any time and in theory we could
> still
> > hit use-after-free problems.
> 
> I'm not familiar with UI code at all, but looking at the callstack in the
linked
> BUG, ISTM the problem is far higher up the chain than this file.  In
particular
> it looks like MediaStreamUIProxy::Core::RequestAccess is calling 
> render_delegate->RequestMediaAccessPermission() on a render_delegate that's
> already invalid.
> 
> What does ASAN have to say about your repro?

This is not true; the render_delegate is still valid at the time
render_delegate->RequestMediaAccessPermission() is invoked. Note the repro step
requires user to close the browser window while the modal dialog is still open,
this results in the destruction of web contents.

[Ami GONE FROM CHROMIUM, 2013-12-10 19:12:47]

Yeah, our emails crossed in the tubes.
See my second email.


On Tue, Dec 10, 2013 at 11:00 AM, <hshi@chromium.org> wrote:

> On 2013/12/10 18:45:51, Ami Fischman wrote:
>
>> On 2013/12/10 17:58:25, hshi1 wrote:
>> > +fischman@ for advice as well
>> >
>> > I still don't feel comfortable with this situation even after the fix.
>> The
>> > pointer |web_contents| could be deleted at any time and in theory we
>> could
>> still
>> > hit use-after-free problems.
>>
>
>  I'm not familiar with UI code at all, but looking at the callstack in the
>>
> linked
>
>> BUG, ISTM the problem is far higher up the chain than this file.  In
>>
> particular
>
>> it looks like MediaStreamUIProxy::Core::RequestAccess is calling
>> render_delegate->RequestMediaAccessPermission() on a render_delegate
>> that's
>> already invalid.
>>
>
>  What does ASAN have to say about your repro?
>>
>
> This is not true; the render_delegate is still valid at the time
> render_delegate->RequestMediaAccessPermission() is invoked. Note the
> repro step
> requires user to close the browser window while the modal dialog is still
> open,
> this results in the destruction of web contents.
>
> https://codereview.chromium.org/98233009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hshi1, 2013-12-10 19:56:53]

> If your analysis is correct then your fix looks brittle to me, b/c there's
> nothing that indicates that web_contents is bogus after SMB() returns.
> Better options in increasing order of awesome:
> - explicate by clearing (web_contents=NULL) before the call to SMB, along
> with a comment about why you're doing it.
> - split the containing function and make the second piece not even accept
> web_contents as a param, with a comment explaining why.
> - make web_contents not deletable inside a nested loop.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

PTAL patch set #2. I have split and refactored the functions
ProcessDesktopCaptureAccessRequest and ProcessScreenCaptureAccessRequest and
added comment to make it abundantly clear that we're not dereferencing
web_contents after the messagebox. Thanks!

[Ami GONE FROM CHROMIUM, 2013-12-10 20:18:28]

LGTM but IDK what I'm talking about here; you should wait for Sergey's say-so
(or anyone else who knows this code)

https://codereview.chromium.org/98233009/diff/50001/chrome/browser/media/medi...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/98233009/diff/50001/chrome/browser/media/medi...
chrome/browser/media/media_capture_devices_dispatcher.cc:362:
GetApplicationTitle(web_contents, extension);
Please follow this with 
web_contents = NULL;
to be safe.

[hshi1, 2013-12-10 20:22:36]

Note to sergeyu@ - your comments are appreciated. In particular please confirm
that this would be compatible with the DesktopMediaID refactor you're working on
in parallel.

https://codereview.chromium.org/98233009/diff/50001/chrome/browser/media/medi...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/98233009/diff/50001/chrome/browser/media/medi...
chrome/browser/media/media_capture_devices_dispatcher.cc:362:
GetApplicationTitle(web_contents, extension);
On 2013/12/10 20:18:28, Ami Fischman wrote:
> Please follow this with 
> web_contents = NULL;
> to be safe.

Done.

[Sergey Ulanov, 2013-12-10 22:36:08]

lgtm

[commit-bot: I haz the power, 2013-12-10 22:39:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/98233009/70001

[hshi1, 2013-12-10 23:01:15]

Committed patchset #3 manually as r239900 (presubmit successful).