Componentize component_updater: Split content::ResourceThrottle from CUS.

Componentize component_updater: Split content::ResourceThrottle from CUS.

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

[tommycli, 2014-07-29 17:49:49]

sorin: Sorry to bombard you. Here's the follow-up change that splits CUS from
ResourceThrottle.

[tommycli, 2014-07-30 18:05:51]

+blundell on this patch

[Sorin Jianu, 2014-07-30 18:22:02]

Thank you Tommy!
Does this change breaks the friendship established by the friend declarations in
component_updater_service.h ?

[tommycli, 2014-07-30 18:44:12]

On 2014/07/30 18:22:02, Sorin Jianu wrote:
> Thank you Tommy!
> Does this change breaks the friendship established by the friend declarations
in
> component_updater_service.h ?

Hi. The friendships that used to be in OnDemandUpdater are no longer needed
because those methods are now public.

[Sorin Jianu, 2014-07-30 19:07:33]

On 2014/07/30 18:44:12, tommycli wrote:
> On 2014/07/30 18:22:02, Sorin Jianu wrote:
> > Thank you Tommy!
> > Does this change breaks the friendship established by the friend
declarations
> in
> > component_updater_service.h ?
> 
> Hi. The friendships that used to be in OnDemandUpdater are no longer needed
> because those methods are now public.

Tommy, the idea was to restrict who can call those functions to the entities
declared as friends.

[tommycli, 2014-07-30 19:11:43]

On 2014/07/30 19:07:33, Sorin Jianu wrote:
> On 2014/07/30 18:44:12, tommycli wrote:
> > On 2014/07/30 18:22:02, Sorin Jianu wrote:
> > > Thank you Tommy!
> > > Does this change breaks the friendship established by the friend
> declarations
> > in
> > > component_updater_service.h ?
> > 
> > Hi. The friendships that used to be in OnDemandUpdater are no longer needed
> > because those methods are now public.
> 
> Tommy, the idea was to restrict who can call those functions to the entities
> declared as friends.

Oh okay, I'll move those back to private and restore friendships after I get
some lunch.

[tommycli, 2014-07-30 20:25:21]

sorin: I moved the methods we discussed back into private. Thanks!

[Sorin Jianu, 2014-07-31 00:16:01]

Thank you Tommy!

https://codereview.chromium.org/421393002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (left):

https://codereview.chromium.org/421393002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:191: class
OnDemandUpdater {
The idea here is that we want to have a class, where we can add friend
declarations for other classes or functions which are allowed to invoke the
on-demand functionality. We don't want to friend the whole service class (since
the friendship is not selective once it has been granted). We do this to allow
some level of control to which components are allowed to call OnDemandUpdate. So
far no exception has been granted but many have asked for one.

We'd like to keep this class if possible.

[tommycli, 2014-07-31 18:00:05]

https://codereview.chromium.org/421393002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (left):

https://codereview.chromium.org/421393002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:191: class
OnDemandUpdater {
On 2014/07/31 00:16:01, Sorin Jianu wrote:
> The idea here is that we want to have a class, where we can add friend
> declarations for other classes or functions which are allowed to invoke the
> on-demand functionality. We don't want to friend the whole service class
(since
> the friendship is not selective once it has been granted). We do this to allow
> some level of control to which components are allowed to call OnDemandUpdate.
So
> far no exception has been granted but many have asked for one.
> 
> We'd like to keep this class if possible.

Done. Makes sense. I restored this class.

I left MaybeThrottle within CUS, since it makes CUResourceThrottle a bit
simpler. Let me know if you want this moved back also.

[Sorin Jianu, 2014-08-01 00:03:06]

lgtm

Thank you Tommy. Please better document the callback mechanism. I've also
included FYI cpu@, the creator of throttling for on demand updates

https://codereview.chromium.org/421393002/diff/80001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/421393002/diff/80001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:176: // is
downloaded and installed. |callback| may be called synchronously
We need to better document the intention and the functionality of the
|callback|.

[waffles, 2014-08-01 16:45:43]

tommy, before you submit, could you please manually verify that the pnacl
throttling still works?

Here's the steps to do so:
#0) on your local build, install the extension
https://chrome.google.com/webstore/detail/pnacl-examples/mblemkccghnfkjignlmg...
#1) delete %USER_DATA_DIR%/pnacl
#2) delete out/Debug/pnacl
#3) delete out/Release/pnacl
#4) launch chrome, the extension should show in apps, launch it, run some
examples - make sure they seem to be working properly.

[tommycli, 2014-08-05 21:29:58]

On 2014/08/01 16:45:43, waffles wrote:
> tommy, before you submit, could you please manually verify that the pnacl
> throttling still works?
> 
> Here's the steps to do so:
> #0) on your local build, install the extension
>
https://chrome.google.com/webstore/detail/pnacl-examples/mblemkccghnfkjignlmg...
> #1) delete %USER_DATA_DIR%/pnacl
> #2) delete out/Debug/pnacl
> #3) delete out/Release/pnacl
> #4) launch chrome, the extension should show in apps, launch it, run some
> examples - make sure they seem to be working properly.

waffles:

I was not able to make that work even with a fresh origin/master checkout.

- I deleted the built-in pnacl components, both in out/Debug/pnacl,
out/Debug/pnacl, and ~/.config/chromium/pnacl
- The ResourceThrottle was never triggered as I never got a resource_type ==
content::RESOURCE_TYPE_OBJECT
- When I manually updated the component, I would get this error:

[3,2943989504:21:26:47.630835] NaClAppRuntimeHostSetup: too late
[3,2943989504:21:26:47.631219] NaClAppDescQuotaSetup: too late
[3,3208333056:21:26:47.631291] NaClAppStartModule: error loading module
[3,3208333056:21:26:47.631498] reap logs

POST-ABORT: LOG_FATAL abort exit

** Signal 6 from trusted code: pc=7f17bc9d94f5
[SRPC:HOST:35,2253777280:14:26:47.632478]
NaClSrpcRpcWait(channel=0x1b31cc5f6540): EOF is received instead of response.
Probably, the other side (usually, nacl module or browser plugin) crashed.
[35,2253777280:14:26:47.632568] Reverse service thread will pick up crash log
[16968:16997:0805/142647:ERROR:nacl_process_host.cc(311)] NaCl process exited
with status 64000 (0xfa00)
[16968:16997:0805/142650:ERROR:pnacl_host.cc(448)] TranslationFinished:
TranslationID 7,-1889557391 not found.

- I'm on Linux, "GYP_DEFINES": "clang=1 component=shared_library use_goma=1". Am
I doing something wrong?

[tommycli, 2014-08-05 21:30:25]

sorin: see below comment

https://codereview.chromium.org/421393002/diff/80001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/421393002/diff/80001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:176: // is
downloaded and installed. |callback| may be called synchronously
On 2014/08/01 00:03:06, Sorin Jianu wrote:
> We need to better document the intention and the functionality of the
> |callback|.

I rewrote parts of the comment. It's a bit long now, Let me know what changes to
make...

After writing the comment, I feel like we should rename this method.

[tommycli, 2014-08-05 22:54:45]

On 2014/08/05 21:29:58, tommycli wrote:
> On 2014/08/01 16:45:43, waffles wrote:
> > tommy, before you submit, could you please manually verify that the pnacl
> > throttling still works?
> > 
> > Here's the steps to do so:
> > #0) on your local build, install the extension
> >
>
https://chrome.google.com/webstore/detail/pnacl-examples/mblemkccghnfkjignlmg...
> > #1) delete %USER_DATA_DIR%/pnacl
> > #2) delete out/Debug/pnacl
> > #3) delete out/Release/pnacl
> > #4) launch chrome, the extension should show in apps, launch it, run some
> > examples - make sure they seem to be working properly.
> 
> waffles:
> 
> I was not able to make that work even with a fresh origin/master checkout.
> 
> - I deleted the built-in pnacl components, both in out/Debug/pnacl,
> out/Debug/pnacl, and ~/.config/chromium/pnacl
> - The ResourceThrottle was never triggered as I never got a resource_type ==
> content::RESOURCE_TYPE_OBJECT
> - When I manually updated the component, I would get this error:
> 
> [3,2943989504:21:26:47.630835] NaClAppRuntimeHostSetup: too late
> [3,2943989504:21:26:47.631219] NaClAppDescQuotaSetup: too late
> [3,3208333056:21:26:47.631291] NaClAppStartModule: error loading module
> [3,3208333056:21:26:47.631498] reap logs
> 
> POST-ABORT: LOG_FATAL abort exit
> 
> ** Signal 6 from trusted code: pc=7f17bc9d94f5
> [SRPC:HOST:35,2253777280:14:26:47.632478]
> NaClSrpcRpcWait(channel=0x1b31cc5f6540): EOF is received instead of response.
> Probably, the other side (usually, nacl module or browser plugin) crashed.
> [35,2253777280:14:26:47.632568] Reverse service thread will pick up crash log
> [16968:16997:0805/142647:ERROR:nacl_process_host.cc(311)] NaCl process exited
> with status 64000 (0xfa00)
> [16968:16997:0805/142650:ERROR:pnacl_host.cc(448)] TranslationFinished:
> TranslationID 7,-1889557391 not found.
> 
> - I'm on Linux, "GYP_DEFINES": "clang=1 component=shared_library use_goma=1".
Am
> I doing something wrong?

waffles:

I did some more investigation. The NaCl error only appears if I don't delete the
~/.config/chromium directory between attempts. After deleting that, between
attempts the error is gone. Here is the behavior I see now on origin/master:

On first run:
 - If I didn't delete the out/Debug/pnacl directory, everything works.
 - If I deleted out/Debug/pnacl, I get: NativeClient: The Portable Native Client
(pnacl) component is not installed. Please consult chrome://components for more
information.
 - If I deleted out/Debug/pnacl, and then went to chrome://components and
manually trigger an on-demand update for pnacl, it works.
 - Under no circumstances is an OnDemandResourceThrottle requested.

[waffles, 2014-08-05 23:10:55]

On 2014/08/05 22:54:45, tommycli wrote:
> On 2014/08/05 21:29:58, tommycli wrote:
> > On 2014/08/01 16:45:43, waffles wrote:
> > > tommy, before you submit, could you please manually verify that the pnacl
> > > throttling still works?
> > > 
> > > Here's the steps to do so:
> > > #0) on your local build, install the extension
> > >
> >
>
https://chrome.google.com/webstore/detail/pnacl-examples/mblemkccghnfkjignlmg...
> > > #1) delete %USER_DATA_DIR%/pnacl
> > > #2) delete out/Debug/pnacl
> > > #3) delete out/Release/pnacl
> > > #4) launch chrome, the extension should show in apps, launch it, run some
> > > examples - make sure they seem to be working properly.
> > 
> > waffles:
> > 
> > I was not able to make that work even with a fresh origin/master checkout.
> > 
> > - I deleted the built-in pnacl components, both in out/Debug/pnacl,
> > out/Debug/pnacl, and ~/.config/chromium/pnacl
> > - The ResourceThrottle was never triggered as I never got a resource_type ==
> > content::RESOURCE_TYPE_OBJECT
> > - When I manually updated the component, I would get this error:
> > 
> > [3,2943989504:21:26:47.630835] NaClAppRuntimeHostSetup: too late
> > [3,2943989504:21:26:47.631219] NaClAppDescQuotaSetup: too late
> > [3,3208333056:21:26:47.631291] NaClAppStartModule: error loading module
> > [3,3208333056:21:26:47.631498] reap logs
> > 
> > POST-ABORT: LOG_FATAL abort exit
> > 
> > ** Signal 6 from trusted code: pc=7f17bc9d94f5
> > [SRPC:HOST:35,2253777280:14:26:47.632478]
> > NaClSrpcRpcWait(channel=0x1b31cc5f6540): EOF is received instead of
response.
> > Probably, the other side (usually, nacl module or browser plugin) crashed.
> > [35,2253777280:14:26:47.632568] Reverse service thread will pick up crash
log
> > [16968:16997:0805/142647:ERROR:nacl_process_host.cc(311)] NaCl process
exited
> > with status 64000 (0xfa00)
> > [16968:16997:0805/142650:ERROR:pnacl_host.cc(448)] TranslationFinished:
> > TranslationID 7,-1889557391 not found.
> > 
> > - I'm on Linux, "GYP_DEFINES": "clang=1 component=shared_library
use_goma=1".
> Am
> > I doing something wrong?
> 
> waffles:
> 
> I did some more investigation. The NaCl error only appears if I don't delete
the
> ~/.config/chromium directory between attempts. After deleting that, between
> attempts the error is gone. Here is the behavior I see now on origin/master:
> 
> On first run:
>  - If I didn't delete the out/Debug/pnacl directory, everything works.
>  - If I deleted out/Debug/pnacl, I get: NativeClient: The Portable Native
Client
> (pnacl) component is not installed. Please consult chrome://components for
more
> information.
>  - If I deleted out/Debug/pnacl, and then went to chrome://components and
> manually trigger an on-demand update for pnacl, it works.
>  - Under no circumstances is an OnDemandResourceThrottle requested.

Hm. This also appears to be the case with a straight LKGR build. I guess the
feature is broken.

[tommycli, 2014-08-05 23:26:35]

On 2014/08/05 23:10:55, waffles wrote:
> On 2014/08/05 22:54:45, tommycli wrote:
> > On 2014/08/05 21:29:58, tommycli wrote:
> > > On 2014/08/01 16:45:43, waffles wrote:
> > > > tommy, before you submit, could you please manually verify that the
pnacl
> > > > throttling still works?
> > > > 
> > > > Here's the steps to do so:
> > > > #0) on your local build, install the extension
> > > >
> > >
> >
>
https://chrome.google.com/webstore/detail/pnacl-examples/mblemkccghnfkjignlmg...
> > > > #1) delete %USER_DATA_DIR%/pnacl
> > > > #2) delete out/Debug/pnacl
> > > > #3) delete out/Release/pnacl
> > > > #4) launch chrome, the extension should show in apps, launch it, run
some
> > > > examples - make sure they seem to be working properly.
> > > 
> > > waffles:
> > > 
> > > I was not able to make that work even with a fresh origin/master checkout.
> > > 
> > > - I deleted the built-in pnacl components, both in out/Debug/pnacl,
> > > out/Debug/pnacl, and ~/.config/chromium/pnacl
> > > - The ResourceThrottle was never triggered as I never got a resource_type
==
> > > content::RESOURCE_TYPE_OBJECT
> > > - When I manually updated the component, I would get this error:
> > > 
> > > [3,2943989504:21:26:47.630835] NaClAppRuntimeHostSetup: too late
> > > [3,2943989504:21:26:47.631219] NaClAppDescQuotaSetup: too late
> > > [3,3208333056:21:26:47.631291] NaClAppStartModule: error loading module
> > > [3,3208333056:21:26:47.631498] reap logs
> > > 
> > > POST-ABORT: LOG_FATAL abort exit
> > > 
> > > ** Signal 6 from trusted code: pc=7f17bc9d94f5
> > > [SRPC:HOST:35,2253777280:14:26:47.632478]
> > > NaClSrpcRpcWait(channel=0x1b31cc5f6540): EOF is received instead of
> response.
> > > Probably, the other side (usually, nacl module or browser plugin) crashed.
> > > [35,2253777280:14:26:47.632568] Reverse service thread will pick up crash
> log
> > > [16968:16997:0805/142647:ERROR:nacl_process_host.cc(311)] NaCl process
> exited
> > > with status 64000 (0xfa00)
> > > [16968:16997:0805/142650:ERROR:pnacl_host.cc(448)] TranslationFinished:
> > > TranslationID 7,-1889557391 not found.
> > > 
> > > - I'm on Linux, "GYP_DEFINES": "clang=1 component=shared_library
> use_goma=1".
> > Am
> > > I doing something wrong?
> > 
> > waffles:
> > 
> > I did some more investigation. The NaCl error only appears if I don't delete
> the
> > ~/.config/chromium directory between attempts. After deleting that, between
> > attempts the error is gone. Here is the behavior I see now on origin/master:
> > 
> > On first run:
> >  - If I didn't delete the out/Debug/pnacl directory, everything works.
> >  - If I deleted out/Debug/pnacl, I get: NativeClient: The Portable Native
> Client
> > (pnacl) component is not installed. Please consult chrome://components for
> more
> > information.
> >  - If I deleted out/Debug/pnacl, and then went to chrome://components and
> > manually trigger an on-demand update for pnacl, it works.
> >  - Under no circumstances is an OnDemandResourceThrottle requested.
> 
> Hm. This also appears to be the case with a straight LKGR build. I guess the
> feature is broken.

waffles: Cool, thanks for verifying.

For reference, the error code comes from here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/re...

I tested this with the version that precedes
https://codereview.chromium.org/420503002/ also. 

My gut instinct is that the on-demand update functionality was not broken by CUS
changes, esp. since the manual update works fine. Thanks!

[Sorin Jianu, 2014-08-05 23:53:43]

I've run this under a debugger on Windows, set breakpoints in the relevant parts
of the code:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...

The following if condition was never true for the tests I've done:
(resource_type == content::RESOURCE_TYPE_OBJECT) { 

Therefore, the update service on-demand code never triggered, in other wordsm
this call never happened for me:
cus->GetOnDemandUpdater().GetOnDemandResourceThrottle.

[tommycli, 2014-08-06 17:09:27]

On 2014/08/05 23:53:43, Sorin Jianu wrote:
> I've run this under a debugger on Windows, set breakpoints in the relevant
parts
> of the code:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...
> 
> The following if condition was never true for the tests I've done:
> (resource_type == content::RESOURCE_TYPE_OBJECT) { 
> 
> Therefore, the update service on-demand code never triggered, in other wordsm
> this call never happened for me:
> cus->GetOnDemandUpdater().GetOnDemandResourceThrottle.

I did the same test under Linux, and also never got a
content::RESOURCE_TYPE_OBJECT.

[tommycli, 2014-08-07 20:35:35]

On 2014/08/06 17:09:27, tommycli wrote:
> On 2014/08/05 23:53:43, Sorin Jianu wrote:
> > I've run this under a debugger on Windows, set breakpoints in the relevant
> parts
> > of the code:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...
> > 
> > The following if condition was never true for the tests I've done:
> > (resource_type == content::RESOURCE_TYPE_OBJECT) { 
> > 
> > Therefore, the update service on-demand code never triggered, in other
wordsm
> > this call never happened for me:
> > cus->GetOnDemandUpdater().GetOnDemandResourceThrottle.
> 
> I did the same test under Linux, and also never got a
> content::RESOURCE_TYPE_OBJECT.

waffles/sorin: What's our strategy to move this forward given that this code
path doesn't seem to be actually called?

This is the last major blocker to moving a ton of files over to components/.

[Sorin Jianu, 2014-08-07 21:00:58]

On 2014/08/07 20:35:35, tommycli wrote:
> On 2014/08/06 17:09:27, tommycli wrote:
> > On 2014/08/05 23:53:43, Sorin Jianu wrote:
> > > I've run this under a debugger on Windows, set breakpoints in the relevant
> > parts
> > > of the code:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...
> > > 
> > > The following if condition was never true for the tests I've done:
> > > (resource_type == content::RESOURCE_TYPE_OBJECT) { 
> > > 
> > > Therefore, the update service on-demand code never triggered, in other
> wordsm
> > > this call never happened for me:
> > > cus->GetOnDemandUpdater().GetOnDemandResourceThrottle.
> > 
> > I did the same test under Linux, and also never got a
> > content::RESOURCE_TYPE_OBJECT.
> 
> waffles/sorin: What's our strategy to move this forward given that this code
> path doesn't seem to be actually called?
> 
> This is the last major blocker to moving a ton of files over to components/.

Tommy, I understand this issue is not a regression introduced by your
refactoring along the way. If this is true, then I suggest continue with the
refactoring work and moving of files around.

On demand for pNaCL must be fixed no matter where the files reside, by the
owners of the component updater and pNaCl code. Is this a correct assumption so
far?

[waffles, 2014-08-07 21:15:39]

On 2014/08/07 20:35:35, tommycli wrote:
> On 2014/08/06 17:09:27, tommycli wrote:
> > On 2014/08/05 23:53:43, Sorin Jianu wrote:
> > > I've run this under a debugger on Windows, set breakpoints in the relevant
> > parts
> > > of the code:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...
> > > 
> > > The following if condition was never true for the tests I've done:
> > > (resource_type == content::RESOURCE_TYPE_OBJECT) { 
> > > 
> > > Therefore, the update service on-demand code never triggered, in other
> wordsm
> > > this call never happened for me:
> > > cus->GetOnDemandUpdater().GetOnDemandResourceThrottle.
> > 
> > I did the same test under Linux, and also never got a
> > content::RESOURCE_TYPE_OBJECT.
> 
> waffles/sorin: What's our strategy to move this forward given that this code
> path doesn't seem to be actually called?
> 
> This is the last major blocker to moving a ton of files over to components/.

It's definitely a bug that this code path isn't called - so in some respect I
kind of think of landing this like landing while the tree is red, since we don't
know if it breaks pnacl on-demand updates. That being said, I think fixing PNaCl
on-demand updates is probably a ways down the road, so I guess let's just keep
in mind that we couldn't get the manual test in this CL, and go ahead and move
forward.

lgtm

[tommycli, 2014-08-07 21:31:42]

thestig: Need OWNER stamp for
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

Thanks

[tommycli, 2014-08-07 21:32:09]

thestig: Need OWNER stamp for
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

Thanks

[Lei Zhang, 2014-08-07 21:33:42]

lgtm

[tommycli, 2014-08-07 21:37:12]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-08-07 22:21:20]

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

[commit-bot: I haz the power, 2014-08-08 02:14:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-08 02:37:06]

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

[commit-bot: I haz the power, 2014-08-08 02:37:07]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[tommycli, 2014-08-08 15:50:47]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-08-08 15:51:52]

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

[commit-bot: I haz the power, 2014-08-08 15:53:26]

Change committed as 288367