DCHECK failure in ContentVerifyJob |
||
Issue descriptionCurrently content verification failure due to content modification can cause DCHECK(!failed_) failure in ContentVerifyJob::DispatchFailureCallback (see steps in [1]) This happens if an extension resource > 4k is modified on disk and ContentVerifyJob runs in a way such that the file's content is available (ContentVerifyJob::DoneReading) before CVJ has hashes (ContentVerifyJob::OnHashesReady). The error is due to the fact that while running OnHashesReady, DispatchFailureCallback is errornously called twice from: 1. From the call to BytesRead from OnHashesReady [2] 2. Call to DispatchFailureCallback directly from OnHashesReady[3] In non-debug mode this isn't a problem thanks to OnceCallback ContentVerifyJob::failure_callback_, whose nullness is checked [4] before calling it from DispatchFailureCallback. [1]: Chromium with DCHECK turned on, install an extension from webstore, modify one of its file that is > 4k in size, navigate to that file in browser through chrome-extnesion/id/resource URL [2]: https://cs.chromium.org/chromium/src/extensions/browser/content_verify_job.cc?rcl=b9364568bd9d6c10f7eed68e32e3847376fc022b&l=195 [3]: https://cs.chromium.org/chromium/src/extensions/browser/content_verify_job.cc?rcl=b9364568bd9d6c10f7eed68e32e3847376fc022b&l=200 [4]: https://cs.chromium.org/chromium/src/extensions/browser/content_verify_job.cc?rcl=cc7ddcb128b5ed469bf44edf34790804066871ce&l=224
,
Jan 26 2018
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2181b01c2d22784987e7e166b97718c0d74a981a commit 2181b01c2d22784987e7e166b97718c0d74a981a Author: vitaliii <vitaliii@chromium.org> Date: Tue Jan 30 13:45:26 2018 Revert "Fix extraneous call to ContentVerifyJob::DispatchFailureCallback." This reverts commit 6e548ee1ca4829c87752ef04b2d97ad15a3ff3f9. Reason for revert: DCHECK at content_verify_job.cc:192 occasionally fails (see crbug.com/806586 ). Original change's description: > Fix extraneous call to ContentVerifyJob::DispatchFailureCallback. > > If ContentVerifyJob receives hashes (ContentVerifyJob::OnHashesReady) > after the content is read (ContentVerifyJob::DoneReading), then it > was possible for DispatchFailureCallback to be called more than once. > This causes a DCHECK failure in DispatchFailureCallback > (DCHECK(!failed_)). > > Bail out early in OnHashesReady if call to ContentVerifyJob::BytesRead > leads to a ContentVerifyJob failure. > > Note that this isn't harmful in Release builds as > DispatchFailureCallback tests ContentVerifyJob::failure_callback_ > before calling the callback, which is reset once it is called. > Nevertheless, this is incorrect behavior and this CL fixes that. > > > Bug: 804630 > Test: See bug description for repro steps. > Change-Id: Icf3a5c8b4c0d01cb20e02de14b11aca4aeff03e5 > Reviewed-on: https://chromium-review.googlesource.com/879961 > Reviewed-by: Devlin <rdevlin.cronin@chromium.org> > Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org> > Cr-Commit-Position: refs/heads/master@{#531755} TBR=lazyboy@chromium.org,rdevlin.cronin@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 804630 , 806586 Change-Id: Ia5c67f60e95ddb607cb364c745f31dde465d3c4f Reviewed-on: https://chromium-review.googlesource.com/893258 Reviewed-by: vitaliii <vitaliii@chromium.org> Commit-Queue: vitaliii <vitaliii@chromium.org> Cr-Commit-Position: refs/heads/master@{#532854} [modify] https://crrev.com/2181b01c2d22784987e7e166b97718c0d74a981a/extensions/browser/content_verifier/content_hash.h [modify] https://crrev.com/2181b01c2d22784987e7e166b97718c0d74a981a/extensions/browser/content_verify_job.cc [modify] https://crrev.com/2181b01c2d22784987e7e166b97718c0d74a981a/extensions/browser/content_verify_job_unittest.cc [modify] https://crrev.com/2181b01c2d22784987e7e166b97718c0d74a981a/extensions/common/constants.cc [modify] https://crrev.com/2181b01c2d22784987e7e166b97718c0d74a981a/extensions/common/constants.h
,
Jan 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15f07945f50fd50f710cdb8a0191ac2a83050087 commit 15f07945f50fd50f710cdb8a0191ac2a83050087 Author: Istiaque Ahmed <lazyboy@chromium.org> Date: Wed Jan 31 05:15:03 2018 Revert "Revert "Fix extraneous call to ContentVerifyJob::DispatchFailureCallback."" This reverts commit 2181b01c2d22784987e7e166b97718c0d74a981a. Reason for revert: The offending test (ContentVerifierTest.FailOnRead) has been taken care of in CL: https://chromium-review.googlesource.com/c/chromium/src/+/888327 Original change's description: > Revert "Fix extraneous call to ContentVerifyJob::DispatchFailureCallback." > > This reverts commit 6e548ee1ca4829c87752ef04b2d97ad15a3ff3f9. > > Reason for revert: DCHECK at content_verify_job.cc:192 occasionally fails (see crbug.com/806586 ). > > Original change's description: > > Fix extraneous call to ContentVerifyJob::DispatchFailureCallback. > > > > If ContentVerifyJob receives hashes (ContentVerifyJob::OnHashesReady) > > after the content is read (ContentVerifyJob::DoneReading), then it > > was possible for DispatchFailureCallback to be called more than once. > > This causes a DCHECK failure in DispatchFailureCallback > > (DCHECK(!failed_)). > > > > Bail out early in OnHashesReady if call to ContentVerifyJob::BytesRead > > leads to a ContentVerifyJob failure. > > > > Note that this isn't harmful in Release builds as > > DispatchFailureCallback tests ContentVerifyJob::failure_callback_ > > before calling the callback, which is reset once it is called. > > Nevertheless, this is incorrect behavior and this CL fixes that. > > > > > > Bug: 804630 > > Test: See bug description for repro steps. > > Change-Id: Icf3a5c8b4c0d01cb20e02de14b11aca4aeff03e5 > > Reviewed-on: https://chromium-review.googlesource.com/879961 > > Reviewed-by: Devlin <rdevlin.cronin@chromium.org> > > Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#531755} > > TBR=lazyboy@chromium.org,rdevlin.cronin@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 804630 , 806586 > Change-Id: Ia5c67f60e95ddb607cb364c745f31dde465d3c4f > Reviewed-on: https://chromium-review.googlesource.com/893258 > Reviewed-by: vitaliii <vitaliii@chromium.org> > Commit-Queue: vitaliii <vitaliii@chromium.org> > Cr-Commit-Position: refs/heads/master@{#532854} TBR=lazyboy@chromium.org,rdevlin.cronin@chromium.org,vitaliii@chromium.org Change-Id: I82988d836f49033e1d618125e06f0d45d1124071 Bug: 804630 , 806586 Reviewed-on: https://chromium-review.googlesource.com/894824 Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Cr-Commit-Position: refs/heads/master@{#533209} [modify] https://crrev.com/15f07945f50fd50f710cdb8a0191ac2a83050087/extensions/browser/content_verifier/content_hash.h [modify] https://crrev.com/15f07945f50fd50f710cdb8a0191ac2a83050087/extensions/browser/content_verify_job.cc [modify] https://crrev.com/15f07945f50fd50f710cdb8a0191ac2a83050087/extensions/browser/content_verify_job_unittest.cc [modify] https://crrev.com/15f07945f50fd50f710cdb8a0191ac2a83050087/extensions/common/constants.cc [modify] https://crrev.com/15f07945f50fd50f710cdb8a0191ac2a83050087/extensions/common/constants.h
,
Jan 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/195fd5c7733c87976fd15b7dfc2329fddead0007 commit 195fd5c7733c87976fd15b7dfc2329fddead0007 Author: Istiaque Ahmed <lazyboy@chromium.org> Date: Wed Jan 31 19:24:13 2018 ContentVerification: Add back test that exercises failure during navigation. Change Ib59e9f0c7a4aac9b6a883d7f42647cecc603fa79 removed two tests: ContentVerifierTest.{FailOnRead,FailOnDone} that used to test navigating a WebContents to a corrupted extension resource file and making sure that the navigation correctly disables the extension. However, the tests were simulating read failures in an obtrusive way. Bring back one of the test to preserve test coverage around navigation. Bug: 804630 Change-Id: I57f0e7ea50d7f7bb8a092f33481556593865f43c Reviewed-on: https://chromium-review.googlesource.com/894373 Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#533355} [modify] https://crrev.com/195fd5c7733c87976fd15b7dfc2329fddead0007/chrome/browser/extensions/content_verifier_browsertest.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Jan 25 2018