URLRequestFileJob doesn't call OnReadComplete for 0 byte files |
||
Issue descriptionI'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.
,
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.
,
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)
,
Mar 22 2017
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
,
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.
,
Mar 28 2017
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.
,
Mar 28 2017
SGTM
,
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
,
Mar 29 2017
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.
,
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 |
||
Comment 1 by mmenke@chromium.org
, Mar 22 2017Owner: ----
Status: Available (was: Assigned)