Split content script injections into multiple tasks

Split content script injections into multiple tasks

Currently, content scripts for all extensions are injected in single
task, causing a big jank on main thread. This patch changes
document_end and document_idle content scripts to run in separate tasks
asynchronously. The window.onload event is blocked until all
document_end scripts are injected.

This new behavior is protected by a feature switch, disabled by default.

To enable asynchronous execution, this CL adds a option parameter to
WebLocalFrame::requestExecuteScriptInIsolatedWorld(). Inside Blink,
it uses SuspendableScriptExecutor's ability to delay execution. So
async script executions are posted as timer tasks.

To test the new behavior, browser tests in content_script_apitest.cc
are parameterized so they run with and without the flag enabled.
Also, some extensions for tests are switched from document_idle to
document_end, since after this patch there's no reliable way to make
sure that all document_idle scripts have finished.

BUG=636655
Review-Url: https://codereview.chromium.org/2633253002
Cr-Commit-Position: refs/heads/master@{#457702}
Committed: https://chromium.googlesource.com/chromium/src/+/cbf167aaccb8c6f17c305dffa76b6523776222bf

[Kunihiko Sakamoto, 2017-01-17 04:03:10]

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

[commit-bot: I haz the power, 2017-01-17 04:03:27]

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-01-17 04:58:54]

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

[commit-bot: I haz the power, 2017-01-17 04:58:54]

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

[Kunihiko Sakamoto, 2017-01-17 05:35:14]

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

[commit-bot: I haz the power, 2017-01-17 05:36:13]

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-01-17 06:30:55]

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

[commit-bot: I haz the power, 2017-01-17 06:30:55]

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

[Kunihiko Sakamoto, 2017-01-17 09:11:16]

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

[commit-bot: I haz the power, 2017-01-17 09:11:23]

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-01-17 10:05:14]

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

[commit-bot: I haz the power, 2017-01-17 10:05:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kunihiko Sakamoto, 2017-01-18 05:03:53]

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

[commit-bot: I haz the power, 2017-01-18 05:04:10]

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-01-18 05:57:53]

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

[commit-bot: I haz the power, 2017-01-18 05:57:53]

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_...)

[Kunihiko Sakamoto, 2017-01-19 04:52:11]

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

[commit-bot: I haz the power, 2017-01-19 04:52:25]

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-01-19 06:53:47]

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

[commit-bot: I haz the power, 2017-01-19 06:53:49]

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

[Kunihiko Sakamoto, 2017-01-19 07:27:07]

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

[commit-bot: I haz the power, 2017-01-19 07:27:22]

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-01-19 09:18:51]

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

[commit-bot: I haz the power, 2017-01-19 09:18:52]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-01-20 04:31:06]

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

[commit-bot: I haz the power, 2017-01-20 04:31:15]

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-01-20 06:27:50]

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

[commit-bot: I haz the power, 2017-01-20 06:27:51]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-01-23 04:49:49]

Description was changed from

==========
WIP
==========

to

==========
[WIP] Split script injections for document_{end,idle} into multiple tasks

BUG=636655
==========

[Kunihiko Sakamoto, 2017-01-27 04:43:59]

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

[commit-bot: I haz the power, 2017-01-27 04:44:30]

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-01-27 06:28:17]

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

[commit-bot: I haz the power, 2017-01-27 06:28:18]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-01-27 07:08:26]

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

[commit-bot: I haz the power, 2017-01-27 07:08:44]

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-01-27 09:01:32]

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

[commit-bot: I haz the power, 2017-01-27 09:01:33]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-02-09 05:03:49]

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

[commit-bot: I haz the power, 2017-02-09 05:04:27]

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-02-09 06:47:11]

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

[commit-bot: I haz the power, 2017-02-09 06:47:12]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-02-10 09:24:26]

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

[commit-bot: I haz the power, 2017-02-10 09:24:41]

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-02-10 11:08:44]

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

[commit-bot: I haz the power, 2017-02-10 11:08:45]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-02-13 05:10:30]

Description was changed from

==========
[WIP] Split script injections for document_{end,idle} into multiple tasks

BUG=636655
==========

to

==========
Split content script injections into multiple tasks

Currently, content scripts for all extensions are injected in single
task, causing a big jank on main thread. This patch changes
document_end and document_idle content scripts to run in separate tasks
asynchronously. The window.onload event is blocked until all
document_end scripts are injected.

This new behavior is protected by a feature switch, disabled by default.

To enable asynchronous execution, this CL adds a option parameter to
WebLocalFrame::requestExecuteScriptInIsolatedWorld(). Inside Blink,
it uses SuspendableScriptExecutor's ability to delay execution. So
async script executions are posted as timer tasks.

To test the new behavior, browser tests in content_script_apitest.cc
are parameterized so they run with and without the flag enabled.
Also, some extensions for tests are switched from document_idle to
document_end, since there's no reliable way to make sure that all
document_idle scripts have finished.

BUG=636655
==========

[Kunihiko Sakamoto, 2017-02-13 05:12:06]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:12:18]

Patchset #1 (id:20001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:12:29]

Patchset #1 (id:40001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:12:41]

Patchset #1 (id:60001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:12:55]

Patchset #1 (id:80001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:13:25]

Patchset #1 (id:100001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:15:27]

Patchset #1 (id:120001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:15:39]

Patchset #1 (id:140001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:15:47]

Patchset #1 (id:160001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:15:58]

Patchset #1 (id:180001) has been deleted

[Kunihiko Sakamoto, 2017-02-13 05:29:37]

Description was changed from

==========
Split content script injections into multiple tasks

Currently, content scripts for all extensions are injected in single
task, causing a big jank on main thread. This patch changes
document_end and document_idle content scripts to run in separate tasks
asynchronously. The window.onload event is blocked until all
document_end scripts are injected.

This new behavior is protected by a feature switch, disabled by default.

To enable asynchronous execution, this CL adds a option parameter to
WebLocalFrame::requestExecuteScriptInIsolatedWorld(). Inside Blink,
it uses SuspendableScriptExecutor's ability to delay execution. So
async script executions are posted as timer tasks.

To test the new behavior, browser tests in content_script_apitest.cc
are parameterized so they run with and without the flag enabled.
Also, some extensions for tests are switched from document_idle to
document_end, since there's no reliable way to make sure that all
document_idle scripts have finished.

BUG=636655
==========

to

==========
Split content script injections into multiple tasks

Currently, content scripts for all extensions are injected in single
task, causing a big jank on main thread. This patch changes
document_end and document_idle content scripts to run in separate tasks
asynchronously. The window.onload event is blocked until all
document_end scripts are injected.

This new behavior is protected by a feature switch, disabled by default.

To enable asynchronous execution, this CL adds a option parameter to
WebLocalFrame::requestExecuteScriptInIsolatedWorld(). Inside Blink,
it uses SuspendableScriptExecutor's ability to delay execution. So
async script executions are posted as timer tasks.

To test the new behavior, browser tests in content_script_apitest.cc
are parameterized so they run with and without the flag enabled.
Also, some extensions for tests are switched from document_idle to
document_end, since after this patch there's no reliable way to make
sure that all document_idle scripts have finished.

BUG=636655
==========

[Kunihiko Sakamoto, 2017-02-13 05:37:29]

ksakamoto@chromium.org changed reviewers:
+ haraken@chromium.org, rdevlin.cronin@chromium.org

[Kunihiko Sakamoto, 2017-02-13 05:37:30]

devlin for extensions
haraken for third_party/WebKit

[Devlin, 2017-02-13 23:13:10]

One problem:
We publicly document [1] that document_idle scripts will run no later than
"immediately after window.onload fires". I don't see any guarantees with
WebLocalFrame::Asynchronous that this will happen, and the changes in the
extension tests imply that this guarantee has been broken.

https://codereview.chromium.org/2633253002/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/content_script_apitest.cc (right):

https://codereview.chromium.org/2633253002/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/content_script_apitest.cc:248: "1");
Defaults change, so let's prefer something like:
command_line->AppendSwitchASCII(
    switches::kYieldBetweenContentScriptRuns,
    GetParam() == TestConfig::kYieldBetweenContentScriptRunsEnabled ?
        "1" : 0);

https://codereview.chromium.org/2633253002/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/content_script_apitest.cc:252:
std::unique_ptr<FeatureSwitch::ScopedOverride> override_;
used?

https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(left):

https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:48:
if (!hasReceivedFirstMessage) {
Why this change?

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (left):

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:285:
UMA_HISTOGRAM_TIMES("Extensions.InjectScriptTime", exec_timer.Elapsed());
This UMA wasn't really accurate before, but we should probably record new time
under a new histogram.

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:98: void willExecute() override {
begin_execution_ = base::TimeTicks::Now(); }
chromium-style: WillExecute

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:102: base::TimeTicks begin_execution_;
nit: maybe start_time_ or execution_start_time_?

[haraken, 2017-02-14 00:16:30]

> One problem:
> We publicly document [1] that document_idle scripts will run no later than
> "immediately after window.onload fires".

Yeah. Would it be a terrible idea to split document_end and document_idle
scripts into multiple tasks and block window.onload until all the document_end
*and* document_idle scripts have run? One concern is that it might delay
window.onload too much.

[dcheng, 2017-02-14 03:00:23]

On 2017/02/14 00:16:30, haraken wrote:
> > One problem:
> > We publicly document [1] that document_idle scripts will run no later than
> > "immediately after window.onload fires".
> 

I'm wondering... is it actually important for document_idle scripts to run
synchronous after window.onload (this is what the quoted documentation indicates
to me)? Is the difference actually observable to content scripts? We want to
make window.onload fire asynchronously anyway at some point...

> Yeah. Would it be a terrible idea to split document_end and document_idle
> scripts into multiple tasks and block window.onload until all the document_end
> *and* document_idle scripts have run? One concern is that it might delay
> window.onload too much.

I think only document_end would block onload, right? As long as onload is also
fire asynchronously, I think this is OK (but I think this is not true yet?)

[haraken, 2017-02-14 03:43:18]

On 2017/02/14 03:00:23, dcheng wrote:
> On 2017/02/14 00:16:30, haraken wrote:
> > > One problem:
> > > We publicly document [1] that document_idle scripts will run no later than
> > > "immediately after window.onload fires".
> > 
> 
> I'm wondering... is it actually important for document_idle scripts to run
> synchronous after window.onload (this is what the quoted documentation
indicates
> to me)? Is the difference actually observable to content scripts? We want to
> make window.onload fire asynchronously anyway at some point...
> 
> > Yeah. Would it be a terrible idea to split document_end and document_idle
> > scripts into multiple tasks and block window.onload until all the
document_end
> > *and* document_idle scripts have run? One concern is that it might delay
> > window.onload too much.
> 
> I think only document_end would block onload, right? As long as onload is also
> fire asynchronously, I think this is OK (but I think this is not true yet?)

If we really need to follow the spec about document_idle (i.e., they need to run
no later than immediately after window.onload fires) and we want to split
document_idle scripts into multiple chunks, I thought that one solution would be
to block window.onload until all of the document_idle scripts have run.

[dcheng, 2017-02-14 03:55:00]

On 2017/02/14 03:43:18, haraken wrote:
> On 2017/02/14 03:00:23, dcheng wrote:
> > On 2017/02/14 00:16:30, haraken wrote:
> > > > One problem:
> > > > We publicly document [1] that document_idle scripts will run no later
than
> > > > "immediately after window.onload fires".
> > > 
> > 
> > I'm wondering... is it actually important for document_idle scripts to run
> > synchronous after window.onload (this is what the quoted documentation
> indicates
> > to me)? Is the difference actually observable to content scripts? We want to
> > make window.onload fire asynchronously anyway at some point...
> > 
> > > Yeah. Would it be a terrible idea to split document_end and document_idle
> > > scripts into multiple tasks and block window.onload until all the
> document_end
> > > *and* document_idle scripts have run? One concern is that it might delay
> > > window.onload too much.
> > 
> > I think only document_end would block onload, right? As long as onload is
also
> > fire asynchronously, I think this is OK (but I think this is not true yet?)
> 
> If we really need to follow the spec about document_idle (i.e., they need to
run
> no later than immediately after window.onload fires) and we want to split
> document_idle scripts into multiple chunks, I thought that one solution would
be
> to block window.onload until all of the document_idle scripts have run.

I see. I feel like there's a higher chance of tasks breaking if we reorder
document_idle tasks before onload (for example, maybe it depends on some DOM
mutations in onload), but I don't have any numbers to back up this assertion. I
wonder if there's an easy way to experiment...

[haraken, 2017-02-14 03:56:40]

On 2017/02/14 03:55:00, dcheng wrote:
> On 2017/02/14 03:43:18, haraken wrote:
> > On 2017/02/14 03:00:23, dcheng wrote:
> > > On 2017/02/14 00:16:30, haraken wrote:
> > > > > One problem:
> > > > > We publicly document [1] that document_idle scripts will run no later
> than
> > > > > "immediately after window.onload fires".
> > > > 
> > > 
> > > I'm wondering... is it actually important for document_idle scripts to run
> > > synchronous after window.onload (this is what the quoted documentation
> > indicates
> > > to me)? Is the difference actually observable to content scripts? We want
to
> > > make window.onload fire asynchronously anyway at some point...
> > > 
> > > > Yeah. Would it be a terrible idea to split document_end and
document_idle
> > > > scripts into multiple tasks and block window.onload until all the
> > document_end
> > > > *and* document_idle scripts have run? One concern is that it might delay
> > > > window.onload too much.
> > > 
> > > I think only document_end would block onload, right? As long as onload is
> also
> > > fire asynchronously, I think this is OK (but I think this is not true
yet?)
> > 
> > If we really need to follow the spec about document_idle (i.e., they need to
> run
> > no later than immediately after window.onload fires) and we want to split
> > document_idle scripts into multiple chunks, I thought that one solution
would
> be
> > to block window.onload until all of the document_idle scripts have run.
> 
> I see. I feel like there's a higher chance of tasks breaking if we reorder
> document_idle tasks before onload (for example, maybe it depends on some DOM
> mutations in onload), but I don't have any numbers to back up this assertion.
I
> wonder if there's an easy way to experiment...

Yeah. Per the spec, it's fine to schedule document_idle scripts before
window.onload.

However, ideally I want to remove the "document_idle scripts must run no later
than immediately after window.onload" part from the spec... :)

[Kunihiko Sakamoto, 2017-02-14 08:42:00]

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

[commit-bot: I haz the power, 2017-02-14 08:42:29]

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

[Kunihiko Sakamoto, 2017-02-14 09:07:54]

Let me check if my understanding is correct...

> We publicly document [1] that document_idle scripts will run no later than
> "immediately after window.onload fires".

Does this mean that

window.onload = function() {
  // document_idle scripts may not be injected here, but
  setTimeout(function() {
    // document_idle scripts must have been injected by here
  }, 0);
}

correct? If so, yeah current patch breaks that, and making document_idle
onload-blocking is the only possible solution I can think of.

https://codereview.chromium.org/2633253002/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/content_script_apitest.cc (right):

https://codereview.chromium.org/2633253002/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/content_script_apitest.cc:248: "1");
On 2017/02/13 23:13:09, Devlin wrote:
> Defaults change, so let's prefer something like:
> command_line->AppendSwitchASCII(
>     switches::kYieldBetweenContentScriptRuns,
>     GetParam() == TestConfig::kYieldBetweenContentScriptRunsEnabled ?
>         "1" : 0);

Done.

https://codereview.chromium.org/2633253002/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/content_script_apitest.cc:252:
std::unique_ptr<FeatureSwitch::ScopedOverride> override_;
On 2017/02/13 23:13:09, Devlin wrote:
> used?

Removed.

https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(left):

https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:48:
if (!hasReceivedFirstMessage) {
On 2017/02/13 23:13:09, Devlin wrote:
> Why this change?

For an iframe with src= javascript: URL, blink opens a frame with about:blank
URL and then execute the JS and replaces the document with the execution result.
 Currently, document_end scripts are injected twice in this process, once for
about:blank and once for the replaced document.

My change delays document_end injections. As a result, the replacement happens
first, so document_end scripts runs only on replaced document.

This is a very subtle corner case and I think this isn't violating the contract
of extensions.

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (left):

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:285:
UMA_HISTOGRAM_TIMES("Extensions.InjectScriptTime", exec_timer.Elapsed());
On 2017/02/13 23:13:10, Devlin wrote:
> This UMA wasn't really accurate before, but we should probably record new time
> under a new histogram.

Done. Should we keep current one?

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:98: void willExecute() override {
begin_execution_ = base::TimeTicks::Now(); }
On 2017/02/13 23:13:09, Devlin wrote:
> chromium-style: WillExecute

This is blink-style because this overrides a function defined in
blink::WebScriptExecutionCallback.

https://codereview.chromium.org/2633253002/diff/200001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:102: base::TimeTicks begin_execution_;
On 2017/02/13 23:13:10, Devlin wrote:
> nit: maybe start_time_ or execution_start_time_?

Done.

[Devlin, 2017-02-14 16:03:51]

On 2017/02/14 09:07:54, Kunihiko Sakamoto wrote:
> Let me check if my understanding is correct...
> 
> > We publicly document [1] that document_idle scripts will run no later than
> > "immediately after window.onload fires".
> 
> Does this mean that
> 
> window.onload = function() {
>   // document_idle scripts may not be injected here, but
>   setTimeout(function() {
>     // document_idle scripts must have been injected by here
>   }, 0);
> }
> 
> correct? If so, yeah current patch breaks that, and making document_idle
> onload-blocking is the only possible solution I can think of.

Yes, that is what the spec currently is.

dcheng:
> I'm wondering... is it actually important for document_idle scripts to run
> synchronous after window.onload (this is what the quoted documentation
indicates
> to me)? Is the difference actually observable to content scripts? We want to
> make window.onload fire asynchronously anyway at some point...
I think the biggest thing is that it's a "guaranteed" time to run, which gives
extensions some confidence that the script will be executed within a reasonable
amount of time.  We try to encourage extensions to prefer document_idle when
they have scripts that need to execute, but don't need to e.g. intercept dom
creation events, but if we just say "asynchronous at some point", this becomes a
lot harder to accept as a developer.

Basically, we need a way to (try to) ensure that document_idle scripts don't
execute "too late", with some reasonable deterministic quality of what "too
late" is.  Currently, that's window.onload.  I'm open to other suggestions, but
the general implication should remain the same, and there would need to be a
compelling story to change the spec.

> I see. I feel like there's a higher chance of tasks breaking if we reorder
> document_idle tasks before onload (for example, maybe it depends on some DOM
> mutations in onload), but I don't have any numbers to back up this assertion.
I
> wonder if there's an easy way to experiment...
We specify that document_idle scripts *may* inject before or after onload, but
no later than immediately after.

[Kunihiko Sakamoto, 2017-02-16 05:03:07]

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

[commit-bot: I haz the power, 2017-02-16 05:03:50]

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-02-16 06:13:59]

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

[commit-bot: I haz the power, 2017-02-16 06:14:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Kunihiko Sakamoto, 2017-02-16 07:33:04]

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

[commit-bot: I haz the power, 2017-02-16 07:34:06]

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

[Kunihiko Sakamoto, 2017-02-16 08:56:06]

Patchset #4 (id:260001) has been deleted

[Kunihiko Sakamoto, 2017-02-16 08:58:52]

PTAL

Now document_idle script injections block window.onload.
The only affected test was WebViewTests/WebViewTest.WhitelistedContentScript,
which assumed onload comes before injection, so I've changed it to use
DOMContentLoaded instead.

[commit-bot: I haz the power, 2017-02-16 09:16:34]

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

[commit-bot: I haz the power, 2017-02-16 09:16:35]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-02-21 02:08:03]

ksakamoto@chromium.org changed reviewers:
+ kouhei@chromium.org

[Devlin, 2017-02-21 18:56:27]

https://codereview.chromium.org/2633253002/diff/280001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2633253002/diff/280001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1406: // |render_frame| might
be dead by now.
Duplicate this comment in the new function.

https://codereview.chromium.org/2633253002/diff/280001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2633253002/diff/280001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3870:
GetContentClient()->renderer()->RunScriptsAtDocumentIdle(this);
Probably worth adding a comment that |frame| and |this| may be deleted by now,
as we do on line 3831

https://codereview.chromium.org/2633253002/diff/280001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/280001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:305: option =
blink::WebLocalFrame::AsyncBlockingOnload;
What all can happen during the yield we're introducing?  We document that
document_end scripts execute before any subresources are instantiated; is that
guarantee preserved here?

https://codereview.chromium.org/2633253002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2633253002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:724: if
(!shouldComplete(m_frame->document()))
m_frame may be deleted at this point.

[Kunihiko Sakamoto, 2017-02-23 09:49:01]

https://codereview.chromium.org/2633253002/diff/280001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2633253002/diff/280001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1406: // |render_frame| might
be dead by now.
On 2017/02/21 18:56:26, Devlin wrote:
> Duplicate this comment in the new function.

Done.

https://codereview.chromium.org/2633253002/diff/280001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2633253002/diff/280001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3870:
GetContentClient()->renderer()->RunScriptsAtDocumentIdle(this);
On 2017/02/21 18:56:26, Devlin wrote:
> Probably worth adding a comment that |frame| and |this| may be deleted by now,
> as we do on line 3831

Done.

https://codereview.chromium.org/2633253002/diff/280001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/280001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:305: option =
blink::WebLocalFrame::AsyncBlockingOnload;
On 2017/02/21 18:56:26, Devlin wrote:
> What all can happen during the yield we're introducing?  We document that
> document_end scripts execute before any subresources are instantiated; is that
> guarantee preserved here?

Anything but window.onload event can happen.

I think that guarantee has already been broken... If I open the DevTools'
network tab and load a large document (ex.
https://www.w3.org/TR/html5/single-page.html), I can see subresources starts
loading while the main HTML is still loading.

https://codereview.chromium.org/2633253002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2633253002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:724: if
(!shouldComplete(m_frame->document()))
On 2017/02/21 18:56:26, Devlin wrote:
> m_frame may be deleted at this point.

Check added.

[Kunihiko Sakamoto, 2017-02-27 01:45:24]

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

[commit-bot: I haz the power, 2017-02-27 01:45:31]

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-02-27 03:28:45]

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

[commit-bot: I haz the power, 2017-02-27 03:28:47]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-03-01 07:26:17]

Devlin - friendly ping :)

[Devlin, 2017-03-02 02:33:17]

Sorry for the delay!

A few small nits, but otherwise I think this looks pretty good for some
experimentation. :)

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:47:
onRequest.removeListener(listener);
why this change?

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_whitelisted/main.js
(right):

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/content_script_whitelisted/main.js:6:
document.addEventListener("DOMContentLoaded", function() {
nit: prefer single quotes
but also, why this change?

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:329:
UMA_HISTOGRAM_TIMES("Extensions.InjectedScriptExecutionTime", elapsed);
Did you forget to add this metric to histograms.xml?

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
extensions/renderer/script_injection_manager.cc:194: if
(FeatureSwitch::yield_between_content_script_runs()->IsEnabled()) {
IsEnabled() is non-trivial - let's cache the result somewhere

[haraken, 2017-03-02 03:54:33]

WebKit/ mostly looks good.

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:725: if
(!m_frame->client())

m_frame->client() => client()

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:206: void
SuspendableScriptExecutor::runAsync(bool blockOnload) {

Use an enum instead of bool.

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:400: // The frame's window.onload
event is about to fire. This method may delay

Shall we drop the part of "The frame's window.onload event is about to fire"?
runScriptsAtDocumentIdle may run enough before window.onload, right?

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:253: // Execute script
synchronously if possible.

What do you mean by "if possible"?

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:258: AsyncBlockingOnload

AsynchronousBlockingOnload

[Kunihiko Sakamoto, 2017-03-06 08:56:43]

Patchset #7 (id:340001) has been deleted

[Kunihiko Sakamoto, 2017-03-06 09:11:06]

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:47:
onRequest.removeListener(listener);
On 2017/03/02 02:33:17, Devlin wrote:
> why this change?

It has been answered here :)
https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_whitelisted/main.js
(right):

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/content_script_whitelisted/main.js:6:
document.addEventListener("DOMContentLoaded", function() {
On 2017/03/02 02:33:17, Devlin wrote:
> nit: prefer single quotes
> but also, why this change?

This test failed when content script is injected before onload.

Changing to DCL wasn't needed, just needed to fix the magic string for
'the-bridge-element' to match the string in
chrome/test/data/extensions/platform_apps/web_view/extension_api/content_script/content_script.js.

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:329:
UMA_HISTOGRAM_TIMES("Extensions.InjectedScriptExecutionTime", elapsed);
On 2017/03/02 02:33:17, Devlin wrote:
> Did you forget to add this metric to histograms.xml?

Oops, added.

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/2633253002/diff/320001/extensions/renderer/sc...
extensions/renderer/script_injection_manager.cc:194: if
(FeatureSwitch::yield_between_content_script_runs()->IsEnabled()) {
On 2017/03/02 02:33:17, Devlin wrote:
> IsEnabled() is non-trivial - let's cache the result somewhere

Added cache to FeatureSwitch class.
(ScriptInjection::InjectJs() uses this flag too, so we'd have to pass around the
value otherwise.)

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:725: if
(!m_frame->client())
On 2017/03/02 03:54:32, haraken wrote:
> 
> m_frame->client() => client()

Done.

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:206: void
SuspendableScriptExecutor::runAsync(bool blockOnload) {
On 2017/03/02 03:54:32, haraken wrote:
> 
> Use an enum instead of bool.

Done.

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:400: // The frame's window.onload
event is about to fire. This method may delay
On 2017/03/02 03:54:32, haraken wrote:
> 
> Shall we drop the part of "The frame's window.onload event is about to fire"?
> runScriptsAtDocumentIdle may run enough before window.onload, right?
> 

document_idle scripts may run enough before window.ounload, but not through this
function. Injection is triggered by runScriptsAtDocumentIdle, or a delayed task
posted by ScriptInjectionManager::RFOHelper::DidFinishDocumentLoad(). This
function is called only just before onload.

So this is a bit different from runScriptsAtDocument{Start,End}. Should we
emphasize that by renaming this, e.g. runScriptsAtDocumentIdleIfNeeded?

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:253: // Execute script
synchronously if possible.
On 2017/03/02 03:54:33, haraken wrote:
> 
> What do you mean by "if possible"?
> 

Since requestExecute* functions use SuspendableScriptExecutor internally,
scripts can be suspended and executed asynchronously.

Changed this comment to be a little more specific.

https://codereview.chromium.org/2633253002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:258: AsyncBlockingOnload
On 2017/03/02 03:54:32, haraken wrote:
> 
> AsynchronousBlockingOnload

Done.

[Devlin, 2017-03-10 03:18:45]

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:47:
onRequest.removeListener(listener);
On 2017/03/06 09:11:05, Kunihiko Sakamoto wrote:
> On 2017/03/02 02:33:17, Devlin wrote:
> > why this change?
> 
> It has been answered here :)
>
https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...

Whoops!  Sorry for the dupe. :)

This still makes me a little worried, though - I thought the intention was that
this CL should only have an impact when the associated switch is included.  Why
is this delaying document_end scripts in all cases?

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
extensions/common/feature_switch.cc:62: kYieldBetweenContentScriptRuns,
We should rename this to be kYieldBetweenContentScriptRunsFieldTrial or similar
to differentiate.

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
extensions/common/feature_switch.h:105: mutable CachedValue cached_value_;
nit: maybe use a base::Optional<bool> here?

https://codereview.chromium.org/2633253002/diff/380001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2633253002/diff/380001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:18395: +    Time taken to execute
scripts injected by extensions. Unlike
Be more specific here - scripts injected at which time?  How many scripts?  All?
 All for one location?  Recorded how many times?  etc.

[Kunihiko Sakamoto, 2017-03-10 07:07:28]

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:47:
onRequest.removeListener(listener);
On 2017/03/10 03:18:44, Devlin wrote:
> On 2017/03/06 09:11:05, Kunihiko Sakamoto wrote:
> > On 2017/03/02 02:33:17, Devlin wrote:
> > > why this change?
> > 
> > It has been answered here :)
> >
>
https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...
> 
> Whoops!  Sorry for the dupe. :)
> 
> This still makes me a little worried, though - I thought the intention was
that
> this CL should only have an impact when the associated switch is included. 
Why
> is this delaying document_end scripts in all cases?

The behavior does not change unless the flag is enabled, but since
ContentScriptApiTest is now parameterized, I changed this test to be more
permissive, to pass in both cases.

In Patch Set 9, I've reverted that and made the test aware of the flag,
expecting current behavior when the flag is disabled.

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
extensions/common/feature_switch.cc:62: kYieldBetweenContentScriptRuns,
On 2017/03/10 03:18:45, Devlin wrote:
> We should rename this to be kYieldBetweenContentScriptRunsFieldTrial or
similar
> to differentiate.

Done.

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/2633253002/diff/380001/extensions/common/feat...
extensions/common/feature_switch.h:105: mutable CachedValue cached_value_;
On 2017/03/10 03:18:45, Devlin wrote:
> nit: maybe use a base::Optional<bool> here?

Done.

https://codereview.chromium.org/2633253002/diff/380001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2633253002/diff/380001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:18395: +    Time taken to execute
scripts injected by extensions. Unlike
On 2017/03/10 03:18:45, Devlin wrote:
> Be more specific here - scripts injected at which time?  How many scripts? 
All?
>  All for one location?  Recorded how many times?  etc.

Done.

[Devlin, 2017-03-14 15:08:55]

Nice, lgtm!

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/2633253002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:47:
onRequest.removeListener(listener);
On 2017/03/10 07:07:28, Kunihiko Sakamoto wrote:
> On 2017/03/10 03:18:44, Devlin wrote:
> > On 2017/03/06 09:11:05, Kunihiko Sakamoto wrote:
> > > On 2017/03/02 02:33:17, Devlin wrote:
> > > > why this change?
> > > 
> > > It has been answered here :)
> > >
> >
>
https://codereview.chromium.org/2633253002/diff/200001/chrome/test/data/exten...
> > 
> > Whoops!  Sorry for the dupe. :)
> > 
> > This still makes me a little worried, though - I thought the intention was
> that
> > this CL should only have an impact when the associated switch is included. 
> Why
> > is this delaying document_end scripts in all cases?
> 
> The behavior does not change unless the flag is enabled, but since
> ContentScriptApiTest is now parameterized, I changed this test to be more
> permissive, to pass in both cases.
> 
> In Patch Set 9, I've reverted that and made the test aware of the flag,
> expecting current behavior when the flag is disabled.

Thanks; Patch Set 9 makes a lot more sense to me.  Otherwise, I'd expect this to
fail without the switch, since the first message would still be 'jsresult/'
(and, patching in the CL, it seems it did).

[haraken, 2017-03-14 15:18:40]

LGTM

https://codereview.chromium.org/2633253002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/2633253002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:211:
m_blockingOnload = true;

Can we replace the boolean with the BlockingOption?

[Kunihiko Sakamoto, 2017-03-15 06:56:52]

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

[commit-bot: I haz the power, 2017-03-15 06:57:11]

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

[Kunihiko Sakamoto, 2017-03-15 07:53:10]

ksakamoto@chromium.org changed reviewers:
+ isherman@chromium.org, jochen@chromium.org, kinuko@chromium.org

[Kunihiko Sakamoto, 2017-03-15 07:53:12]

+jochen PTAL for chrome/renderer/chrome_content_renderer_client.{h,cc}
+kinuko PTAL for content/
+isherman PTAL for histograms.xml

Thanks!

https://codereview.chromium.org/2633253002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/2633253002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:211:
m_blockingOnload = true;
On 2017/03/14 15:18:40, haraken wrote:
> 
> Can we replace the boolean with the BlockingOption?

Done.

[Ilya Sherman, 2017-03-15 08:18:07]

Metrics LGTM, with a question:

https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:18417: +    Unlike
Extensions.InjectScriptTime, this includes execution time of
Is it perhaps worth deprecating Extensions.InjectScriptTime?

[commit-bot: I haz the power, 2017-03-15 09:28:03]

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

[commit-bot: I haz the power, 2017-03-15 09:28:04]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-15 12:13:29]

https://codereview.chromium.org/2633253002/diff/440001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2633253002/diff/440001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3916: return;
How could it be null right after this method is called?

https://codereview.chromium.org/2633253002/diff/440001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/440001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:84:
TimedScriptInjectionCallback(base::WeakPtr<ScriptInjection> injection)
nit: explicit

[jochen (gone - plz use gerrit), 2017-03-15 15:52:03]

chrome/renderer/chrome_content_renderer_client.{h,cc} lgtm

[Kunihiko Sakamoto, 2017-03-16 07:39:03]

https://codereview.chromium.org/2633253002/diff/440001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2633253002/diff/440001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3916: return;
On 2017/03/15 12:13:28, kinuko wrote:
> How could it be null right after this method is called?

Obviously it can't. Removed.

https://codereview.chromium.org/2633253002/diff/440001/extensions/renderer/sc...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2633253002/diff/440001/extensions/renderer/sc...
extensions/renderer/script_injection.cc:84:
TimedScriptInjectionCallback(base::WeakPtr<ScriptInjection> injection)
On 2017/03/15 12:13:28, kinuko wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:18417: +    Unlike
Extensions.InjectScriptTime, this includes execution time of
On 2017/03/15 08:18:07, Ilya Sherman wrote:
> Is it perhaps worth deprecating Extensions.InjectScriptTime?

Devlin, is it okay to deprecate Extensions.InjectScriptTime?

[kinuko, 2017-03-16 09:54:38]

content lgtm

[kinuko, 2017-03-16 09:54:41]

content lgtm

[Devlin, 2017-03-16 14:56:17]

https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:18417: +    Unlike
Extensions.InjectScriptTime, this includes execution time of
On 2017/03/16 07:39:02, Kunihiko Sakamoto wrote:
> On 2017/03/15 08:18:07, Ilya Sherman wrote:
> > Is it perhaps worth deprecating Extensions.InjectScriptTime?
> 
> Devlin, is it okay to deprecate Extensions.InjectScriptTime?

TL;DR: Yes.

Extensions.InjectScriptTime now measures the synchronous amount of time taken to
inject scripts, right?  In principle, that might still be useful information. 
In practice, it's probably too noisy without being broken down further (e.g.
into document_start et al) to really infer much from, because we don't know
whether the scripts will be executed sync, async, blocked, etc.  It might be
worth revisiting at a later date adding in a sync execution timer that has this
extra granularity, but my guess is that InjectScriptTime will probably just
confuse people.

[Kunihiko Sakamoto, 2017-03-17 05:01:51]

On 2017/03/16 14:56:17, Devlin wrote:
>
https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2633253002/diff/440001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:18417: +    Unlike
> Extensions.InjectScriptTime, this includes execution time of
> On 2017/03/16 07:39:02, Kunihiko Sakamoto wrote:
> > On 2017/03/15 08:18:07, Ilya Sherman wrote:
> > > Is it perhaps worth deprecating Extensions.InjectScriptTime?
> > 
> > Devlin, is it okay to deprecate Extensions.InjectScriptTime?
> 
> TL;DR: Yes.
> 
> Extensions.InjectScriptTime now measures the synchronous amount of time taken
to
> inject scripts, right?  In principle, that might still be useful information. 
> In practice, it's probably too noisy without being broken down further (e.g.
> into document_start et al) to really infer much from, because we don't know
> whether the scripts will be executed sync, async, blocked, etc.  It might be
> worth revisiting at a later date adding in a sync execution timer that has
this
> extra granularity, but my guess is that InjectScriptTime will probably just
> confuse people.

I agree.
Let me land this, and do the deprecation in a following patch.

[Kunihiko Sakamoto, 2017-03-17 05:02:14]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2017-03-17 05:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rdevlin.cronin@chromium.org, jochen@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2633253002/#ps460001
(title: "comments addressed")

[commit-bot: I haz the power, 2017-03-17 05:02:38]

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-03-17 06:45:54]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1489726934140650,
"parent_rev": "303f5b11ca5701fd3410ad0c559fb6a88f832c71", "commit_rev":
"cbf167aaccb8c6f17c305dffa76b6523776222bf"}

[commit-bot: I haz the power, 2017-03-17 06:46:56]

Description was changed from

==========
Split content script injections into multiple tasks

Currently, content scripts for all extensions are injected in single
task, causing a big jank on main thread. This patch changes
document_end and document_idle content scripts to run in separate tasks
asynchronously. The window.onload event is blocked until all
document_end scripts are injected.

This new behavior is protected by a feature switch, disabled by default.

To enable asynchronous execution, this CL adds a option parameter to
WebLocalFrame::requestExecuteScriptInIsolatedWorld(). Inside Blink,
it uses SuspendableScriptExecutor's ability to delay execution. So
async script executions are posted as timer tasks.

To test the new behavior, browser tests in content_script_apitest.cc
are parameterized so they run with and without the flag enabled.
Also, some extensions for tests are switched from document_idle to
document_end, since after this patch there's no reliable way to make
sure that all document_idle scripts have finished.

BUG=636655
==========

to

==========
Split content script injections into multiple tasks

Currently, content scripts for all extensions are injected in single
task, causing a big jank on main thread. This patch changes
document_end and document_idle content scripts to run in separate tasks
asynchronously. The window.onload event is blocked until all
document_end scripts are injected.

This new behavior is protected by a feature switch, disabled by default.

To enable asynchronous execution, this CL adds a option parameter to
WebLocalFrame::requestExecuteScriptInIsolatedWorld(). Inside Blink,
it uses SuspendableScriptExecutor's ability to delay execution. So
async script executions are posted as timer tasks.

To test the new behavior, browser tests in content_script_apitest.cc
are parameterized so they run with and without the flag enabled.
Also, some extensions for tests are switched from document_idle to
document_end, since after this patch there's no reliable way to make
sure that all document_idle scripts have finished.

BUG=636655

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

[commit-bot: I haz the power, 2017-03-17 06:46:59]

Committed patchset #12 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/cbf167aaccb8c6f17c305dffa76b...

[haraken, 2017-03-24 09:53:29]

Have we observed any improvement in UMA numbers?

[kouhei (in TOK), 2017-03-24 10:30:07]

On 2017/03/24 09:53:29, haraken wrote:
> Have we observed any improvement in UMA numbers?
We are trying to assess that via Finch and currently Finch launch process under
way

[Kunihiko Sakamoto, 2017-03-27 07:26:22]

On 2017/03/24 10:30:07, kouhei wrote:
> On 2017/03/24 09:53:29, haraken wrote:
> > Have we observed any improvement in UMA numbers?
> We are trying to assess that via Finch and currently Finch launch process
under
> way

Yes this is the launch bug: http://crbug.com/705400