Disable memory caching of XHRs

Disable memory caching of XHRs

XHRs get removed from memory cache anyway after load finishes (see
issue 399166).  This change can save significant memory during the
fetch for large resources.

BUG=659789
Committed: https://crrev.com/e3207a5d277b089f5ea868fa402b8f703bdc830c
Cr-Commit-Position: refs/heads/master@{#432502}

[halliwell, 2016-10-27 16:10:47]

Description was changed from

==========
Disable memory caching of XHRs

XHRs get removed from memory cache anyway after load finishes (see
issue 399166).  This change can save significant memory during the
fetch for large resources.

BUG=659789
==========

to

==========
Disable memory caching of XHRs

XHRs get removed from memory cache anyway after load finishes (see
issue 399166).  This change can save significant memory during the
fetch for large resources.

BUG=659789
==========

[halliwell, 2016-10-27 16:10:47]

halliwell@chromium.org changed reviewers:
+ yhirano@chromium.org

[halliwell, 2016-10-27 16:11:17]

PTAL.  I wasn't 100% clear from discussion on the bug whether you think this is
ok for all XHRs, or just for ArrayBuffer fetches?

[yhirano, 2016-10-28 01:52:40]

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

[commit-bot: I haz the power, 2016-10-28 01:52:55]

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, 2016-10-28 02:21:02]

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

[commit-bot: I haz the power, 2016-10-28 02:21:03]

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

[yhirano, 2016-10-28 03:50:46]

On 2016/10/27 16:11:17, halliwell wrote:
> PTAL.  I wasn't 100% clear from discussion on the bug whether you think this
is
> ok for all XHRs, or just for ArrayBuffer fetches?

Test are failing: Can you stop calling Resource::setResourceBuffer in
ResourceLoader::requestSynchronously when the option is DO_NOT_BUFFER_DATA?

[halliwell, 2016-10-28 03:53:59]

On 2016/10/28 03:50:46, yhirano wrote:
> On 2016/10/27 16:11:17, halliwell wrote:
> > PTAL.  I wasn't 100% clear from discussion on the bug whether you think this
> is
> > ok for all XHRs, or just for ArrayBuffer fetches?
> 
> Test are failing: Can you stop calling Resource::setResourceBuffer in
> ResourceLoader::requestSynchronously when the option is DO_NOT_BUFFER_DATA?

Yeah, I'm looking at the test failures right now.  Thanks for the suggestion,
will try :)

[halliwell, 2016-10-28 04:33:30]

On 2016/10/28 03:53:59, halliwell wrote:
> On 2016/10/28 03:50:46, yhirano wrote:
> > On 2016/10/27 16:11:17, halliwell wrote:
> > > PTAL.  I wasn't 100% clear from discussion on the bug whether you think
this
> > is
> > > ok for all XHRs, or just for ArrayBuffer fetches?
> > 
> > Test are failing: Can you stop calling Resource::setResourceBuffer in
> > ResourceLoader::requestSynchronously when the option is DO_NOT_BUFFER_DATA?
> 
> Yeah, I'm looking at the test failures right now.  Thanks for the suggestion,
> will try :)

Ok, it fixes content_browsertests in local run.  I'll use CQ dry run again to
check a wider range of tests.

[halliwell, 2016-10-28 04:33:38]

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

[commit-bot: I haz the power, 2016-10-28 04:33:54]

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, 2016-10-28 05:14:10]

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

[commit-bot: I haz the power, 2016-10-28 05:14:11]

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

[halliwell, 2016-10-28 20:01:02]

On 2016/10/28 04:33:30, halliwell wrote:
> On 2016/10/28 03:53:59, halliwell wrote:
> > On 2016/10/28 03:50:46, yhirano wrote:
> > > On 2016/10/27 16:11:17, halliwell wrote:
> > > > PTAL.  I wasn't 100% clear from discussion on the bug whether you think
> this
> > > is
> > > > ok for all XHRs, or just for ArrayBuffer fetches?
> > > 
> > > Test are failing: Can you stop calling Resource::setResourceBuffer in
> > > ResourceLoader::requestSynchronously when the option is
DO_NOT_BUFFER_DATA?
> > 
> > Yeah, I'm looking at the test failures right now.  Thanks for the
suggestion,
> > will try :)
> 
> Ok, it fixes content_browsertests in local run.  I'll use CQ dry run again to
> check a wider range of tests.

Hmmm, seems like synchronous requests depend on setResourceBuffer, otherwise
they don't work.  I don't really understand this code well enough to know why.

In new patchset, I tried instead enabling the BufferData option for sync
requests, let me know what you think.

[halliwell, 2016-10-28 20:01:12]

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

[commit-bot: I haz the power, 2016-10-28 20:01:57]

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, 2016-10-28 20:57:55]

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

[commit-bot: I haz the power, 2016-10-28 20:57:56]

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

[halliwell, 2016-10-29 00:36:39]

On 2016/10/28 20:01:02, halliwell wrote:
> On 2016/10/28 04:33:30, halliwell wrote:
> > On 2016/10/28 03:53:59, halliwell wrote:
> > > On 2016/10/28 03:50:46, yhirano wrote:
> > > > On 2016/10/27 16:11:17, halliwell wrote:
> > > > > PTAL.  I wasn't 100% clear from discussion on the bug whether you
think
> > this
> > > > is
> > > > > ok for all XHRs, or just for ArrayBuffer fetches?
> > > > 
> > > > Test are failing: Can you stop calling Resource::setResourceBuffer in
> > > > ResourceLoader::requestSynchronously when the option is
> DO_NOT_BUFFER_DATA?
> > > 
> > > Yeah, I'm looking at the test failures right now.  Thanks for the
> suggestion,
> > > will try :)
> > 
> > Ok, it fixes content_browsertests in local run.  I'll use CQ dry run again
to
> > check a wider range of tests.
> 
> Hmmm, seems like synchronous requests depend on setResourceBuffer, otherwise
> they don't work.  I don't really understand this code well enough to know why.
> 
> In new patchset, I tried instead enabling the BufferData option for sync
> requests, let me know what you think.

There are still some genuine test failures in layout tests that I don't yet
understand.
e.g. http/tests/inspector/network/network-empty-xhr.html has this diff:

http://127.0.0.1:8000/inspector/network/resources/empty.html?async
-resource.content: 
+resource.content: null

[halliwell, 2016-10-30 00:27:40]

On 2016/10/29 00:36:39, halliwell wrote:
> On 2016/10/28 20:01:02, halliwell wrote:
> > On 2016/10/28 04:33:30, halliwell wrote:
> > > On 2016/10/28 03:53:59, halliwell wrote:
> > > > On 2016/10/28 03:50:46, yhirano wrote:
> > > > > On 2016/10/27 16:11:17, halliwell wrote:
> > > > > > PTAL.  I wasn't 100% clear from discussion on the bug whether you
> think
> > > this
> > > > > is
> > > > > > ok for all XHRs, or just for ArrayBuffer fetches?
> > > > > 
> > > > > Test are failing: Can you stop calling Resource::setResourceBuffer in
> > > > > ResourceLoader::requestSynchronously when the option is
> > DO_NOT_BUFFER_DATA?
> > > > 
> > > > Yeah, I'm looking at the test failures right now.  Thanks for the
> > suggestion,
> > > > will try :)
> > > 
> > > Ok, it fixes content_browsertests in local run.  I'll use CQ dry run again
> to
> > > check a wider range of tests.
> > 
> > Hmmm, seems like synchronous requests depend on setResourceBuffer, otherwise
> > they don't work.  I don't really understand this code well enough to know
why.
> > 
> > In new patchset, I tried instead enabling the BufferData option for sync
> > requests, let me know what you think.
> 
> There are still some genuine test failures in layout tests that I don't yet
> understand.
> e.g. http/tests/inspector/network/network-empty-xhr.html has this diff:
> 
> http://127.0.0.1:8000/inspector/network/resources/empty.html?async
> -resource.content: 
> +resource.content: null

yhirano: I will need more time to actually understand what's going on here.  If
you have any more thoughts, that would be appreciated :)

In the meantime I also found that restricting the change to ArrayBuffer XHRs
passes all tests (see this CL https://codereview.chromium.org/2461173002/).  I
could just apply that in our downstream branch to work around the bug while we
figure this one out.

[yhirano, 2016-10-31 04:20:31]

On 2016/10/30 00:27:40, halliwell wrote:
> In the meantime I also found that restricting the change to ArrayBuffer XHRs
> passes all tests (see this CL https://codereview.chromium.org/2461173002/).  I
> could just apply that in our downstream branch to work around the bug while we
> figure this one out.

I haven't look into the issue in detail yet but I suspect that tests pass in the
other CL because we don't have enough coverage. So I would like to understand
what is wrong before landing the change.

[yhirano, 2016-10-31 12:46:13]

yhirano@chromium.org changed reviewers:
+ dgozman@chromium.org

[yhirano, 2016-10-31 12:46:14]

+dgozman@ for the inspector test failure.

[yhirano, 2016-10-31 12:48:18]

If you want to set DO_NOT_BUFFER_DATA only for async requests, that's OK, but it
should be done in XMLHttpRequest.cpp, I think.

[dgozman, 2016-10-31 19:39:20]

On 2016/10/31 12:46:14, yhirano wrote:
> +dgozman@ for the inspector test failure.

I think that http/tests/inspector/network/network-empty-xhr.html failure exposes
a problem: we are not able to fetch content for async xhr anymore.
For inspector, we copy content from memory before resource is collected. See
NetworkResourcesData for details. I think in this case there is no content in
memory when we try to copy it.

[yhirano, 2016-11-01 06:20:15]

On 2016/10/31 19:39:20, dgozman wrote:
> On 2016/10/31 12:46:14, yhirano wrote:
> > +dgozman@ for the inspector test failure.
> 
> I think that http/tests/inspector/network/network-empty-xhr.html failure
exposes
> a problem: we are not able to fetch content for async xhr anymore.
> For inspector, we copy content from memory before resource is collected. See
> NetworkResourcesData for details. I think in this case there is no content in
> memory when we try to copy it.

I thought that InspectorNetworkAgent::didReceiveData[1] copies data when
DoNotbufferData is set. Is my understanding wrong?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...

[dgozman, 2016-11-01 17:59:01]

On 2016/11/01 06:20:15, yhirano wrote:
> On 2016/10/31 19:39:20, dgozman wrote:
> > On 2016/10/31 12:46:14, yhirano wrote:
> > > +dgozman@ for the inspector test failure.
> > 
> > I think that http/tests/inspector/network/network-empty-xhr.html failure
> exposes
> > a problem: we are not able to fetch content for async xhr anymore.
> > For inspector, we copy content from memory before resource is collected. See
> > NetworkResourcesData for details. I think in this case there is no content
in
> > memory when we try to copy it.
> 
> I thought that InspectorNetworkAgent::didReceiveData[1] copies data when
> DoNotbufferData is set. Is my understanding wrong?
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...

Good point. I'm not sure then. Maybe we don't receive data there? Or maybe the
fact that data is empty (it's an empty xhr) makes it report no content?

[halliwell, 2016-11-01 19:02:06]

On 2016/11/01 17:59:01, dgozman wrote:
> On 2016/11/01 06:20:15, yhirano wrote:
> > On 2016/10/31 19:39:20, dgozman wrote:
> > > On 2016/10/31 12:46:14, yhirano wrote:
> > > > +dgozman@ for the inspector test failure.
> > > 
> > > I think that http/tests/inspector/network/network-empty-xhr.html failure
> > exposes
> > > a problem: we are not able to fetch content for async xhr anymore.
> > > For inspector, we copy content from memory before resource is collected.
See
> > > NetworkResourcesData for details. I think in this case there is no content
> in
> > > memory when we try to copy it.
> > 
> > I thought that InspectorNetworkAgent::didReceiveData[1] copies data when
> > DoNotbufferData is set. Is my understanding wrong?
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> 
> Good point. I'm not sure then. Maybe we don't receive data there? Or maybe the
> fact that data is empty (it's an empty xhr) makes it report no content?

It does seem like something special about the empty xhr case.  The tests that
fetch non-empty content appear to pass.

[halliwell, 2016-11-01 23:01:15]

On 2016/11/01 19:02:06, halliwell wrote:
> On 2016/11/01 17:59:01, dgozman wrote:
> > On 2016/11/01 06:20:15, yhirano wrote:
> > > On 2016/10/31 19:39:20, dgozman wrote:
> > > > On 2016/10/31 12:46:14, yhirano wrote:
> > > > > +dgozman@ for the inspector test failure.
> > > > 
> > > > I think that http/tests/inspector/network/network-empty-xhr.html failure
> > > exposes
> > > > a problem: we are not able to fetch content for async xhr anymore.
> > > > For inspector, we copy content from memory before resource is collected.
> See
> > > > NetworkResourcesData for details. I think in this case there is no
content
> > in
> > > > memory when we try to copy it.
> > > 
> > > I thought that InspectorNetworkAgent::didReceiveData[1] copies data when
> > > DoNotbufferData is set. Is my understanding wrong?
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> > 
> > Good point. I'm not sure then. Maybe we don't receive data there? Or maybe
the
> > fact that data is empty (it's an empty xhr) makes it report no content?
> 
> It does seem like something special about the empty xhr case.  The tests that
> fetch non-empty content appear to pass.

Ok, so in the non-empty case, InspectorNetworkAgent::getResponseBody finds
resourceData->hasContent() is true, and returns the data from
resourceData->content():
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...

In the empty case, resourceData->hasContent() returns false, and it falls
further down into the resourceData->cachedResource() case.  The sync request has
a cached resource and returns it here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...

In constrast, the async request, although it goes into the cached resource
branch, InspectorPageAgent::cachedResourceContent returns false, so it falls all
the way through to callback->sendFailure at the bottom of the function.


I'm now trying to understand what 'hasContent' means (seems like it conflates
some concept of 'empty content' with 'I don't have the content'??)  Is there a
way to distinguish those two cases?

[halliwell, 2016-11-01 23:34:11]

On 2016/11/01 23:01:15, halliwell wrote:
> On 2016/11/01 19:02:06, halliwell wrote:
> > On 2016/11/01 17:59:01, dgozman wrote:
> > > On 2016/11/01 06:20:15, yhirano wrote:
> > > > On 2016/10/31 19:39:20, dgozman wrote:
> > > > > On 2016/10/31 12:46:14, yhirano wrote:
> > > > > > +dgozman@ for the inspector test failure.
> > > > > 
> > > > > I think that http/tests/inspector/network/network-empty-xhr.html
failure
> > > > exposes
> > > > > a problem: we are not able to fetch content for async xhr anymore.
> > > > > For inspector, we copy content from memory before resource is
collected.
> > See
> > > > > NetworkResourcesData for details. I think in this case there is no
> content
> > > in
> > > > > memory when we try to copy it.
> > > > 
> > > > I thought that InspectorNetworkAgent::didReceiveData[1] copies data when
> > > > DoNotbufferData is set. Is my understanding wrong?
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> > > 
> > > Good point. I'm not sure then. Maybe we don't receive data there? Or maybe
> the
> > > fact that data is empty (it's an empty xhr) makes it report no content?
> > 
> > It does seem like something special about the empty xhr case.  The tests
that
> > fetch non-empty content appear to pass.
> 
> Ok, so in the non-empty case, InspectorNetworkAgent::getResponseBody finds
> resourceData->hasContent() is true, and returns the data from
> resourceData->content():
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> 
> In the empty case, resourceData->hasContent() returns false, and it falls
> further down into the resourceData->cachedResource() case.  The sync request
has
> a cached resource and returns it here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> 
> In constrast, the async request, although it goes into the cached resource
> branch, InspectorPageAgent::cachedResourceContent returns false, so it falls
all
> the way through to callback->sendFailure at the bottom of the function.
> 
> 
> I'm now trying to understand what 'hasContent' means (seems like it conflates
> some concept of 'empty content' with 'I don't have the content'??)  Is there a
> way to distinguish those two cases?

dgozman: seems like NetworkResourcesData::maybeDecodeDataToContent hits
early-out if (!resourceData->hasData()), so in the case of empty response,
m_content is left as null instead of being set as empty string (this is where
m_content would be populated in the non-empty response case).

Do you have any suggestions on what would be a clean way to fix this?

[dgozman, 2016-11-03 21:29:08]

> > I'm now trying to understand what 'hasContent' means (seems like it
conflates
> > some concept of 'empty content' with 'I don't have the content'??)  Is there
a
> > way to distinguish those two cases?
> 
> dgozman: seems like NetworkResourcesData::maybeDecodeDataToContent hits
> early-out if (!resourceData->hasData()), so in the case of empty response,
> m_content is left as null instead of being set as empty string (this is where
> m_content would be populated in the non-empty response case).
> 
> Do you have any suggestions on what would be a clean way to fix this?

I was under impression that empty data is still hasData(). If that's not the
case, let's maybe fix that?

[halliwell, 2016-11-05 01:53:31]

On 2016/11/03 21:29:08, dgozman wrote:
> > > I'm now trying to understand what 'hasContent' means (seems like it
> conflates
> > > some concept of 'empty content' with 'I don't have the content'??)  Is
there
> a
> > > way to distinguish those two cases?
> > 
> > dgozman: seems like NetworkResourcesData::maybeDecodeDataToContent hits
> > early-out if (!resourceData->hasData()), so in the case of empty response,
> > m_content is left as null instead of being set as empty string (this is
where
> > m_content would be populated in the non-empty response case).
> > 
> > Do you have any suggestions on what would be a clean way to fix this?
> 
> I was under impression that empty data is still hasData(). If that's not the
> case, let's maybe fix that?

hasData is based on whether m_dataBuffer is non-null, but m_dataBuffer is
created lazily in appendData:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...

What would the condition be for hasData if it's not based on whether data has
been appended?

[yhirano, 2016-11-09 06:53:30]

On 2016/11/05 01:53:31, halliwell wrote:
> On 2016/11/03 21:29:08, dgozman wrote:
> > > > I'm now trying to understand what 'hasContent' means (seems like it
> > conflates
> > > > some concept of 'empty content' with 'I don't have the content'??)  Is
> there
> > a
> > > > way to distinguish those two cases?
> > > 
> > > dgozman: seems like NetworkResourcesData::maybeDecodeDataToContent hits
> > > early-out if (!resourceData->hasData()), so in the case of empty response,
> > > m_content is left as null instead of being set as empty string (this is
> where
> > > m_content would be populated in the non-empty response case).
> > > 
> > > Do you have any suggestions on what would be a clean way to fix this?
> > 
> > I was under impression that empty data is still hasData(). If that's not the
> > case, let's maybe fix that?
> 
> hasData is based on whether m_dataBuffer is non-null, but m_dataBuffer is
> created lazily in appendData:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> 
> What would the condition be for hasData if it's not based on whether data has
> been appended?

Is it possible to run 

 m_resourcesData->maybeAddResourceData(requestId, "", 0);

in InspectorNetworkAgent::didFinishLoading if 

  resourceData &&
    (!resourceData->cachedResource() ||
      resourceData->cachedResource()->getDataBufferingPolicy() ==
DoNotBufferData ||
      isErrorStatusCode(resourceData->httpStatusCode())))

holds?

[dgozman, 2016-11-09 19:36:57]

On 2016/11/09 06:53:30, yhirano wrote:
> On 2016/11/05 01:53:31, halliwell wrote:
> > On 2016/11/03 21:29:08, dgozman wrote:
> > > > > I'm now trying to understand what 'hasContent' means (seems like it
> > > conflates
> > > > > some concept of 'empty content' with 'I don't have the content'??)  Is
> > there
> > > a
> > > > > way to distinguish those two cases?
> > > > 
> > > > dgozman: seems like NetworkResourcesData::maybeDecodeDataToContent hits
> > > > early-out if (!resourceData->hasData()), so in the case of empty
response,
> > > > m_content is left as null instead of being set as empty string (this is
> > where
> > > > m_content would be populated in the non-empty response case).
> > > > 
> > > > Do you have any suggestions on what would be a clean way to fix this?
> > > 
> > > I was under impression that empty data is still hasData(). If that's not
the
> > > case, let's maybe fix that?
> > 
> > hasData is based on whether m_dataBuffer is non-null, but m_dataBuffer is
> > created lazily in appendData:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> > 
> > What would the condition be for hasData if it's not based on whether data
has
> > been appended?
> 
> Is it possible to run 
> 
>  m_resourcesData->maybeAddResourceData(requestId, "", 0);
> 
> in InspectorNetworkAgent::didFinishLoading if 
> 
>   resourceData &&
>     (!resourceData->cachedResource() ||
>       resourceData->cachedResource()->getDataBufferingPolicy() ==
> DoNotBufferData ||
>       isErrorStatusCode(resourceData->httpStatusCode())))
> 
> holds?

Sounds good!

[halliwell, 2016-11-09 19:41:49]

On 2016/11/09 19:36:57, dgozman wrote:
> On 2016/11/09 06:53:30, yhirano wrote:
> > On 2016/11/05 01:53:31, halliwell wrote:
> > > On 2016/11/03 21:29:08, dgozman wrote:
> > > > > > I'm now trying to understand what 'hasContent' means (seems like it
> > > > conflates
> > > > > > some concept of 'empty content' with 'I don't have the content'??) 
Is
> > > there
> > > > a
> > > > > > way to distinguish those two cases?
> > > > > 
> > > > > dgozman: seems like NetworkResourcesData::maybeDecodeDataToContent
hits
> > > > > early-out if (!resourceData->hasData()), so in the case of empty
> response,
> > > > > m_content is left as null instead of being set as empty string (this
is
> > > where
> > > > > m_content would be populated in the non-empty response case).
> > > > > 
> > > > > Do you have any suggestions on what would be a clean way to fix this?
> > > > 
> > > > I was under impression that empty data is still hasData(). If that's not
> the
> > > > case, let's maybe fix that?
> > > 
> > > hasData is based on whether m_dataBuffer is non-null, but m_dataBuffer is
> > > created lazily in appendData:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> > > 
> > > What would the condition be for hasData if it's not based on whether data
> has
> > > been appended?
> > 
> > Is it possible to run 
> > 
> >  m_resourcesData->maybeAddResourceData(requestId, "", 0);
> > 
> > in InspectorNetworkAgent::didFinishLoading if 
> > 
> >   resourceData &&
> >     (!resourceData->cachedResource() ||
> >       resourceData->cachedResource()->getDataBufferingPolicy() ==
> > DoNotBufferData ||
> >       isErrorStatusCode(resourceData->httpStatusCode())))
> > 
> > holds?
> 
> Sounds good!

Ok, will try that.  Thanks yhirano :)

[halliwell, 2016-11-10 20:22:02]

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

[commit-bot: I haz the power, 2016-11-10 20:22:33]

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, 2016-11-10 22:17:57]

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

[commit-bot: I haz the power, 2016-11-10 22:17:58]

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

[halliwell, 2016-11-11 02:32:16]

On 2016/11/09 19:41:49, halliwell wrote:
> On 2016/11/09 19:36:57, dgozman wrote:
> > On 2016/11/09 06:53:30, yhirano wrote:
> > > On 2016/11/05 01:53:31, halliwell wrote:
> > > > On 2016/11/03 21:29:08, dgozman wrote:
> > > > > > > I'm now trying to understand what 'hasContent' means (seems like
it
> > > > > conflates
> > > > > > > some concept of 'empty content' with 'I don't have the content'??)

> Is
> > > > there
> > > > > a
> > > > > > > way to distinguish those two cases?
> > > > > > 
> > > > > > dgozman: seems like NetworkResourcesData::maybeDecodeDataToContent
> hits
> > > > > > early-out if (!resourceData->hasData()), so in the case of empty
> > response,
> > > > > > m_content is left as null instead of being set as empty string (this
> is
> > > > where
> > > > > > m_content would be populated in the non-empty response case).
> > > > > > 
> > > > > > Do you have any suggestions on what would be a clean way to fix
this?
> > > > > 
> > > > > I was under impression that empty data is still hasData(). If that's
not
> > the
> > > > > case, let's maybe fix that?
> > > > 
> > > > hasData is based on whether m_dataBuffer is non-null, but m_dataBuffer
is
> > > > created lazily in appendData:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...
> > > > 
> > > > What would the condition be for hasData if it's not based on whether
data
> > has
> > > > been appended?
> > > 
> > > Is it possible to run 
> > > 
> > >  m_resourcesData->maybeAddResourceData(requestId, "", 0);
> > > 
> > > in InspectorNetworkAgent::didFinishLoading if 
> > > 
> > >   resourceData &&
> > >     (!resourceData->cachedResource() ||
> > >       resourceData->cachedResource()->getDataBufferingPolicy() ==
> > > DoNotBufferData ||
> > >       isErrorStatusCode(resourceData->httpStatusCode())))
> > > 
> > > holds?
> > 
> > Sounds good!
> 
> Ok, will try that.  Thanks yhirano :)

That test is fixed.  Two failures left:

(1) fast/encoding/char-decoding.html is using XHR to do char decoding:
req.open('GET', `data:text/plain,${characterSequence}`);

yhirano: do you know why this xhr is affected?

(2) http/tests/inspector-protocol/reload-memory-cache.html   - description
"Tests that reloads when coming from protocol still use memory cache."

dgozman: what exactly is this checking for ... is it still valid given
deliberately disabling memory cache here?  Should the test still exist but use a
different type of request?

[dgozman, 2016-11-11 18:02:10]

> (2) http/tests/inspector-protocol/reload-memory-cache.html   - description
> "Tests that reloads when coming from protocol still use memory cache."
> 
> dgozman: what exactly is this checking for ... is it still valid given
> deliberately disabling memory cache here?  Should the test still exist but use
a
> different type of request?

I think it makes sense to change this test to use e.g. <script src="...">
instead of XHR, since we still cache scripts but not XHRs.

[halliwell, 2016-11-11 21:06:10]

On 2016/11/11 18:02:10, dgozman wrote:
> > (2) http/tests/inspector-protocol/reload-memory-cache.html   - description
> > "Tests that reloads when coming from protocol still use memory cache."
> > 
> > dgozman: what exactly is this checking for ... is it still valid given
> > deliberately disabling memory cache here?  Should the test still exist but
use
> a
> > different type of request?
> 
> I think it makes sense to change this test to use e.g. <script src="...">
> instead of XHR, since we still cache scripts but not XHRs.

Thanks dgozman, that works.

yhirano: I started looking at the char-decoding test.  It seems like the problem
is related to caching.  If you look at the results, it always fails when the
data is the same as a previous request (here are some of the early results):

PASS Decode UTF-8: %E2%88%9A => U+221A 
PASS Decode gb2312: %A3%A0 => U+3000 
FAIL Decode gb_2312: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
""
FAIL Decode gb_2312-80: %A3%A0 => U+3000 assert_equals: expected "U+3000" but
got ""
FAIL Decode csgb2312: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
""
FAIL Decode iso-ir-58: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
""
FAIL Decode csiso58gb231280: %A3%A0 => U+3000 assert_equals: expected "U+3000"
but got ""
FAIL Decode chinese: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
""
FAIL Decode gbk: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got ""
FAIL Decode x-gbk: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got ""
FAIL Decode gb18030: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
""
FAIL Decode EUC-CN: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got ""
PASS Decode gbk: %A8%BF => U+01F9 
PASS Decode gbk: %A1%AD => U+2026 
PASS Decode gbk: %A1%AB => U+FF5E 
FAIL Decode gb18030: %A8%BF => U+01F9 assert_equals: expected "U+01F9" but got
""

Seems like the same Resource object is used, its status is Cached, but
XMLHttpRequest::didReceiveData is never called.  I didn't managed to figure out
why yet, let me know if you do :)

[yhirano, 2016-11-15 06:16:29]

On 2016/11/11 21:06:10, halliwell wrote:
> On 2016/11/11 18:02:10, dgozman wrote:
> > > (2) http/tests/inspector-protocol/reload-memory-cache.html   - description
> > > "Tests that reloads when coming from protocol still use memory cache."
> > > 
> > > dgozman: what exactly is this checking for ... is it still valid given
> > > deliberately disabling memory cache here?  Should the test still exist but
> use
> > a
> > > different type of request?
> > 
> > I think it makes sense to change this test to use e.g. <script src="...">
> > instead of XHR, since we still cache scripts but not XHRs.
> 
> Thanks dgozman, that works.
> 
> yhirano: I started looking at the char-decoding test.  It seems like the
problem
> is related to caching.  If you look at the results, it always fails when the
> data is the same as a previous request (here are some of the early results):
> 
> PASS Decode UTF-8: %E2%88%9A => U+221A 
> PASS Decode gb2312: %A3%A0 => U+3000 
> FAIL Decode gb_2312: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
> ""
> FAIL Decode gb_2312-80: %A3%A0 => U+3000 assert_equals: expected "U+3000" but
> got ""
> FAIL Decode csgb2312: %A3%A0 => U+3000 assert_equals: expected "U+3000" but
got
> ""
> FAIL Decode iso-ir-58: %A3%A0 => U+3000 assert_equals: expected "U+3000" but
got
> ""
> FAIL Decode csiso58gb231280: %A3%A0 => U+3000 assert_equals: expected "U+3000"
> but got ""
> FAIL Decode chinese: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
> ""
> FAIL Decode gbk: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got ""
> FAIL Decode x-gbk: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
""
> FAIL Decode gb18030: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
> ""
> FAIL Decode EUC-CN: %A3%A0 => U+3000 assert_equals: expected "U+3000" but got
""
> PASS Decode gbk: %A8%BF => U+01F9 
> PASS Decode gbk: %A1%AD => U+2026 
> PASS Decode gbk: %A1%AB => U+FF5E 
> FAIL Decode gb18030: %A8%BF => U+01F9 assert_equals: expected "U+01F9" but got
> ""
> 
> Seems like the same Resource object is used, its status is Cached, but
> XMLHttpRequest::didReceiveData is never called.  I didn't managed to figure
out
> why yet, let me know if you do :)

Sorry for the inconvenience. I'm investigating the issue.

[yhirano, 2016-11-16 10:25:56]

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

[commit-bot: I haz the power, 2016-11-16 10:26:16]

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, 2016-11-16 12:07:55]

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

[commit-bot: I haz the power, 2016-11-16 12:07:56]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-16 12:24:08]

OK, I believe https://codereview.chromium.org/2509533002/fixed the problem.

[yhirano, 2016-11-16 12:24:31]

LGTM

[halliwell, 2016-11-16 14:33:22]

On 2016/11/16 12:24:31, yhirano wrote:
> LGTM

Thank you, yhirano!!

dgozman: can you review the inspector changes now please :)

[dgozman, 2016-11-16 15:36:54]

lgtm

[halliwell, 2016-11-16 15:51:26]

The CQ bit was checked by halliwell@chromium.org

[commit-bot: I haz the power, 2016-11-16 15:52:09]

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-11-16 15:58:30]

Description was changed from

==========
Disable memory caching of XHRs

XHRs get removed from memory cache anyway after load finishes (see
issue 399166).  This change can save significant memory during the
fetch for large resources.

BUG=659789
==========

to

==========
Disable memory caching of XHRs

XHRs get removed from memory cache anyway after load finishes (see
issue 399166).  This change can save significant memory during the
fetch for large resources.

BUG=659789
==========

[commit-bot: I haz the power, 2016-11-16 15:58:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-16 16:05:38]

Description was changed from

==========
Disable memory caching of XHRs

XHRs get removed from memory cache anyway after load finishes (see
issue 399166).  This change can save significant memory during the
fetch for large resources.

BUG=659789
==========

to

==========
Disable memory caching of XHRs

XHRs get removed from memory cache anyway after load finishes (see
issue 399166).  This change can save significant memory during the
fetch for large resources.

BUG=659789

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

[commit-bot: I haz the power, 2016-11-16 16:05:40]

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