New issue
Advanced search Search tips

Issue 703892 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 703888



Sign in to add a comment

URLRequestFileJob doesn't call OnReadComplete for 0 byte files

Project Member Reported by lazyboy@chromium.org, Mar 21 2017

Issue description

I've discovered this while investigating extensions corruption. Here's a test I wrote to quickly demonstrate the issue:
https://codereview.chromium.org/2770503002/

Run URLRequestFileJobEventsTest.ZeroFile

You'd only see these printfs:
T: OnOpenComplete: 0
T: OnSeekComplete: 0

but not OnReadComplete.
Assigning to mmenke@ for triaging.
 

Comment 1 by mmenke@chromium.org, Mar 22 2017

Labels: Fixit-Net
Owner: ----
Status: Available (was: Assigned)

Comment 2 by mmenke@chromium.org, Mar 22 2017

This is probably not going to get fixed by anyone on the network stack team - it's something added by an external team in code no one on the network stack is using or actively maintaining, and it's not a crasher.  Patches certainly welcome, though.

Comment 3 by mmenke@chromium.org, Mar 22 2017

I had thought you mean the 0-byte EOF read was not sent to the URLRequest::Delegate, which would be a more significant issue (Though perhaps this bug indicates that may be happening, too)
Oh, did you think it was Delegate::OnReadCompleted?

This is how I created the file:
rm background.js
touch background.js

I looked at logs on ResourceLoader (that's the delegate, right?), I do see one ResourceLoader::OnReadCompleted:

ResourceLoader::OnReadCompleted: bytes_read = 0, url = chrome-extension://bijihlabcfdnabacffofojgmehjdielb/background.js

Details: interesting function call logs of URLRequestExtensionJob (subclass of URLRequestFileJob):

1) For 0 byte case:
URLRequestExtensionJob::URLRequestExtensionJob
URLRequestExtensionJob::OnOpenComplete(result = 0)
URLRequestExtensionJob::OnSeekComplete(result = 0)
ResourceLoader::OnResponseStarted(url = chrome-extension://bijihlabcfdnabacffofojgmehjdielb/background.js)
ResourceLoader::OnReadCompleted(bytes_read = 0, url = chrome-extension://bijihlabcfdnabacffofojgmehjdielb/background.js)
URLRequestExtensionJob::~URLRequestExtensionJob

2) For non 0 case, 73 bytes file (obviously that works):
URLRequestExtensionJob::URLRequestExtensionJob
URLRequestExtensionJob::OnOpenComplete(result = 0)
URLRequestExtensionJob::OnSeekComplete(result = 0)
ResourceLoader::OnResponseStarted url = chrome-extension://bijihlabcfdnabacffofojgmehjdielb/new.js
URLRequestExtensionJob::OnReadComplete(result = 73)  <-------- DIFFERENCE
ResourceLoader::OnReadCompleted(bytes_read = 73, url = chrome-extension://bijihlabcfdnabacffofojgmehjdielb/new.js)
ResourceLoader::OnReadCompleted(bytes_read = 0, url = chrome-extension://bijihlabcfdnabacffofojgmehjdielb/new.js)
URLRequestExtensionJob::~URLRequestExtensionJob

Comment 5 by mmenke@chromium.org, Mar 22 2017

Thanks!  Yea, looks like the URLRequest::Delegate is getting the right results, so not too concerned about this.

If you need an OnReadComplete(0), feel free to add it (I'm happy to review), but should probably glance at other consumers of the interface, to make sure it doesn't break them.
Ah it calls URLRequestJob::DoneReading() for the signal I'm looking for, even for zero byte files!
That should be enough for me to correctly identify 0 byte file's ending in URLRequestExtensionJob.
I'll update the tests in url_request_file_job to add a zero byte case and make sure we call DoneReading(). Then I'll close this WontFix.

Comment 7 by mmenke@chromium.org, Mar 28 2017

SGTM
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/306eec3b648d47328aba78b0c80bf1d59f72f42e

commit 306eec3b648d47328aba78b0c80bf1d59f72f42e
Author: lazyboy <lazyboy@chromium.org>
Date: Wed Mar 29 20:06:15 2017

Modify tests to ensure URLRequestJob::DoneReading is always called,
even for zero byte files.

URLRequestExtensionJob depends on this behavior, so add a zero-byte
test to url_request_file_job_unittest. Also update other relevant tests
in that file to check for DoneReading.

This is merely a test addition/modification for  crbug.com/703892 

BUG= 703892 
Test=None, internal change only.

Review-Url: https://codereview.chromium.org/2776873002
Cr-Commit-Position: refs/heads/master@{#460507}

[modify] https://crrev.com/306eec3b648d47328aba78b0c80bf1d59f72f42e/net/url_request/url_request_file_job_unittest.cc

Status: WontFix (was: Available)
I've added a test for zero byte file, updated the tests in url_request_file_job_unittest to make sure we see URLRequestJob::DoneReading for files that are successfully read.

The underlying issue is WontFix as I've pointed out in comment #6. Closing.
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e83ab9c6c27deb3e96dee4a33a6518c206724d16

commit e83ab9c6c27deb3e96dee4a33a6518c206724d16
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Mar 30 03:18:26 2017

Make content verification see 0-byte files.

Currently ContentVerifyJob never sees DoneReading() for (legitimate
or not) 0-byte files. This means 0-byte files (legitimate or corrupted)
never goes through content verification process.
Override URLRequestJob::DoneReading() in URLRequestExtensionJob to
detect the end of 0-byte files so that ContentVerifyJob knows about
these 0-byte files.

Add a test to ensure that URLRequestExtensionJob will see 0-byte files.
ContentVerifyJobUnittest.DeletedAndMissingFiles already makes sure
that 0-byte read will fail content verification.

BUG= 703888 , 703892 
Test=With content verification turned on (e.g. with
--extension-content-verification=enforce_strict flag), install an
extension from webstore and modify a JS content inside that extension
to 0 byte file. Content verification should fail for that extension.
If the extension is part of system policy, the extension will be
reinsalled.

Review-Url: https://codereview.chromium.org/2779083005
Cr-Commit-Position: refs/heads/master@{#460638}

[modify] https://crrev.com/e83ab9c6c27deb3e96dee4a33a6518c206724d16/chrome/browser/extensions/extension_protocols_unittest.cc
[modify] https://crrev.com/e83ab9c6c27deb3e96dee4a33a6518c206724d16/extensions/browser/extension_protocols.cc

Sign in to add a comment