[MD Settings] Remove jank for webui.

[MD Settings] Remove jank for webui.

WebUI URLRequest source data is (on desktop) backed by a memory-mapped
file. Copying that data in CompleteRead can trigger jankiness due to
potential file IO caused by the read.

This CL moves the memcpy on the file thread. It is also a re-land of
https://codereview.chromium.org/2158123003/ with minor changes to fix a
crash.

R=dbeam@chromium.org, mmenke@chromium.org, dproy@chromium.org
BUG=455423
Committed: https://crrev.com/d7997eadf6490da014985949715b2cb0d4f67518
Cr-Commit-Position: refs/heads/master@{#414269}

[groby-ooo-7-16, 2016-08-22 18:21:08]

PTAL - fixed the original crash, and made PostReadTask take a refcounted ptr so
ownership is clear.

[groby-ooo-7-16, 2016-08-22 18:21:13]

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

[mmenke, 2016-08-22 18:29:11]

On 2016/08/22 18:21:08, groby wrote:
> PTAL - fixed the original crash, and made PostReadTask take a refcounted ptr
so
> ownership is clear.

This LGTM, though I wonder if we need to beef up tests - I'm surprised that none
of the bots caught this.  Surely anything that tests a WebUI page would have run
into this case?

[commit-bot: I haz the power, 2016-08-22 18:33:54]

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

[Dan Beam, 2016-08-22 19:30:52]

lgtm

[dproy, 2016-08-22 19:57:43]

lgtm

[groby-ooo-7-16, 2016-08-22 20:15:16]

IIRC, the chrome://tracing peeps added an interactive UI test for their
particular problem. 

It's not all WebUI tests, since several parts need to come together
* The read request must come in before the backend has delivered data
* The read request must actually be for zero bytes.
* The URLRequestJob must be deleted as a result. (I'm not sure where that
happens, still digging)


I'd assume the zero-byte part is the crucial one. Maybe dproy@ can shed some
light?

[mmenke, 2016-08-22 20:19:12]

On 2016/08/22 20:15:16, groby wrote:
> IIRC, the chrome://tracing peeps added an interactive UI test for their
> particular problem. 
> 
> It's not all WebUI tests, since several parts need to come together
> * The read request must come in before the backend has delivered data
> * The read request must actually be for zero bytes.
> * The URLRequestJob must be deleted as a result. (I'm not sure where that
> happens, still digging)
> 
> 
> I'd assume the zero-byte part is the crucial one. Maybe dproy@ can shed some
> light?

There shouldn't be any requests for 0 bytes, they're blocked upstream, I
believe.  Instead, we're returning 0 bytes for a read request for a non-zero
number of bytes.  That only happens for the last read.  If the backend hasn't
delivered data yet, does that mean that this only happens for empty files (i.e.,
data is always delivered as one massive chunk)?

[commit-bot: I haz the power, 2016-08-22 20:31:21]

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

[commit-bot: I haz the power, 2016-08-22 20:31:22]

Dry run: This issue passed the CQ dry run.

[dproy, 2016-08-22 20:59:00]

On 2016/08/22 20:15:16, groby wrote:
> IIRC, the chrome://tracing peeps added an interactive UI test for their
> particular problem. 
> 
> It's not all WebUI tests, since several parts need to come together
> * The read request must come in before the backend has delivered data
> * The read request must actually be for zero bytes.
> * The URLRequestJob must be deleted as a result. (I'm not sure where that
> happens, still digging)
> 
> 
> I'd assume the zero-byte part is the crucial one. Maybe dproy@ can shed some
> light?

I'm not intimately familiar with the WebUI or the tracing backend code, nor do I
know what the existing test coverage looks like. But I can see from the tracing
code (mostly in this
file:https://cs.chromium.org/chromium/src/content/browser/tracing/tracing_ui.cc?dr&sq=package:chromium&rcl=1471879278&l=173)
that the problematic query (begin_recording) does indeed return zero bytes
(https://cs.chromium.org/chromium/src/content/browser/tracing/tracing_ui.cc?dr...).

I won't be surprised if the zero byte part is crucial for the missed test
coverage here.

[mmenke, 2016-08-22 21:03:04]

On 2016/08/22 20:59:00, dproy1 wrote:
> On 2016/08/22 20:15:16, groby wrote:
> > IIRC, the chrome://tracing peeps added an interactive UI test for their
> > particular problem. 
> > 
> > It's not all WebUI tests, since several parts need to come together
> > * The read request must come in before the backend has delivered data
> > * The read request must actually be for zero bytes.
> > * The URLRequestJob must be deleted as a result. (I'm not sure where that
> > happens, still digging)
> > 
> > 
> > I'd assume the zero-byte part is the crucial one. Maybe dproy@ can shed some
> > light?
> 
> I'm not intimately familiar with the WebUI or the tracing backend code, nor do
I
> know what the existing test coverage looks like. But I can see from the
tracing
> code (mostly in this
>
file:https://cs.chromium.org/chromium/src/content/browser/tracing/tracing_ui.cc?dr&sq=package:chromium&rcl=1471879278&l=173)
> that the problematic query (begin_recording) does indeed return zero bytes
>
(https://cs.chromium.org/chromium/src/content/browser/tracing/tracing_ui.cc?dr...).
> 
> I won't be surprised if the zero byte part is crucial for the missed test
> coverage here.

Can we add a test for that case to url_data_manager_backend_unittest.cc? 
Doesn't look like that file currently injects fixed responses for any chrome
URLs, unfortunately, but in this particular case, think we really do want a test
of some sort.

[groby-ooo-7-16, 2016-08-24 19:47:23]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2016-08-24 19:48:13]

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, 2016-08-24 19:52:03]

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

[commit-bot: I haz the power, 2016-08-24 19:52:07]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[groby-ooo-7-16, 2016-08-24 20:47:03]

The CQ bit was checked by groby@chromium.org

[groby-ooo-7-16, 2016-08-24 20:47:03]

The patchset sent to the CQ was uploaded after l-g-t-m from dproy@chromium.org,
dbeam@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2268653002/#ps40001
(title: "Rebase to HEAD")

[commit-bot: I haz the power, 2016-08-24 20:48:26]

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, 2016-08-24 20:52:44]

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

[commit-bot: I haz the power, 2016-08-24 20:52:45]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[groby-ooo-7-16, 2016-08-25 01:06:25]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2016-08-25 01:07:04]

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, 2016-08-25 02:28:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-25 02:30:37]

Description was changed from

==========
[MD Settings] Remove jank for webui.

WebUI URLRequest source data is (on desktop) backed by a memory-mapped
file. Copying that data in CompleteRead can trigger jankiness due to
potential file IO caused by the read.

This CL moves the memcpy on the file thread. It is also a re-land of
https://codereview.chromium.org/2158123003/ with minor changes to fix a
crash.

R=dbeam@chromium.org, mmenke@chromium.org, dproy@chromium.org
BUG=455423
==========

to

==========
[MD Settings] Remove jank for webui.

WebUI URLRequest source data is (on desktop) backed by a memory-mapped
file. Copying that data in CompleteRead can trigger jankiness due to
potential file IO caused by the read.

This CL moves the memcpy on the file thread. It is also a re-land of
https://codereview.chromium.org/2158123003/ with minor changes to fix a
crash.

R=dbeam@chromium.org, mmenke@chromium.org, dproy@chromium.org
BUG=455423

Committed: https://crrev.com/d7997eadf6490da014985949715b2cb0d4f67518
Cr-Commit-Position: refs/heads/master@{#414269}
==========

[commit-bot: I haz the power, 2016-08-25 02:30:38]

Patchset 3 (id:??) landed as
https://crrev.com/d7997eadf6490da014985949715b2cb0d4f67518
Cr-Commit-Position: refs/heads/master@{#414269}