Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766733002:
Reland of allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously (Closed)
|
Created:
3 years, 9 months ago by mmenke Modified:
3 years, 8 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, Randy Smith (Not in Mondays), yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, loading-reviews_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland of https://codereview.chromium.org/2738973002.
It failed to mark a request as unblocked before resuming it.
Unittests didn't catch this because they don't actually drive
a URLRequest, which is where the failing DCHECK was.
Reason for revert:
This CL leads to a layout test crash.
virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html
Original issue's description:
> Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously
> when a Mojo buffer can't be allocated synchronously.
>
> The class still needs to create its own intermediary buffer
> when the Mojo buffer is too small to meet the
> MimeSniffingResourceHandler's needs, but once that's fixed,
> the class can be made significantly simpler.
>
> BUG=687240
>
> Review-Url: https://codereview.chromium.org/2738973002
> Cr-Commit-Position: refs/heads/master@{#458093}
> Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8479a12a2f8aa
Revert info:
> Review-Url: https://codereview.chromium.org/2760223002
> Cr-Commit-Position: refs/heads/master@{#458297}
BUG=687240, 703471
Patch Set 1 : Original CL (Merged to trunk - uninteresting merge) #Patch Set 2 : Fix #
Messages
Total messages: 40 (33 generated)
Description was changed from ========== ... BUG= ========== to ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the test is. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} Committed: https://chromium.googlesource.com/chromium/src/+/4fede0f13bb7bd62971e98b96654... ==========
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
Just a silly little fix - tests didn't catch it because a DCHECK in URLRequest was being triggered, but the unit tests don't actually drive the URLRequest. There aren't any integration tests in content, currently, and loader/ changes don't trigger blink trybots, sadly.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/21 21:16:54, mmenke wrote: > Just a silly little fix - tests didn't catch it because a DCHECK in URLRequest > was being triggered, but the unit tests don't actually drive the URLRequest. > There aren't any integration tests in content, currently, and loader/ changes > don't trigger blink trybots, sadly. Err, sorry, the blink tests triggered a DCHECK in URLRequest, but unit tests don't actually drive the URLRequest, they just pretend to.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Description was changed from ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the test is. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} Committed: https://chromium.googlesource.com/chromium/src/+/4fede0f13bb7bd62971e98b96654... ========== to ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the DCHECK is. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} Committed: https://chromium.googlesource.com/chromium/src/+/4fede0f13bb7bd62971e98b96654... ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the DCHECK is. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} Committed: https://chromium.googlesource.com/chromium/src/+/4fede0f13bb7bd62971e98b96654... ========== to ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the failing DCHECK was. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} Committed: https://chromium.googlesource.com/chromium/src/+/4fede0f13bb7bd62971e98b96654... ==========
*nodsigh* around lack of integration tests in content/. LGTM.
The CQ bit was checked by mmenke@chromium.org
On 2017/03/21 21:28:54, Randy Smith (Not in Mondays) wrote: > *nodsigh* around lack of integration tests in content/. LGTM. Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by mmenke@chromium.org
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490131776062990,
"parent_rev": "0116ce334c021757db311c867d96bb56faeda1b7", "commit_rev":
"6add17253a0318bb3360c33de735f1f0ab3b8cd2"}
Description was changed from ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the failing DCHECK was. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} Committed: https://chromium.googlesource.com/chromium/src/+/4fede0f13bb7bd62971e98b96654... ========== to ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the failing DCHECK was. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} ==========
Description was changed from ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the failing DCHECK was. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... TBR=rdsmith@chromium.org,mmenke@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. BUG=687240, 703471 Review-Url: https://codereview.chromium.org/2760223002 Cr-Commit-Position: refs/heads/master@{#458297} ========== to ========== Reland of https://codereview.chromium.org/2738973002. It failed to mark a request as unblocked before resuming it. Unittests didn't catch this because they don't actually drive a URLRequest, which is where the failing DCHECK was. Reason for revert: This CL leads to a layout test crash. virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html Original issue's description: > Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously > when a Mojo buffer can't be allocated synchronously. > > The class still needs to create its own intermediary buffer > when the Mojo buffer is too small to meet the > MimeSniffingResourceHandler's needs, but once that's fixed, > the class can be made significantly simpler. > > BUG=687240 > > Review-Url: https://codereview.chromium.org/2738973002 > Cr-Commit-Position: refs/heads/master@{#458093} > Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8... Revert info: > Review-Url: https://codereview.chromium.org/2760223002 > Cr-Commit-Position: refs/heads/master@{#458297} BUG=687240, 703471 ==========
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/21 21:30:32, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Gah! Already landed...I accidentally copied the NOTRY from the revert's description.
On 2017/03/21 21:35:47, mmenke wrote: > On 2017/03/21 21:30:32, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > Gah! Already landed...I accidentally copied the NOTRY from the revert's > description. Given the nature of the change, I'll just watch the tree, rather than reverting.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
