Title: [112445] trunk/Source/WebCore
- Revision
- 112445
- Author
- [email protected]
- Date
- 2012-03-28 14:43:10 -0700 (Wed, 28 Mar 2012)
Log Message
FileWriter has two race conditions
https://bugs.webkit.org/show_bug.cgi?id=81861
Reviewed by David Levin.
Should make current tests less flaky.
* Modules/filesystem/FileWriter.h:
* Modules/filesystem/FileWriter.cpp:
Track the in-flight operation, whether it be an abort/write/truncate.
Whether an abort comes back as didWrite, didTruncate, or didFail, handle
it appropriately. Before this fix, the Chromium implementation would
assert in two cases:
If the user calls abort, then write, then abort before the backend
catches up, we'd send both aborts to the backend, even though it hadn't
received the write yet. Chromium's backend asserts if there's an abort
with no write in progress. We now record that we've sent an abort and
are waiting for the response.
If the user calls abort while a write/truncate is just finishing, on the
Chromium worker implementation, the completion message could be
thread-hopping back to WebCore at the
WorkerAsyncFileWriterCallbacksBridge while the abort is thread-hopping
in the other direction. Again, this leads to an abort call to the
backend with no write in progress, and an assert. We're now robust to
completions coming back when we're expecting an abort, and
https://chromiumcodereview.appspot.com/9764018/ will make the backend
robust to extra abort calls.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (112444 => 112445)
--- trunk/Source/WebCore/ChangeLog 2012-03-28 21:41:34 UTC (rev 112444)
+++ trunk/Source/WebCore/ChangeLog 2012-03-28 21:43:10 UTC (rev 112445)
@@ -1,3 +1,35 @@
+2012-03-26 Eric Uhrhane <[email protected]>
+
+ FileWriter has two race conditions
+ https://bugs.webkit.org/show_bug.cgi?id=81861
+
+ Reviewed by David Levin.
+
+ Should make current tests less flaky.
+
+ * Modules/filesystem/FileWriter.h:
+ * Modules/filesystem/FileWriter.cpp:
+ Track the in-flight operation, whether it be an abort/write/truncate.
+ Whether an abort comes back as didWrite, didTruncate, or didFail, handle
+ it appropriately. Before this fix, the Chromium implementation would
+ assert in two cases:
+
+ If the user calls abort, then write, then abort before the backend
+ catches up, we'd send both aborts to the backend, even though it hadn't
+ received the write yet. Chromium's backend asserts if there's an abort
+ with no write in progress. We now record that we've sent an abort and
+ are waiting for the response.
+
+ If the user calls abort while a write/truncate is just finishing, on the
+ Chromium worker implementation, the completion message could be
+ thread-hopping back to WebCore at the
+ WorkerAsyncFileWriterCallbacksBridge while the abort is thread-hopping
+ in the other direction. Again, this leads to an abort call to the
+ backend with no write in progress, and an assert. We're now robust to
+ completions coming back when we're expecting an abort, and
+ https://chromiumcodereview.appspot.com/9764018/ will make the backend
+ robust to extra abort calls.
+
2012-03-27 Ryosuke Niwa <[email protected]>
Deleting a paragraph of text should not add elements for typing style
Modified: trunk/Source/WebCore/Modules/filesystem/FileWriter.cpp (112444 => 112445)
--- trunk/Source/WebCore/Modules/filesystem/FileWriter.cpp 2012-03-28 21:41:34 UTC (rev 112444)
+++ trunk/Source/WebCore/Modules/filesystem/FileWriter.cpp 2012-03-28 21:43:10 UTC (rev 112445)
@@ -55,7 +55,7 @@
FileWriter::FileWriter(ScriptExecutionContext* context)
: ActiveDOMObject(context, this)
, m_readyState(INIT)
- , m_isOperationInProgress(false)
+ , m_operationInProgress(OperationNone)
, m_queuedOperation(OperationNone)
, m_bytesWritten(0)
, m_bytesToWrite(0)
@@ -91,9 +91,9 @@
void FileWriter::stop()
{
// Make sure we've actually got something to stop, and haven't already called abort().
- if (writer() && m_readyState == WRITING && m_isOperationInProgress && m_queuedOperation == OperationNone)
- writer()->abort();
- m_blobBeingWritten.clear();
+ if (!writer() || m_readyState != WRITING)
+ return;
+ doOperation(OperationAbort);
m_readyState = DONE;
}
@@ -119,11 +119,13 @@
m_bytesWritten = 0;
m_bytesToWrite = data->size();
ASSERT(m_queuedOperation == OperationNone);
- if (m_isOperationInProgress) // We must be waiting for an abort to complete, since m_readyState wasn't WRITING.
+ if (m_operationInProgress != OperationNone) {
+ // We must be waiting for an abort to complete, since m_readyState wasn't WRITING.
+ ASSERT(m_operationInProgress == OperationAbort);
m_queuedOperation = OperationWrite;
- else
- writer()->write(position(), m_blobBeingWritten.get());
- m_isOperationInProgress = true;
+ } else
+ doOperation(OperationWrite);
+
fireEvent(eventNames().writestartEvent);
}
@@ -157,11 +159,12 @@
m_bytesToWrite = 0;
m_truncateLength = position;
ASSERT(m_queuedOperation == OperationNone);
- if (m_isOperationInProgress) // We must be waiting for an abort to complete.
+ if (m_operationInProgress != OperationNone) {
+ // We must be waiting for an abort to complete, since m_readyState wasn't WRITING.
+ ASSERT(m_operationInProgress == OperationAbort);
m_queuedOperation = OperationTruncate;
- else
- writer()->truncate(m_truncateLength);
- m_isOperationInProgress = true;
+ } else
+ doOperation(OperationTruncate);
fireEvent(eventNames().writestartEvent);
}
@@ -172,19 +175,19 @@
return;
++m_numAborts;
- if (m_isOperationInProgress)
- writer()->abort();
- m_queuedOperation = OperationNone;
- m_blobBeingWritten.clear();
- m_truncateLength = -1;
+ doOperation(OperationAbort);
signalCompletion(FileError::ABORT_ERR);
}
void FileWriter::didWrite(long long bytes, bool complete)
{
+ if (m_operationInProgress == OperationAbort) {
+ completeAbort();
+ return;
+ }
ASSERT(m_readyState == WRITING);
ASSERT(m_truncateLength == -1);
- ASSERT(m_isOperationInProgress);
+ ASSERT(m_operationInProgress == OperationWrite);
ASSERT(bytes + m_bytesWritten > 0);
ASSERT(bytes + m_bytesWritten <= m_bytesToWrite);
m_bytesWritten += bytes;
@@ -199,63 +202,83 @@
// already handled the cleanup and signalCompletion call.
if (complete && numAborts == m_numAborts) {
m_blobBeingWritten.clear();
- m_isOperationInProgress = false;
+ m_operationInProgress = OperationNone;
signalCompletion(FileError::OK);
}
}
void FileWriter::didTruncate()
{
+ if (m_operationInProgress == OperationAbort) {
+ completeAbort();
+ return;
+ }
+ ASSERT(m_operationInProgress == OperationTruncate);
ASSERT(m_truncateLength >= 0);
setLength(m_truncateLength);
if (position() > length())
setPosition(length());
- m_isOperationInProgress = false;
+ m_operationInProgress = OperationNone;
signalCompletion(FileError::OK);
}
void FileWriter::didFail(FileError::ErrorCode code)
{
- ASSERT(m_isOperationInProgress);
- m_isOperationInProgress = false;
+ ASSERT(m_operationInProgress != OperationNone);
ASSERT(code != FileError::OK);
- if (code == FileError::ABORT_ERR) {
- Operation operation = m_queuedOperation;
- m_queuedOperation = OperationNone;
- doOperation(operation);
- } else {
- ASSERT(m_queuedOperation == OperationNone);
- ASSERT(m_readyState == WRITING);
- m_blobBeingWritten.clear();
- signalCompletion(code);
+ if (m_operationInProgress == OperationAbort) {
+ completeAbort();
+ return;
}
+ ASSERT(code != FileError::ABORT_ERR);
+ ASSERT(m_queuedOperation == OperationNone);
+ ASSERT(m_readyState == WRITING);
+ m_blobBeingWritten.clear();
+ m_operationInProgress = OperationNone;
+ signalCompletion(code);
}
+void FileWriter::completeAbort()
+{
+ ASSERT(m_operationInProgress == OperationAbort);
+ m_operationInProgress = OperationNone;
+ Operation operation = m_queuedOperation;
+ m_queuedOperation = OperationNone;
+ doOperation(operation);
+}
+
void FileWriter::doOperation(Operation operation)
{
- ASSERT(m_queuedOperation == OperationNone);
- ASSERT(!m_isOperationInProgress);
- ASSERT(operation == OperationNone || operation == OperationWrite || operation == OperationTruncate);
switch (operation) {
case OperationWrite:
+ ASSERT(m_operationInProgress == OperationNone);
ASSERT(m_truncateLength == -1);
ASSERT(m_blobBeingWritten.get());
ASSERT(m_readyState == WRITING);
writer()->write(position(), m_blobBeingWritten.get());
- m_isOperationInProgress = true;
break;
case OperationTruncate:
+ ASSERT(m_operationInProgress == OperationNone);
ASSERT(m_truncateLength >= 0);
ASSERT(m_readyState == WRITING);
writer()->truncate(m_truncateLength);
- m_isOperationInProgress = true;
break;
case OperationNone:
+ ASSERT(m_operationInProgress == OperationNone);
ASSERT(m_truncateLength == -1);
ASSERT(!m_blobBeingWritten.get());
ASSERT(m_readyState == DONE);
break;
+ case OperationAbort:
+ if (m_operationInProgress == OperationWrite || m_operationInProgress == OperationTruncate)
+ writer()->abort();
+ m_queuedOperation = OperationNone;
+ m_blobBeingWritten.clear();
+ m_truncateLength = -1;
+ break;
}
+ ASSERT(m_queuedOperation == OperationNone);
+ m_operationInProgress = operation;
}
void FileWriter::signalCompletion(FileError::ErrorCode code)
Modified: trunk/Source/WebCore/Modules/filesystem/FileWriter.h (112444 => 112445)
--- trunk/Source/WebCore/Modules/filesystem/FileWriter.h 2012-03-28 21:41:34 UTC (rev 112444)
+++ trunk/Source/WebCore/Modules/filesystem/FileWriter.h 2012-03-28 21:43:10 UTC (rev 112445)
@@ -91,7 +91,8 @@
enum Operation {
OperationNone,
OperationWrite,
- OperationTruncate
+ OperationTruncate,
+ OperationAbort
};
FileWriter(ScriptExecutionContext*);
@@ -104,6 +105,8 @@
virtual EventTargetData* eventTargetData() { return &m_eventTargetData; }
virtual EventTargetData* ensureEventTargetData() { return &m_eventTargetData; }
+ void completeAbort();
+
void doOperation(Operation);
void signalCompletion(FileError::ErrorCode);
@@ -115,7 +118,7 @@
RefPtr<FileError> m_error;
EventTargetData m_eventTargetData;
ReadyState m_readyState;
- bool m_isOperationInProgress;
+ Operation m_operationInProgress;
Operation m_queuedOperation;
long long m_bytesWritten;
long long m_bytesToWrite;
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
