Issue 8523003: URLFetcher should not act on writes to closed temp files.

Issue 8523003: URLFetcher should not act on writes to closed temp files. (Closed)

Created:
9 years, 1 month ago by Sam Kerner (Chrome)

Modified:
9 years, 1 month ago

Reviewers:
eroman

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam, dpranke-watch+content_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

URLFetcher should not act on writes to closed temp files. BUG=103424 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109687

Patch Set 1 #

Created: 9 years, 1 month ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+16 lines, -0 lines)			Patch
	M	content/common/net/url_fetcher_impl.cc	View		1 chunk	+16 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

eroman

LGTM. Can you add a corresponding unit-test? On Thu, Nov 10, 2011 at 9:59 AM, ...

9 years, 1 month ago (2011-11-10 22:01:04 UTC) #2

LGTM.

Can you add a corresponding unit-test?

On Thu, Nov 10, 2011 at 9:59 AM, <skerner@chromium.org> wrote:

> Reviewers: eroman,
>
> Description:
> URLFetcher should not act on writes to closed temp files.
>
>
> BUG=103424
> TEST=none
>
>
> Please review this at
http://codereview.chromium.**org/8523003/<http://codereview.chromium.org/8523...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M content/common/net/url_**fetcher_impl.cc
>
>
> Index: content/common/net/url_**fetcher_impl.cc
> diff --git a/content/common/net/url_**fetcher_impl.cc
> b/content/common/net/url_**fetcher_impl.cc
> index d0d7ad2e9898a7d3792d84547f62eb**5903173192..**
> cbac732d9dc2bd85ae1734540b5a85**31bfd1cb1c 100644
> --- a/content/common/net/url_**fetcher_impl.cc
> +++ b/content/common/net/url_**fetcher_impl.cc
> @@ -380,6 +380,22 @@ void URLFetcherImpl::Core::**
> TempFileWriter::ContinueWrite(
>     int bytes_written) {
>   DCHECK(core_->io_message_loop_**proxy_->**BelongsToCurrentThread());
>
> +  if (temp_file_handle_ == base::**kInvalidPlatformFileValue) {
> +    // While a write was being done on the file thread, a request
> +    // to close or disown the file occured on the IO thread.  At
> +    // this point a request to close the file is pending on the
> +    // file thread.
> +    return;
> +  }
> +
> +  // Every code path that resets |core_->request_| should reset
> +  // |core->temp_file_writer_| or cause the temp file writer to
> +  // disown the temp file.  In the former case, this callback can
> +  // not be called, because the weak pointer to |this| will be NULL.
> +  // In the latter case, the check of |temp_file_handle_| at the start
> +  // of this method ensures that we can not reach this point.
> +  CHECK(core_->request_.get());
> +
>   if (base::PLATFORM_FILE_OK != error_code) {
>     error_code_ = error_code;
>     RemoveTempFile();
>
>
>

Sam Kerner (Chrome)

On 2011/11/10 22:01:04, eroman wrote: > LGTM. > > Can you add a corresponding unit-test? ...

9 years, 1 month ago (2011-11-11 18:58:06 UTC) #3

On 2011/11/10 22:01:04, eroman wrote:
> LGTM.
> 
> Can you add a corresponding unit-test?

Working on one.  Checking in the fix before it is done because I want to see if
it solves the crashes mentioned in the bug.

> On Thu, Nov 10, 2011 at 9:59 AM, <mailto:skerner@chromium.org> wrote:
> 
> > Reviewers: eroman,
> >
> > Description:
> > URLFetcher should not act on writes to closed temp files.
> >
> >
> > BUG=103424
> > TEST=none
> >
> >
> > Please review this at
>
http://codereview.chromium.**org/8523003/%3Chttp://codereview.chromium.org/85...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >  M content/common/net/url_**fetcher_impl.cc
> >
> >
> > Index: content/common/net/url_**fetcher_impl.cc
> > diff --git a/content/common/net/url_**fetcher_impl.cc
> > b/content/common/net/url_**fetcher_impl.cc
> > index d0d7ad2e9898a7d3792d84547f62eb**5903173192..**
> > cbac732d9dc2bd85ae1734540b5a85**31bfd1cb1c 100644
> > --- a/content/common/net/url_**fetcher_impl.cc
> > +++ b/content/common/net/url_**fetcher_impl.cc
> > @@ -380,6 +380,22 @@ void URLFetcherImpl::Core::**
> > TempFileWriter::ContinueWrite(
> >     int bytes_written) {
> >   DCHECK(core_->io_message_loop_**proxy_->**BelongsToCurrentThread());
> >
> > +  if (temp_file_handle_ == base::**kInvalidPlatformFileValue) {
> > +    // While a write was being done on the file thread, a request
> > +    // to close or disown the file occured on the IO thread.  At
> > +    // this point a request to close the file is pending on the
> > +    // file thread.
> > +    return;
> > +  }
> > +
> > +  // Every code path that resets |core_->request_| should reset
> > +  // |core->temp_file_writer_| or cause the temp file writer to
> > +  // disown the temp file.  In the former case, this callback can
> > +  // not be called, because the weak pointer to |this| will be NULL.
> > +  // In the latter case, the check of |temp_file_handle_| at the start
> > +  // of this method ensures that we can not reach this point.
> > +  CHECK(core_->request_.get());
> > +
> >   if (base::PLATFORM_FILE_OK != error_code) {
> >     error_code_ = error_code;
> >     RemoveTempFile();
> >
> >
> >

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skerner@chromium.org/8523003/1

9 years, 1 month ago (2011-11-11 18:58:20 UTC) #4

Change committed as 109687

Expand Messages | Collapse Messages