Fix DownloadPermissionRequest crash when host_ disappears

Fix DownloadPermissionRequest crash when host_ disappears

A DownloadPermissionRequest may outlive its |host_|. If
GetRequestingHostname is called after the |host_| is gone, it still
needs to return a valid URL. This CL creates a |request_url_| member
variable for DownloadPermissionRequest, so that the URL doesn't die when
|host_| dies.

BUG=461752
Committed: https://crrev.com/239d7338d41a731b66a53aa145711f2c5d8009b1
Cr-Commit-Position: refs/heads/master@{#324302}

[felt, 2015-04-07 17:57:49]

felt@chromium.org changed reviewers:
+ rdsmith@chromium.org

[felt, 2015-04-07 17:58:01]

rdsmith, PTAL?

[Randy Smith (Not in Mondays), 2015-04-07 18:09:15]

I'll try to review this within the next day or so (tomorrow, probably, as I'm on
a plane today) but: Do you know if there's any test code that exercises this
into which a regression could be put?  

The major thing I want to understand is the circumstances in which
DownloadPermissionRequest can outlive its host and make sure there aren't
further lifetime bugs lurking there--the change itself looks pretty clear and
simple.

[Randy Smith (Not in Mondays), 2015-04-07 18:10:49]

Actually, just in case you know the code better than I do (wouldn't be hard):
Under what circumstances should a permission request outlive its host?  I
thought the TabDownloadState was supposed to stick around as long as a response
was being awaited from the user; did permissions bubbles change that?

[felt, 2015-04-07 20:08:27]

On 2015/04/07 18:10:49, rdsmith wrote:
> Actually, just in case you know the code better than I do (wouldn't be hard):
> Under what circumstances should a permission request outlive its host?  I
> thought the TabDownloadState was supposed to stick around as long as a
response
> was being awaited from the user; did permissions bubbles change that?

Disclaimer: I am unfamiliar with this code, as I did not write any of it; I am
simply trying to fix a crash. So this is the result of a recent investigation,
not a deep understanding of the code. With that disclaimer out of the way, here
is my current *theory* about what's causing the crash...

A) The PermissionBubbleManager keeps a queue of permission requests for a
WebContents. It lives as long as the WebContents does.
B) The DownloadRequestLimiter (host_) kills itself on
NOTIFICATION_NAV_ENTRY_PENDING.
C) A DownloadPermissionRequest, like all other PermissionBubbleRequests, should
be able to live as long as the PermissionBubbleManager.

The constraint in (C) is violated because the DownloadPermissionRequest depends
on host_, which may not live as long as the PermissionBubbleManager because of
(B). This CL removes that dependency.

[felt, 2015-04-07 20:09:38]

On 2015/04/07 20:08:27, felt wrote:
> On 2015/04/07 18:10:49, rdsmith wrote:
> > Actually, just in case you know the code better than I do (wouldn't be
hard):
> > Under what circumstances should a permission request outlive its host?  I
> > thought the TabDownloadState was supposed to stick around as long as a
> response
> > was being awaited from the user; did permissions bubbles change that?
> 
> Disclaimer: I am unfamiliar with this code, as I did not write any of it; I am
> simply trying to fix a crash. So this is the result of a recent investigation,
> not a deep understanding of the code. With that disclaimer out of the way,
here
> is my current *theory* about what's causing the crash...
> 
> A) The PermissionBubbleManager keeps a queue of permission requests for a
> WebContents. It lives as long as the WebContents does.
> B) The DownloadRequestLimiter (host_) kills itself on
> NOTIFICATION_NAV_ENTRY_PENDING.
> C) A DownloadPermissionRequest, like all other PermissionBubbleRequests,
should
> be able to live as long as the PermissionBubbleManager.
> 
> The constraint in (C) is violated because the DownloadPermissionRequest
depends
> on host_, which may not live as long as the PermissionBubbleManager because of
> (B). This CL removes that dependency.

And to be more concrete, I think this crash is happening when the user navigates
a tab without closing the request first. In that instance, the NAV_ENTRY_PENDING
event will hit the host_ before the WebContents destructs the
PermissionBubbleManager.

[Randy Smith (Not in Mondays), 2015-04-08 16:08:55]

That's plausible enough to me, based on my memories of the code, to LGTM the CL.

To me this does suggest a design flaw in the integration of permissions bubbles
with the DownloadRequestLimiter.  This is a conflict in lifetime semantics
between TabDownloadState and DownloadPermissionRequest; I wonder if we could
have gotten rid of TabDownloadState and moved its guts into
DownloadPermissionRequest, and thus not have had two object lifetimes to manage.
 But maybe DownloadPermissionRequest would sometimes violate the expectations of
DownloadRequestLimiter.  Not your problem, in any case.

[felt, 2015-04-08 19:55:34]

On 2015/04/08 16:08:55, rdsmith wrote:
> That's plausible enough to me, based on my memories of the code, to LGTM the
CL.
> 
> To me this does suggest a design flaw in the integration of permissions
bubbles
> with the DownloadRequestLimiter.  This is a conflict in lifetime semantics
> between TabDownloadState and DownloadPermissionRequest; I wonder if we could
> have gotten rid of TabDownloadState and moved its guts into
> DownloadPermissionRequest, and thus not have had two object lifetimes to
manage.
>  But maybe DownloadPermissionRequest would sometimes violate the expectations
of
> DownloadRequestLimiter.  Not your problem, in any case.

Yes, I think it would make sense to merge them. Filed
https://code.google.com/p/chromium/issues/detail?id=475167

[felt, 2015-04-08 19:56:44]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2015-04-08 19:58:17]

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

[commit-bot: I haz the power, 2015-04-08 23:03:39]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-08 23:04:23]

Patchset 1 (id:??) landed as
https://crrev.com/239d7338d41a731b66a53aa145711f2c5d8009b1
Cr-Commit-Position: refs/heads/master@{#324302}