fix some bugs in navigation preload response body consumption |
||||||||
Issue descriptionIn bug 884807 I landed a CL to allow the navigation preload underlying mojo::DataPipe to be re-used when the response is directly passed to respondWith(). In further testing I found some issues with new DataPipeBytesConsumer that could be triggered in some cases where the navigation preload response is read directly instead of being passed to respondWith(). I have a CL ready to fix these issues, but it missed the branch point yesterday. I'm splitting this into a separate bug to make the merge process easier.
,
Oct 12
,
Oct 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b54b6aa8be4525318436d271397fc7d90c21da4 commit 3b54b6aa8be4525318436d271397fc7d90c21da4 Author: Clark DuVall <cduvall@chromium.org> Date: Fri Oct 12 20:54:35 2018 Revert "Make DataPipeBytesConsumer support ReadableStream loading better." This reverts commit c43eba9311573672d0773beed07f117d08eed09f. Reason for revert: This broke http/tests/devtools/service-workers/service-workers-navigation-preload.js on the mojo bots: Mojo Windows: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mojo%20Windows/18554 Mojo Linux: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mojo%20Linux/20553 Original change's description: > Make DataPipeBytesConsumer support ReadableStream loading better. > > This CL fixes some edge condition interactions when being loaded as > a ReadableStream. In particular, it: > > 1. Properly distinguishes between the end of the DataPipe and a closed > DataPipe with bytes left to be read. A ReadableStream that is not > actively draining the pipe could get closed too early. > 2. Responses must support explicit completion in order to handle error > conditions properly. This CL makes DataPipeBytesConsumer wait > for an explicit signal before closing. > 3. Service worker navigation preload is updated to provide the explicit > completion signals. > > Bug: 894815 > Change-Id: I8cff3de94aa2dcbc8deb4a9601a95c13b8ab94d9 > Reviewed-on: https://chromium-review.googlesource.com/c/1272715 > Commit-Queue: Ben Kelly <wanderview@chromium.org> > Reviewed-by: Yutaka Hirano <yhirano@chromium.org> > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Cr-Commit-Position: refs/heads/master@{#599192} TBR=kinuko@chromium.org,mek@chromium.org,yhirano@chromium.org,wanderview@chromium.org Change-Id: Iddb6b2121a0b014b4b08c6ec64fc534820474010 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 894815 Reviewed-on: https://chromium-review.googlesource.com/c/1279194 Reviewed-by: Clark DuVall <cduvall@chromium.org> Commit-Queue: Clark DuVall <cduvall@chromium.org> Cr-Commit-Position: refs/heads/master@{#599343} [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/core/fetch/bytes_consumer.h [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.cc [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.h [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer_test.cc [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/core/fetch/fetch_data_loader.cc [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/core/fetch/fetch_data_loader.h [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/modules/service_worker/fetch_event.cc [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/modules/service_worker/fetch_event.h [modify] https://crrev.com/3b54b6aa8be4525318436d271397fc7d90c21da4/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc
,
Oct 12
I can't reproduce the failures locally. I'll have to investigate what is different about these bots next week.
,
Oct 14
I believe I have found the problem. New CL: https://chromium-review.googlesource.com/c/chromium/src/+/1278910
,
Oct 15
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e178e0de8ca17d871fcdf56a79915bf7ebd7671 commit 2e178e0de8ca17d871fcdf56a79915bf7ebd7671 Author: Ben Kelly <wanderview@chromium.org> Date: Mon Oct 15 20:44:48 2018 Make DataPipeBytesConsumer support ReadableStream loading better. This CL fixes some edge condition interactions when being loaded as a ReadableStream. In particular, it: 1. Properly distinguishes between the end of the DataPipe and a closed DataPipe with bytes left to be read. A ReadableStream that is not actively draining the pipe could get closed too early. 2. Responses must support explicit completion in order to handle error conditions properly. This CL makes DataPipeBytesConsumer wait for an explicit signal before closing. 3. Service worker navigation preload is updated to provide the explicit completion signals. Bug: 894815 Change-Id: I2b5d61f9fe4a6f17c37e8a728b2f081debe6d936 Reviewed-on: https://chromium-review.googlesource.com/c/1278910 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Ben Kelly <wanderview@chromium.org> Cr-Commit-Position: refs/heads/master@{#599739} [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/resources/navigation-preload-worker.js [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/core/fetch/bytes_consumer.h [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.cc [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.h [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer_test.cc [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/core/fetch/fetch_data_loader.cc [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/core/fetch/fetch_data_loader.h [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/modules/service_worker/fetch_event.cc [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/modules/service_worker/fetch_event.h [modify] https://crrev.com/2e178e0de8ca17d871fcdf56a79915bf7ebd7671/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc
,
Oct 16
I would like to merge this to 71 to fix bug 895198, which is a release blocker. This CL has been released in canary 72.0.3582.0 and I have verified on my local device that bug 895198 no longer occurs in that version.
,
Oct 16
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 16
Are you requesting a merge for CL listed at #7 to M71? And how safe it is to merge?
,
Oct 16
Yes, I'd like to merge the CL listed at #c7 to M71. I believe it is safe to merge. It fixes a known bug in M71 and adds additional automated tests for the affected code paths. I've also run this code (DataPipeBytesConsumer) in a follow-on CL (https://crrev.com/c/1251841) that uses the code in more places subjecting it to many more tests. This combined testing gives me confidence that this CL is safe to merge. Note, we want to get this code in 71 to support a partner site that we are working with.
,
Oct 16
Approving merge to M71 branch 3578 based on comment #11. Pls merge now. Thank you.
,
Oct 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/76f55cc9931961721237cde48796b34adf3328b8 commit 76f55cc9931961721237cde48796b34adf3328b8 Author: Ben Kelly <wanderview@chromium.org> Date: Tue Oct 16 15:25:24 2018 Make DataPipeBytesConsumer support ReadableStream loading better. This CL fixes some edge condition interactions when being loaded as a ReadableStream. In particular, it: 1. Properly distinguishes between the end of the DataPipe and a closed DataPipe with bytes left to be read. A ReadableStream that is not actively draining the pipe could get closed too early. 2. Responses must support explicit completion in order to handle error conditions properly. This CL makes DataPipeBytesConsumer wait for an explicit signal before closing. 3. Service worker navigation preload is updated to provide the explicit completion signals. Bug: 894815 Change-Id: I2b5d61f9fe4a6f17c37e8a728b2f081debe6d936 Reviewed-on: https://chromium-review.googlesource.com/c/1278910 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Ben Kelly <wanderview@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#599739}(cherry picked from commit 2e178e0de8ca17d871fcdf56a79915bf7ebd7671) Reviewed-on: https://chromium-review.googlesource.com/c/1283412 Reviewed-by: Ben Kelly <wanderview@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#38} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/WebKit/LayoutTests/http/tests/devtools/service-workers/resources/navigation-preload-worker.js [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/core/fetch/bytes_consumer.h [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.cc [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.h [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer_test.cc [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/core/fetch/fetch_data_loader.cc [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/core/fetch/fetch_data_loader.h [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/modules/service_worker/fetch_event.cc [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/modules/service_worker/fetch_event.h [modify] https://crrev.com/76f55cc9931961721237cde48796b34adf3328b8/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc
,
Oct 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/76f55cc9931961721237cde48796b34adf3328b8 Commit: 76f55cc9931961721237cde48796b34adf3328b8 Author: wanderview@chromium.org Commiter: wanderview@chromium.org Date: 2018-10-16 15:25:24 +0000 UTC Make DataPipeBytesConsumer support ReadableStream loading better. This CL fixes some edge condition interactions when being loaded as a ReadableStream. In particular, it: 1. Properly distinguishes between the end of the DataPipe and a closed DataPipe with bytes left to be read. A ReadableStream that is not actively draining the pipe could get closed too early. 2. Responses must support explicit completion in order to handle error conditions properly. This CL makes DataPipeBytesConsumer wait for an explicit signal before closing. 3. Service worker navigation preload is updated to provide the explicit completion signals. Bug: 894815 Change-Id: I2b5d61f9fe4a6f17c37e8a728b2f081debe6d936 Reviewed-on: https://chromium-review.googlesource.com/c/1278910 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Ben Kelly <wanderview@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#599739}(cherry picked from commit 2e178e0de8ca17d871fcdf56a79915bf7ebd7671) Reviewed-on: https://chromium-review.googlesource.com/c/1283412 Reviewed-by: Ben Kelly <wanderview@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#38} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Nov 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c4e604aa9276fa14d80bab1cc0501a2c5b33f92c commit c4e604aa9276fa14d80bab1cc0501a2c5b33f92c Author: Yutaka Hirano <yhirano@chromium.org> Date: Thu Nov 01 09:27:56 2018 Introduce DataPipeBytesConsumer::CompletionNotifier Move SignalComplete and SignalError from DataPipeBytesConsumer to the class to separate them from BytesConsumer interface. Bug: 894815 Change-Id: Ib592ba7615c9c0d0fa24e9a9e1a7a07f56cca8ab Reviewed-on: https://chromium-review.googlesource.com/c/1304115 Reviewed-by: Ben Kelly <wanderview@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#604530} [modify] https://crrev.com/c4e604aa9276fa14d80bab1cc0501a2c5b33f92c/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.cc [modify] https://crrev.com/c4e604aa9276fa14d80bab1cc0501a2c5b33f92c/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.h [modify] https://crrev.com/c4e604aa9276fa14d80bab1cc0501a2c5b33f92c/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer_test.cc [modify] https://crrev.com/c4e604aa9276fa14d80bab1cc0501a2c5b33f92c/third_party/blink/renderer/modules/service_worker/fetch_event.cc [modify] https://crrev.com/c4e604aa9276fa14d80bab1cc0501a2c5b33f92c/third_party/blink/renderer/modules/service_worker/fetch_event.h |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Oct 12