[webkit-reviews] review denied: [Bug 67684] [Chromium/FileWriter] race condition in FileWriter completion can lead to assert : [Attachment 108256] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 67684: [Chromium/FileWriter] race condition in FileWriter completion can
lead to assert
https://bugs.webkit.org/show_bug.cgi?id=67684

Attachment 108256: Patch
https://bugs.webkit.org/attachment.cgi?id=108256&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=108256&action=review


Looks close!

> LayoutTests/fast/filesystem/resources/file-writer-abort-continue.js:20
> +    onwritestart : onWriteStart0,

I think these methods like "onWriteStart0" would be better named if they
indicated what they did as opposed to when they are called.

For example "onWriteStart0" could be "abortWriter".

"onAbort0" could be "logAbort"

> LayoutTests/fast/filesystem/resources/file-writer-abort-continue.js:21
> +    onwrite : onError,

I don't see onError defined anywhere.

> LayoutTests/fast/filesystem/resources/file-writer-abort-continue.js:62
> +// the followon action.

follow on?

> LayoutTests/fast/filesystem/resources/file-writer-abort.js:58
> +    cleanUp();

where is cleanUp?

> Source/WebCore/fileapi/FileWriter.cpp:82
> +    if (writer() && m_readyState == WRITING && m_backendIsActive &&
(m_queuedOperation == QueuedOperationNone))

Odd to add parens for the == at the end when the other == doesn't have them. (I
think we avoid them in this case but if you want them, add them for
m_readyState as well.)

> Source/WebCore/fileapi/FileWriter.cpp:100
> +    if (m_recursionDepth > kMaxRecursionDepth) {

Where is the test for this?

> Source/WebCore/fileapi/FileWriter.cpp:109
> +    if (m_backendIsActive) // We must be waiting for an abort to complete.

" // We must be waiting for an abort to complete."
How do you know that? ok, I see because m_readyState != WRITING and
m_backendIsActive

> Source/WebCore/fileapi/FileWriter.cpp:112
> +	   writer()->write(position(), m_blobBeingWritten.get());

It feels ugly to have this here an in the switch statement.

Can you pull that out to one place?

I'm thinking of something like this:

ASSERT(m_readyState != WRITING);
ASSERT(m_queuedOperation == QueuedOperationNone);

if (m_backendIsActive) // We must be waiting for an abort to complete.
  m_queuedOperation = operation;
else {
  switch (operation) {
// Something similar to what you have in FileWriter::didFail right now.
  }
}

> Source/WebCore/fileapi/FileWriter.cpp:136
> +    if (m_recursionDepth > kMaxRecursionDepth) {

Where is the test for this?

> Source/WebCore/fileapi/FileWriter.cpp:-132
> -	   setError(FileError::INVALID_STATE_ERR, ec);

Is there is test that verifies this is no longer an error?

> Source/WebCore/fileapi/FileWriter.cpp:211
> +	       writer()->write(position(), m_blobBeingWritten.get());

Do I get an event from this? If so, could I do an abort and then another write
which would get messed up by the m_queuedOperation = QueuedOperationNone;
below?

> Source/WebCore/fileapi/FileWriter.cpp:219
> +	   default:

Omit the "default" to get the benefit of the compiler detecting when other
enums are added.

Shouldn't this case also be able to do ASSERT(m_readyState == WRITING);?

> Source/WebCore/fileapi/FileWriter.cpp:224
> +    } else {

ASSERT(m_queuedOperation == QueuedOperationNone); ?

> Source/WebCore/fileapi/FileWriter.cpp:234
> +    m_truncateLength = -1;

Is there a test that would fail if this were not set here?

> Source/WebCore/fileapi/FileWriter.cpp:240
> +	       fireEvent(eventNames().errorEvent);

There were no tests affected by this change? Why not?

(Does one of the added tests fail if I were to revert this part of the change?)


> Source/WebCore/fileapi/FileWriter.h:110
> +	 QueuedOperationNone,

indent further

> Source/WebCore/fileapi/FileWriter.h:118
> +    bool m_backendIsActive;

What does this mean "backendIsAction"?

I think it means "m_isOperationInProgress"?