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

Issue 679483 link

Starred by 1 user
Status: Fixed
Owner:
mmenke@chromium.org
Closed: Jan 2017
Cc:
csharrison@chromium.org
█ rdsmith@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

RedirectToFileResourceHandler may resume before signalling suspend synchronously

Project Member Reported by rdsmith@chromium.org, Jan 9 2017 
Copying mmenke@'s comment @ https://codereview.chromium.org/2586543003/diff/100001/content/browser/loader/redirect_to_file_resource_handler.cc#newcode319:

"If WriteMore() is called from OnReadCompleted, and BufIsFull(), we set did_defer_ to
true in OnReadCompleted before calling this method.  Then if we write the entire
buffer synchronously in this loop, we end up here (write_cursor_ ==
buf_->offset(), !buf_write_pending_, BufIsFull() are all true)....And then we
call Resume().  But we haven't yet deferred the request!"

It's my belief from reading the code for Writer and the classes it uses that writer_->Write() will never return synchronously; it will only return an error or ERR_IO_PENDING.  So the bug above will not be triggered.  However, it's still a bug--the Writer::Write() API clearly allows for synchronous returns.  

[Matt: What's the right component for bugs in RedirectToFileResourceHandler?]

Comment 1 by mmenke@chromium.org, Jan 9 2017

Cc: -mmenke@chromium.org rdsmith@chromium.org
Owner: mmenke@chromium.org
Status: Assigned (was: Untriaged)
There is no component for browser/loader/ bugs, unfortunately.

It looks to me like FileStream::Context::Write looks like it can theoretically synchronously return on Windows.  Am I missing something?

Comment 2 by mmenke@chromium.org, Jan 9 2017 

Oh, I am, it only completes synchronously on error.

Comment 3 by mmenke@chromium.org, Jan 12 2017 

I think the interface refactor can easily be modified to just magically fix this, but it's enough of a change that I want to have more unittests before doing it (Current CL is written to try and be minimally disruptive to logic flows.  If we change them more, need to make sure we have adequate test coverage).
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 26 2017 

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

commit 9874e16e6ccd443a33d4207619e87c6fc6ad1dcd
Author: mmenke <mmenke@chromium.org>
Date: Thu Jan 26 22:37:20 2017

Add unit tests for RedirectToFileResourceHandler.

It had some integration tests, but no real unit tests.  Also fix its
behavior when a write synchronously succeeds, which would cause it to
resume a request without first pausing it.  The API for writing to files
allows sync completion, though it currently never happens, so was not a
visible bug.

BUG= 679483 , 659317 

Review-Url: https://codereview.chromium.org/2654893002
Cr-Commit-Position: refs/heads/master@{#446472}

[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/redirect_to_file_resource_handler.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/redirect_to_file_resource_handler.h
[add] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/redirect_to_file_resource_handler_unittest.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/resource_loader_unittest.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/test_resource_handler.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/test/BUILD.gn

Comment 5 by mmenke@chromium.org, Jan 27 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment