New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 766240 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Proj-Servicification

Blocking:
issue 729849



Sign in to add a comment

URLLoaderImpl::OnResponseBodyStreamReady(): handle failure results.

Project Member Reported by yzshen@chromium.org, Sep 18 2017

Issue description

This is causing layout test flakiness:
 - virtual/mojo-loading/http/tests/devtools/audits/audits-panel-noimages-functional.html

Please see https://chromium-swarm.appspot.com/task?id=38aecd57d4b5c710&refresh=10&show_raw=1

10:16:10.025 22085   [24059:24136:0918/101609.936044:12417338075:FATAL:url_loader_impl.cc(458)] Check failed: result == MOJO_RESULT_OK (1 vs. 0)
10:16:10.025 22085   #0 0x000001ccda17 base::debug::StackTrace::StackTrace()
10:16:10.025 22085   #1 0x000001ce49d1 logging::LogMessage::~LogMessage()
10:16:10.025 22085   #2 0x000001b394cc content::URLLoaderImpl::OnResponseBodyStreamReady()
10:16:10.025 22085   #3 0x00000093ed60 mojo::SimpleWatcher::DiscardReadyState()
10:16:10.025 22085   #4 0x000001ec8912 mojo::SimpleWatcher::OnHandleReady()
10:16:10.025 22085   #5 0x000001ec8cee mojo::SimpleWatcher::Context::Notify()
10:16:10.025 22085   #6 0x000001ec7ade mojo::SimpleWatcher::Context::CallNotify()
10:16:10.025 22085   #7 0x000002f603a5 mojo::edk::Watch::InvokeCallback()
10:16:10.025 22085   #8 0x000002f5c100 mojo::edk::RequestContext::~RequestContext()
10:16:10.025 22085   #9 0x000002f5c227 mojo::edk::RequestContext::~RequestContext()
10:16:10.025 22085   #10 0x000002f674cb mojo::edk::NodeChannel::OnChannelMessage()
10:16:10.025 22085   #11 0x000002f65d04 mojo::edk::Channel::OnReadComplete()
10:16:10.025 22085   #12 0x000002f690c7 mojo::edk::(anonymous namespace)::ChannelPosix::OnFileCanReadWithoutBlocking()
10:16:10.026 22085   #13 0x000001cee460 base::MessagePumpLibevent::OnLibeventNotification()
10:16:10.026 22085   #14 0x000001d8660e event_base_loop
10:16:10.026 22085   #15 0x000001cee6be base::MessagePumpLibevent::Run()
10:16:10.026 22085   #16 0x000001ceac9a base::MessageLoop::Run()
10:16:10.026 22085   #17 0x000001d0c1a6 base::RunLoop::Run()
10:16:10.026 22085   #18 0x000001d3380c base::Thread::Run()
10:16:10.026 22085   #19 0x000001d33db2 base::Thread::ThreadMain()
10:16:10.026 22085   #20 0x000001d2c59c base::(anonymous namespace)::ThreadFunc()
10:16:10.026 22085   #21 0x7f939822b184 start_thread
10:16:10.026 22085   #22 0x7f9393938ffd clone
 

Comment 1 by mmenke@chromium.org, Sep 19 2017

Hrm...Do things just work if we remove that DCHECK?  I assume the following BeginWriteData call would return an error, and we already handle the case where that fails.

Comment 2 by yzshen@chromium.org, Sep 19 2017

Cc: mmenke@chromium.org
Owner: yzshen@chromium.org
I think remove the DCHECK should be safe.
In addition to that, I think we get result == 1 here because when we get a peer closed notification for the response body pipe, we close our producer handle without explicitly cancel the watcher on it.

I will produce a patch. Thanks!
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 20 2017

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

commit 8867cd665fb39ec2f07627c0c18f47250359f3a6
Author: Yuzhu Shen <yzshen@chromium.org>
Date: Wed Sep 20 21:19:09 2017

URLLoaderImpl: cancel watching writability of response body handle before closing it.

BUG= 766240 

Change-Id: Ie8b0dade40c90d534b0b1a78a9011f50eb3c08e3
Reviewed-on: https://chromium-review.googlesource.com/673356
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Yuzhu Shen <yzshen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503249}
[modify] https://crrev.com/8867cd665fb39ec2f07627c0c18f47250359f3a6/content/network/url_loader_impl.cc
[modify] https://crrev.com/8867cd665fb39ec2f07627c0c18f47250359f3a6/content/network/url_loader_impl.h
[modify] https://crrev.com/8867cd665fb39ec2f07627c0c18f47250359f3a6/content/network/url_loader_unittest.cc

Comment 4 by yzshen@chromium.org, Sep 20 2017

Status: Fixed (was: Available)

Comment 5 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.

Sign in to add a comment