Add int params to URLDataSource::StartDataRequest().

Change signature of URLDataSource::StartDataRequest().

Previously StartDataRequest() included a bool is_incognito argument for
a few data sources which needed to know that. In CL/13375003, this
method also needs to know the requesting render process id and render
view id. Since we can determine is_incognito from the process id, pass
the ints down instead of the bools.

This change populates the new params and updates all the affected
callsites, but does not include any other logic from 13375003.

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

[Jered, 2013-04-16 22:37:47]

jam -> content api change
others -> OWNERS stamp

[jam, 2013-04-16 23:15:30]

https://codereview.chromium.org/14039004/diff/1/content/public/browser/url_da...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/14039004/diff/1/content/public/browser/url_da...
content/public/browser/url_data_source.h:58: const ExtraRequestInfo& info,
Per the conventions for the Content API
(http://www.chromium.org/developers/content-module/content-api), each
struct/class/enum should be in a separate file. we also try to minimize the
number of files, so I would avoid the struct. this is made easier since
"is_incognito" is now redundant if you pass in the render_process_id and
render_view_is: one can figure out incognito by doing
RenderProcessHost::FromID(render_process_id)->GetBrowserContext()->IsOffTheRecord()

it looks like only 3 of the URLDataSource implementations use this boolean, so
it's pretty easy to convert them to the above.

[Jered, 2013-04-16 23:48:15]

quick question

https://codereview.chromium.org/14039004/diff/1/content/public/browser/url_da...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/14039004/diff/1/content/public/browser/url_da...
content/public/browser/url_data_source.h:58: const ExtraRequestInfo& info,
On 2013/04/16 23:15:30, jam wrote:
> Per the conventions for the Content API
> (http://www.chromium.org/developers/content-module/content-api), each
> struct/class/enum should be in a separate file. we also try to minimize the
> number of files, so I would avoid the struct. this is made easier since
> "is_incognito" is now redundant if you pass in the render_process_id and
> render_view_is: one can figure out incognito by doing
>
RenderProcessHost::FromID(render_process_id)->GetBrowserContext()->IsOffTheRecord()
One of the callees who wants to check is_incognito is on the IO thread, but
RenderProcessHost::FromID is only allowed on the UI thread. That caller is just
doing a DCHECK, and I think I can convince the owner to remove it (it's Instant
code); but what would you prefer?
> 
> it looks like only 3 of the URLDataSource implementations use this boolean, so
> it's pretty easy to convert them to the above.

[Jered, 2013-04-17 00:19:34]

PS2 has the int version.

dhollowa/estade, note that ThemeSource no longer DCHECKs about is_incognito. But
I think that was added during implementation for sanity checking and is not
buying us much now. Are you ok with this?

On 2013/04/16 23:48:15, Jered wrote:
> quick question
> 
>
https://codereview.chromium.org/14039004/diff/1/content/public/browser/url_da...
> File content/public/browser/url_data_source.h (right):
> 
>
https://codereview.chromium.org/14039004/diff/1/content/public/browser/url_da...
> content/public/browser/url_data_source.h:58: const ExtraRequestInfo& info,
> On 2013/04/16 23:15:30, jam wrote:
> > Per the conventions for the Content API
> > (http://www.chromium.org/developers/content-module/content-api), each
> > struct/class/enum should be in a separate file. we also try to minimize the
> > number of files, so I would avoid the struct. this is made easier since
> > "is_incognito" is now redundant if you pass in the render_process_id and
> > render_view_is: one can figure out incognito by doing
> >
>
RenderProcessHost::FromID(render_process_id)->GetBrowserContext()->IsOffTheRecord()
> One of the callees who wants to check is_incognito is on the IO thread, but
> RenderProcessHost::FromID is only allowed on the UI thread. That caller is
just
> doing a DCHECK, and I think I can convince the owner to remove it (it's
Instant
> code); but what would you prefer?
> > 
> > it looks like only 3 of the URLDataSource implementations use this boolean,
so
> > it's pretty easy to convert them to the above.

[jam, 2013-04-17 00:45:08]

re the DCHECK, agreed better to take it out given that it wouldn't be convenient
to get this data on the IO thread

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/ap...
File chrome/browser/ui/webui/app_launcher_page_ui.cc (right):

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/ap...
chrome/browser/ui/webui/app_launcher_page_ui.cc:108: DCHECK(render_host !=
NULL);
nit: remove this, it's redundant. in release builds, it does nothing. in debug,
a crash in the next line is just as strong of a signal to the developer

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/ap...
chrome/browser/ui/webui/app_launcher_page_ui.cc:109: const bool is_incognito =
render_host->GetBrowserContext()->IsOffTheRecord();
personally, i think the const is too much. but this isn't in the style guide, so
up to you

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/de...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/de...
chrome/browser/ui/webui/devtools_ui.cc:187: const
content::URLDataSource::ExtraRequestInfo& info,
nit: you forgot to update this

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:352: DCHECK(render_host != NULL);
ditto

https://codereview.chromium.org/14039004/diff/6001/content/browser/webui/url_...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/14039004/diff/6001/content/browser/webui/url_...
content/browser/webui/url_data_manager_backend.cc:514: if (info)
when could this be null?

I'm not sure this can actually happens. i.e. if the tab is gone, it cancels all
its requests.

[Jered, 2013-04-17 01:03:07]

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/ap...
File chrome/browser/ui/webui/app_launcher_page_ui.cc (right):

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/ap...
chrome/browser/ui/webui/app_launcher_page_ui.cc:108: DCHECK(render_host !=
NULL);
On 2013/04/17 00:45:09, jam wrote:
> nit: remove this, it's redundant. in release builds, it does nothing. in
debug,
> a crash in the next line is just as strong of a signal to the developer

Done.

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/ap...
chrome/browser/ui/webui/app_launcher_page_ui.cc:109: const bool is_incognito =
render_host->GetBrowserContext()->IsOffTheRecord();
On 2013/04/17 00:45:09, jam wrote:
> personally, i think the const is too much. but this isn't in the style guide,
so
> up to you

Removed. I have no strong preference, just habit.

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/de...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/de...
chrome/browser/ui/webui/devtools_ui.cc:187: const
content::URLDataSource::ExtraRequestInfo& info,
On 2013/04/17 00:45:09, jam wrote:
> nit: you forgot to update this

Done.

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

https://codereview.chromium.org/14039004/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:352: DCHECK(render_host != NULL);
On 2013/04/17 00:45:09, jam wrote:
> ditto

Done.

https://codereview.chromium.org/14039004/diff/6001/content/browser/webui/url_...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/14039004/diff/6001/content/browser/webui/url_...
content/browser/webui/url_data_manager_backend.cc:514: if (info)
On 2013/04/17 00:45:09, jam wrote:
> when could this be null?
> 
> I'm not sure this can actually happens. i.e. if the tab is gone, it cancels
all
> its requests.

How about this way? I think it's clearer anyway.

[Jói, 2013-04-17 09:31:32]

Removing myself as reviewer; jam@ is already reviewing and can approve for
content/public.

Cheers,
Jói

[jam, 2013-04-17 16:28:17]

lgtm

[dhollowa, 2013-04-17 22:32:36]

LGTM.  Please update descriptions of this CL though (no POD anymore) and include
BUG=223608.

[Jered, 2013-04-17 23:27:08]

ping estade/gene?

On 2013/04/17 22:32:36, dhollowa wrote:
> LGTM.  Please update descriptions of this CL though (no POD anymore) and
include
> BUG=223608.

Done.

[gene, 2013-04-17 23:30:36]

lgtm for cloudprint

On 2013/04/17 23:27:08, Jered wrote:
> ping estade/gene?
> 
> On 2013/04/17 22:32:36, dhollowa wrote:
> > LGTM.  Please update descriptions of this CL though (no POD anymore) and
> include
> > BUG=223608.
> 
> Done.

[Jered, 2013-04-18 15:43:07]

Thanks gene. Evan can you do this quick webui review or should I ping a
different OWNER?
Paweł -> chrome/test/base

[Paweł Hajdan Jr., 2013-04-18 17:42:23]

chrome/test/base LGTM

[Evan Stade, 2013-04-18 18:51:28]

lgtm

[Jered, 2013-04-18 18:53:57]

On 2013/04/18 18:51:28, Evan Stade wrote:
> lgtm

Thanks.

[commit-bot: I haz the power, 2013-04-18 18:55:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/14039004/30001

[Jered, 2013-04-18 21:04:10]

jam are you ok with a null check in PS7? browsertest found that the renderhost
here can be null when draining the message loop during shutdown.

On 2013/04/18 18:55:43, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/jered%40chromium.org/14039004/30001

[jam, 2013-04-19 00:34:45]

On 2013/04/18 21:04:10, Jered wrote:
> jam are you ok with a null check in PS7? browsertest found that the renderhost
> here can be null when draining the message loop during shutdown.

it probably can happen in normal chrome as well? (someone more familiar with
this code should verify, or you can try to simulate this through creative usage
of breakpoints and pausing threads). if the tab is closed, and we post a task to
the IO thread to cancel its requests, but the IO thread had already posted a
task to the UI thread to get the data.

it seems that URLDataManagerBackend can check this itself, if the target thread
happens to be the UI thread.

[jam, 2013-04-19 00:35:22]

btw another question is what happens if the callback isn't called? i.e. will
memory be reserved forever?

[Jered, 2013-04-19 02:14:09]

On 2013/04/19 00:35:22, jam wrote:
> btw another question is what happens if the callback isn't called? i.e. will
> memory be reserved forever?

The comment in url_data_source.h says it should be called, so I'll call it in
both places where new_tab_ui.cc can return early. I think something would
otherwise end up stuck in pending_requests_ unless URLRequestChromeJob magically
cleans that up.

[Jered, 2013-04-19 04:33:05]

On 2013/04/19 00:34:45, jam wrote:
> On 2013/04/18 21:04:10, Jered wrote:
> > jam are you ok with a null check in PS7? browsertest found that the
renderhost
> > here can be null when draining the message loop during shutdown.
> 
> it probably can happen in normal chrome as well? (someone more familiar with
> this code should verify, or you can try to simulate this through creative
usage
> of breakpoints and pausing threads). if the tab is closed, and we post a task
to
> the IO thread to cancel its requests, but the IO thread had already posted a
> task to the UI thread to get the data.
> 
> it seems that URLDataManagerBackend can check this itself, if the target
thread
> happens to be the UI thread.

So in InProcessBrowserTest, I am seeing the IO thread still running at the same
time chrome::AttemptExit() is closing windows on the UI thread and getting rid
of render process hosts. In the IO thread, the URLDataManagerBackend happily
posts another task on the UI loop which gets run _after_ AttemptExit() has
dismantled the render process host, despite the comment at
in_process_browsertest.cc:404 which claims that AttemptExit() exits the message
loop.

I don't see an obvious nonhacky way for URLDataManagerBackend to know that, at
this point, it should not be running. If anything it seems like
InProcessBrowserTest is not performing a proper shutdown, since the comment
seems wrong, and the message loop should really be flushed immediately after
AttemptExit().

About normal Chrome, from reading the code, I think that due to "fast shutdown"
on a tab close the render process host would go away before the URLRequests are
cancelled, so yes, this is probably possible on tab close also. But I can't find
the parts that actually do the canceling so I am not 100% sure. Again, I'm not
sure how to tell this in URLDataManagerBackend... I think the check would
actually have to be on the UI thread prior to the tasks it's posted there.

So in conclusion, I kinda prefer the null check or adding back a bool
is_incognito to avoid all this really confusing behavior.

[jam, 2013-04-19 18:33:49]

On 2013/04/19 04:33:05, Jered wrote:
> On 2013/04/19 00:34:45, jam wrote:
> > On 2013/04/18 21:04:10, Jered wrote:
> > > jam are you ok with a null check in PS7? browsertest found that the
> renderhost
> > > here can be null when draining the message loop during shutdown.
> > 
> > it probably can happen in normal chrome as well? (someone more familiar with
> > this code should verify, or you can try to simulate this through creative
> usage
> > of breakpoints and pausing threads). if the tab is closed, and we post a
task
> to
> > the IO thread to cancel its requests, but the IO thread had already posted a
> > task to the UI thread to get the data.
> > 
> > it seems that URLDataManagerBackend can check this itself, if the target
> thread
> > happens to be the UI thread.
> 
> So in InProcessBrowserTest, I am seeing the IO thread still running at the
same
> time chrome::AttemptExit() is closing windows on the UI thread and getting rid
> of render process hosts. In the IO thread, the URLDataManagerBackend happily
> posts another task on the UI loop which gets run _after_ AttemptExit() has
> dismantled the render process host, despite the comment at
> in_process_browsertest.cc:404 which claims that AttemptExit() exits the
message
> loop.
> 
> I don't see an obvious nonhacky way for URLDataManagerBackend to know that, at
> this point, it should not be running. If anything it seems like
> InProcessBrowserTest is not performing a proper shutdown, since the comment
> seems wrong, and the message loop should really be flushed immediately after
> AttemptExit().
> 
> About normal Chrome, from reading the code, I think that due to "fast
shutdown"
> on a tab close the render process host would go away before the URLRequests
are
> cancelled, so yes, this is probably possible on tab close also.

fast shutdown just affects whether renderers are killed or allowed to close
gracefully. it shouldn't impact this.

> But I can't find
> the parts that actually do the canceling so I am not 100% sure. Again, I'm not
> sure how to tell this in URLDataManagerBackend... I think the check would
> actually have to be on the UI thread prior to the tasks it's posted there.
> 
> So in conclusion, I kinda prefer the null check

the problem then is that any other place that starts using this might have racy
crashes. why null check in all places instead of just one?

> or adding back a bool
> is_incognito to avoid all this really confusing behavior.

this is a non-starter. if you want to add new parameters to this callback, they
can't be redundant with existing ones

[Jered, 2013-04-19 18:39:03]

On 2013/04/19 18:33:49, jam wrote:
> On 2013/04/19 04:33:05, Jered wrote:
> > On 2013/04/19 00:34:45, jam wrote:
> > > On 2013/04/18 21:04:10, Jered wrote:
> > > > jam are you ok with a null check in PS7? browsertest found that the
> > renderhost
> > > > here can be null when draining the message loop during shutdown.
> > > 
> > > it probably can happen in normal chrome as well? (someone more familiar
with
> > > this code should verify, or you can try to simulate this through creative
> > usage
> > > of breakpoints and pausing threads). if the tab is closed, and we post a
> task
> > to
> > > the IO thread to cancel its requests, but the IO thread had already posted
a
> > > task to the UI thread to get the data.
> > > 
> > > it seems that URLDataManagerBackend can check this itself, if the target
> > thread
> > > happens to be the UI thread.
> > 
> > So in InProcessBrowserTest, I am seeing the IO thread still running at the
> same
> > time chrome::AttemptExit() is closing windows on the UI thread and getting
rid
> > of render process hosts. In the IO thread, the URLDataManagerBackend happily
> > posts another task on the UI loop which gets run _after_ AttemptExit() has
> > dismantled the render process host, despite the comment at
> > in_process_browsertest.cc:404 which claims that AttemptExit() exits the
> message
> > loop.
> > 
> > I don't see an obvious nonhacky way for URLDataManagerBackend to know that,
at
> > this point, it should not be running. If anything it seems like
> > InProcessBrowserTest is not performing a proper shutdown, since the comment
> > seems wrong, and the message loop should really be flushed immediately after
> > AttemptExit().
> > 
> > About normal Chrome, from reading the code, I think that due to "fast
> shutdown"
> > on a tab close the render process host would go away before the URLRequests
> are
> > cancelled, so yes, this is probably possible on tab close also.
> 
> fast shutdown just affects whether renderers are killed or allowed to close
> gracefully. it shouldn't impact this.
> 
> > But I can't find
> > the parts that actually do the canceling so I am not 100% sure. Again, I'm
not
> > sure how to tell this in URLDataManagerBackend... I think the check would
> > actually have to be on the UI thread prior to the tasks it's posted there.
> > 
> > So in conclusion, I kinda prefer the null check
> 
> the problem then is that any other place that starts using this might have
racy
> crashes. why null check in all places instead of just one?

How about we look up the RPH and do a null check in URLDataManagerBackend on the
UI thread prior to calling StartDataRequest()? If it failed, we would not call
StartDataRequest().

> 
> > or adding back a bool
> > is_incognito to avoid all this really confusing behavior.
> 
> this is a non-starter. if you want to add new parameters to this callback,
they
> can't be redundant with existing ones

[jam, 2013-04-19 21:26:17]

On 2013/04/19 18:39:03, Jered wrote:
> How about we look up the RPH and do a null check in URLDataManagerBackend on
the
> UI thread prior to calling StartDataRequest()? If it failed, we would not call
> StartDataRequest().

sgtm (that's what I tried to say, perhaps not clearly, in comment 18)

[Jered, 2013-04-19 22:14:17]

On 2013/04/19 21:26:17, jam wrote:
> On 2013/04/19 18:39:03, Jered wrote:
> > How about we look up the RPH and do a null check in URLDataManagerBackend on
> the
> > UI thread prior to calling StartDataRequest()? If it failed, we would not
call
> > StartDataRequest().
> 
> sgtm (that's what I tried to say, perhaps not clearly, in comment 18)

Ok, done.

[Jered, 2013-04-22 15:48:40]

jam does PS10 resolve this for you?

On 2013/04/19 22:14:17, Jered wrote:
> On 2013/04/19 21:26:17, jam wrote:
> > On 2013/04/19 18:39:03, Jered wrote:
> > > How about we look up the RPH and do a null check in URLDataManagerBackend
on
> > the
> > > UI thread prior to calling StartDataRequest()? If it failed, we would not
> call
> > > StartDataRequest().
> > 
> > sgtm (that's what I tried to say, perhaps not clearly, in comment 18)
> 
> Ok, done.

[jam, 2013-04-22 18:17:09]

lgtm with comment

https://codereview.chromium.org/14039004/diff/13002/content/browser/webui/url...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/14039004/diff/13002/content/browser/webui/url...
content/browser/webui/url_data_manager_backend.cc:561: if
(content::RenderProcessHost::FromID(render_process_id) == NULL) {
this is called on whatever target message loop the source gives us. i.e. it's
not necessarily the UI loop. so surround this check with

if (BrowserThread::CurrentlyOn(BrowserThread::UI))

nits:
-no content:: since this is is in the content namespace
-convention is not to compare to NULL, i.e. just
if (BrowserThread::CurrentlyOn(BrowserThread::UI) &&
    !RenderProcessHost::FromID(render_process_id))

[Jered, 2013-04-22 20:46:54]

Thanks.

https://codereview.chromium.org/14039004/diff/13002/content/browser/webui/url...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/14039004/diff/13002/content/browser/webui/url...
content/browser/webui/url_data_manager_backend.cc:561: if
(content::RenderProcessHost::FromID(render_process_id) == NULL) {
On 2013/04/22 18:17:10, jam wrote:
> this is called on whatever target message loop the source gives us. i.e. it's
> not necessarily the UI loop. so surround this check with
> 
> if (BrowserThread::CurrentlyOn(BrowserThread::UI))
> 
> nits:
> -no content:: since this is is in the content namespace
> -convention is not to compare to NULL, i.e. just
> if (BrowserThread::CurrentlyOn(BrowserThread::UI) &&
>     !RenderProcessHost::FromID(render_process_id))

Done.

[commit-bot: I haz the power, 2013-04-22 20:53:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/14039004/100001

[commit-bot: I haz the power, 2013-04-22 23:43:49]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-23 14:45:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/14039004/100001

[commit-bot: I haz the power, 2013-04-23 22:45:05]

Change committed as 195932