Only support seeking file streams from the beginning of the file.

Only support seeking file streams from the beginning of the file.

On Windows file streams are backed by files opened for overlapped I/O.
In this mode SetFilePointerEx (used for seeking) will return a write-
protected media error if the file is on write-protected media. This is
even if the file is opened read-only! Since no code (except for the
FileStream tests) calls Seek with anything other than File::FROM_BEGIN
it is simplest to simply remove this capability. That way we can update
the offset in the FileStream context's overlapped structure without
going through a call to Windows.

BUG=478227
Committed: https://crrev.com/aa435e70c70364430093361b5b8c460831f80bb4
Cr-Commit-Position: refs/heads/master@{#334518}

[Reilly Grant (use Gerrit), 2015-05-20 23:30:14]

Patchset #1 (id:1) has been deleted

[Reilly Grant (use Gerrit), 2015-05-21 00:03:56]

reillyg@chromium.org changed reviewers:
+ davidben@chromium.org, hirono@chromium.org, tzik@chromium.org

[Reilly Grant (use Gerrit), 2015-05-21 00:06:29]

davidben@, please take a look at the bulk of this change in //net and //content.

tzik@, please take a look at //storage/browser/fileapi.

hirono@, please take a look at //chrome/browser/chromeos/drive.

[hirono, 2015-05-21 01:40:15]

For //chrome/browser/chromeos/drive, lgtm

[davidben, 2015-05-21 17:22:12]

davidben@chromium.org changed reviewers:
+ eroman@chromium.org

[davidben, 2015-05-21 17:22:12]

I'm not super-familiar with Windows. Eric, do you mind looking at this? I'll
stamp content when you're happy with it.

https://codereview.chromium.org/1148383003/diff/20001/net/base/file_stream_co...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/1148383003/diff/20001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:117: FileStream::Context::IOResult
FileStream::Context::SeekFileImpl(int64 offset) {
If I'm understanding this correctly, not calling seek on the underlying
base::File only works because we always use overlapped IO, correct? This seems
to be correct, but it's probably worth a comment to this effect and why we're
not seeking the file.

There is a file() accessor to FileStream::Context which would be sensitive to
this. If we're relying on it, probably best to not leak that method. It's only
used to implement FileStream::IsOpen and FileStream::GetFileForTesting, which is
only used to assert on things redundant with asserting on IsOpen. Perhaps
replace both of those with FileStream::Context::IsOpen. That way we can be sure
that the base::File doesn't leak.

Actually, does this work if the file doesn't support overlapped IO? Is that
meaningful? This seems to suggest it matters whether the file was opened for
overlapped IO? Though if it wasn't, presumably we'd have problems with doing
disk on the IO thread. I see line 65 has a TODO to check it... perhaps checking
that file_.async() is true is sufficient?

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747%28v=vs.85%2...

[Reilly Grant (use Gerrit), 2015-05-21 18:36:53]

On 2015/05/21 17:22:12, David Benjamin wrote:
> I'm not super-familiar with Windows. Eric, do you mind looking at this? I'll
> stamp content when you're happy with it.
> 
>
https://codereview.chromium.org/1148383003/diff/20001/net/base/file_stream_co...
> File net/base/file_stream_context_win.cc (right):
> 
>
https://codereview.chromium.org/1148383003/diff/20001/net/base/file_stream_co...
> net/base/file_stream_context_win.cc:117: FileStream::Context::IOResult
> FileStream::Context::SeekFileImpl(int64 offset) {
> If I'm understanding this correctly, not calling seek on the underlying
> base::File only works because we always use overlapped IO, correct? This seems
> to be correct, but it's probably worth a comment to this effect and why we're
> not seeking the file.
> 
> There is a file() accessor to FileStream::Context which would be sensitive to
> this. If we're relying on it, probably best to not leak that method. It's only
> used to implement FileStream::IsOpen and FileStream::GetFileForTesting, which
is
> only used to assert on things redundant with asserting on IsOpen. Perhaps
> replace both of those with FileStream::Context::IsOpen. That way we can be
sure
> that the base::File doesn't leak.
> 
> Actually, does this work if the file doesn't support overlapped IO? Is that
> meaningful? This seems to suggest it matters whether the file was opened for
> overlapped IO? Though if it wasn't, presumably we'd have problems with doing
> disk on the IO thread. I see line 65 has a TODO to check it... perhaps
checking
> that file_.async() is true is sufficient?
> 
>
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747%28v=vs.85%2...

I've removed the file() accessor as suggested. file_steam_context_win.cc assumes
that the file supports overlapped I/O and won't work without it. I've added the
DCHECK suggested by line 65 to make sure that the file was opened properly.

[eroman, 2015-05-21 22:25:34]

The description sounds reasonable.

Ricardo would be the expert to review this, but he is out right now (cc-ed)

I will take a look as soon as I get the chance

[Reilly Grant (use Gerrit), 2015-05-22 20:53:31]

reillyg@chromium.org changed reviewers:
+ thestig@chromium.org

[Reilly Grant (use Gerrit), 2015-05-22 20:53:32]

Adding thestig@ for //base review. Let me know if you think there's a better way
of setting the async flag. It doesn't look like there's a way to check the flags
the HANDLE was opened with.

[Lei Zhang, 2015-05-25 00:29:58]

https://codereview.chromium.org/1148383003/diff/80001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1148383003/diff/80001/base/files/file.h#newco...
base/files/file.h:172: File(PlatformFile platform_file, bool async);
Calling this with |async| set to false is the same as the ctor above. Maybe do
"static File CreateForAsyncHandle(PlatformFile platform_file)" instead?

[tzik, 2015-05-25 07:32:17]

fileapi part LGTM.

[Reilly Grant (use Gerrit), 2015-05-26 23:41:33]

https://codereview.chromium.org/1148383003/diff/80001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1148383003/diff/80001/base/files/file.h#newco...
base/files/file.h:172: File(PlatformFile platform_file, bool async);
On 2015/05/25 00:29:58, Lei Zhang wrote:
> Calling this with |async| set to false is the same as the ctor above. Maybe do
> "static File CreateForAsyncHandle(PlatformFile platform_file)" instead?

Done.

[Lei Zhang, 2015-05-26 23:58:06]

base/ lgtm with comment added.

https://codereview.chromium.org/1148383003/diff/100001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1148383003/diff/100001/base/files/file.h#newc...
base/files/file.h:179: static File CreateForAsyncHandle(PlatformFile
platform_file);
You need to mention this takes ownership of |platform_file|.

[Reilly Grant (use Gerrit), 2015-06-02 00:43:58]

//net folks, ping?

[Reilly Grant (use Gerrit), 2015-06-08 22:23:42]

Ping again?

[howard, 2015-06-13 00:24:11]

On 2015/06/08 22:23:42, reillyg wrote:
> Ping again?

//net folks thanks so much for taking a look at this - this fix is needed by
Tidepool.org, a non-profit supporting Type 1 Diabetes research. We have 15
clinics running windows that could use this fix ASAP. Thank you!!!

[davidben, 2015-06-15 20:30:06]

davidben@chromium.org changed reviewers:
+ scottmg@chromium.org

[davidben, 2015-06-15 20:30:07]

Eric is on vacation for a while. (Sorry, I didn't notice this earlier.)

scottmg@, do you mind reviewing this? I'm not familiar enough with Windows.

[scottmg, 2015-06-15 21:42:45]

lgtm

(SetFilePointerEx can extend the file, so I guess that's probably why it fails.)

https://codereview.chromium.org/1148383003/diff/120001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1148383003/diff/120001/base/files/file.cc#new...
base/files/file.cc:70: // with FLAG_OVERLAPPED on Windows but this doesn't
appear to be possible.
nit; FILE_FLAG_OVERLAPPED (I don't know of a direct way to check either)

[Reilly Grant (use Gerrit), 2015-06-15 22:01:12]

https://codereview.chromium.org/1148383003/diff/120001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1148383003/diff/120001/base/files/file.cc#new...
base/files/file.cc:70: // with FLAG_OVERLAPPED on Windows but this doesn't
appear to be possible.
On 2015/06/15 21:42:45, scottmg wrote:
> nit; FILE_FLAG_OVERLAPPED (I don't know of a direct way to check either)

Done.

[davidben, 2015-06-15 22:04:19]

content and net lgtm. Sorry about the delay!

[scottmg, 2015-06-15 22:06:55]

https://codereview.chromium.org/1148383003/diff/140001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1148383003/diff/140001/base/files/file.cc#new...
base/files/file.cc:70: // FILE_FLAG_OVERLAPPED on Windows but this doesn't
appear to be possible.
Actually, I think NtQueryObject will return the creation flags when retrieving
OBJECT_BASIC_INFORMATION, but I'm not sure how important it is to do that here.

[Reilly Grant (use Gerrit), 2015-06-15 22:36:36]

https://codereview.chromium.org/1148383003/diff/140001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1148383003/diff/140001/base/files/file.cc#new...
base/files/file.cc:70: // FILE_FLAG_OVERLAPPED on Windows but this doesn't
appear to be possible.
On 2015/06/15 22:06:54, scottmg wrote:
> Actually, I think NtQueryObject will return the creation flags when retrieving
> OBJECT_BASIC_INFORMATION, but I'm not sure how important it is to do that
here.

Microsoft also appears to be threatening to remove that function.

[scottmg, 2015-06-15 22:42:13]

https://codereview.chromium.org/1148383003/diff/140001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1148383003/diff/140001/base/files/file.cc#new...
base/files/file.cc:70: // FILE_FLAG_OVERLAPPED on Windows but this doesn't
appear to be possible.
On 2015/06/15 22:36:36, reillyg wrote:
> On 2015/06/15 22:06:54, scottmg wrote:
> > Actually, I think NtQueryObject will return the creation flags when
retrieving
> > OBJECT_BASIC_INFORMATION, but I'm not sure how important it is to do that
> here.
> 
> Microsoft also appears to be threatening to remove that function.

Nah, it's just "internal"/"not documented", but it's never going anywhere. We
also use it quite a bit elsewhere fwiw:
https://code.google.com/p/chromium/codesearch#search/&q=ntqueryobject&sq=pack...
But I don't think the assertion is probably worth the complexity here.

[Reilly Grant (use Gerrit), 2015-06-16 00:11:19]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-06-16 00:11:20]

The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org,
tzik@chromium.org, thestig@chromium.org, scottmg@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1148383003/#ps160001
(title: "Fix Win DBG build by Pass()-ing file out of CreateForAsyncHandle.")

[commit-bot: I haz the power, 2015-06-16 00:12:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148383003/160001

[commit-bot: I haz the power, 2015-06-16 00:48:53]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-06-16 00:49:53]

Patchset 8 (id:??) landed as
https://crrev.com/aa435e70c70364430093361b5b8c460831f80bb4
Cr-Commit-Position: refs/heads/master@{#334518}