New issue
Advanced search Search tips

Issue 894815 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 16
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug

Blocked on:
issue 884807

Blocking:
issue 894819
issue 895198



Sign in to add a comment

fix some bugs in navigation preload response body consumption

Project Member Reported by wanderview@chromium.org, Oct 12

Issue description

In  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.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 12

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

commit c43eba9311573672d0773beed07f117d08eed09f
Author: Ben Kelly <wanderview@chromium.org>
Date: Fri Oct 12 13:36: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: 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}
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/core/fetch/bytes_consumer.h
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.cc
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.h
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer_test.cc
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/core/fetch/fetch_data_loader.cc
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/core/fetch/fetch_data_loader.h
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/modules/service_worker/fetch_event.cc
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/modules/service_worker/fetch_event.h
[modify] https://crrev.com/c43eba9311573672d0773beed07f117d08eed09f/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc

Blocking: 894819
Project Member

Comment 3 by bugdroid1@chromium.org, 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

I can't reproduce the failures locally.  I'll have to investigate what is different about these bots next week.
I believe I have found the problem.  New CL:

https://chromium-review.googlesource.com/c/chromium/src/+/1278910
Blocking: 895198
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Labels: -Pri-2 Merge-Request-71 OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows Pri-1
Status: Fixed (was: Assigned)
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.
Project Member

Comment 9 by sheriffbot@chromium.org, Oct 16

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
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
Are you requesting a merge for CL listed at #7 to M71? And how safe it is to merge?
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.
Labels: -Merge-Review-71 Merge-Approved-71
Approving merge to M71 branch 3578 based on comment #11. Pls merge now. Thank you.
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 16

Labels: -merge-approved-71 merge-merged-3578
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

Labels: Merge-Merged-71-3578
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}
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 1

Sign in to add a comment