Changeset 96177 in webkit


Ignore:
Timestamp:
Sep 27, 2011 6:46:08 PM (13 years ago)
Author:
ericu@chromium.org
Message:

[Chromium/FileWriter] race condition in FileWriter completion can lead to assert
​https://bugs.webkit.org/show_bug.cgi?id=67684

Reviewed by David Levin.

Source/WebCore:

Tests: fast/filesystem/file-writer-abort-continue.html

fast/filesystem/file-writer-abort.html

Track the state of the backend and be prepared for reentrant user
requests. Limit recursion depth to an arbitrary small constant.

  • fileapi/FileWriter.cpp: Lots of event-handling changes.
  • fileapi/FileWriter.h:

LayoutTests:

  • fast/filesystem/file-writer-abort-continue-expected.txt: Added.
  • fast/filesystem/file-writer-abort-continue.html: Added.
  • fast/filesystem/file-writer-abort-expected.txt: Added.
  • fast/filesystem/file-writer-abort.html: Added.
  • fast/filesystem/resources/file-writer-abort-continue.js: Added.
  • fast/filesystem/resources/file-writer-abort.js: Added.
  • fast/filesystem/resources/file-writer-events.js: Fixed a copy-paste error.
Location:
trunk
Files:
8 added
5 edited
1 copied

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r96176 r96177  
     12011-09-27  Eric Uhrhane  <ericu@chromium.org>
     2
     3        [Chromium/FileWriter] race condition in FileWriter completion can lead to assert
     4        https://bugs.webkit.org/show_bug.cgi?id=67684
     5
     6        Reviewed by David Levin.
     7
     8        * fast/filesystem/file-writer-abort-continue-expected.txt: Added.
     9        * fast/filesystem/file-writer-abort-continue.html: Added.
     10        * fast/filesystem/file-writer-abort-expected.txt: Added.
     11        * fast/filesystem/file-writer-abort.html: Added.
     12        * fast/filesystem/resources/file-writer-abort-continue.js: Added.
     13        * fast/filesystem/resources/file-writer-abort.js: Added.
     14        * fast/filesystem/resources/file-writer-events.js: Fixed a copy-paste error.
     15
    1162011-09-27  Tim Horton  <timothy_horton@apple.com>
    217

  • trunk/LayoutTests/fast/filesystem/resources/file-writer-abort.js

    r96176 r96177  
    44}
    55
    6 description("Test that FileWriter produces proper progress events.");
     6description("Test that FileWriter handles abort properly.");
    77
    8 var fileEntry;
    98var sawWriteStart;
    10 var sawWrite;
     9var sawAbort;
    1110var sawWriteEnd;
    12 var sawProgress;
    1311var writer;
    14 var lastProgress = 0;
    15 var toBeWritten;
    1612
    1713function tenXBlob(blob) {
    … …  
    2824    assert(e.type == "writestart");
    2925    assert(!sawWriteStart);
    30     assert(!sawProgress);
    31     assert(!sawWrite);
    3226    assert(!sawWriteEnd);
    3327    assert(!e.loaded);
    34     assert(e.total == toBeWritten);
    3528    sawWriteStart = true;
     29    testPassed("Calling abort");
     30    writer.abort();
    3631}
    3732
    38 function onProgress(e) {
    39     assert(writer.readyState == writer.WRITING);
    40     assert(sawWriteStart);
    41     assert(!sawWrite);
    42     assert(!sawWriteEnd);
    43     assert(e.type == "progress");
    44     assert(e.loaded <= e.total);
    45     assert(lastProgress < e.loaded);
    46     assert(e.total == toBeWritten);
    47     lastProgress = e.loaded;
    48     sawProgress = true;
     33// We should always abort before completion.
     34function onWrite(e) {
     35    testFailed("In onWrite.");
    4936}
    5037
    51 function onWrite(e) {
     38function onAbort(e) {
    5239    assert(writer.readyState == writer.DONE);
     40    assert(writer.error.code == writer.error.ABORT_ERR);
    5341    assert(sawWriteStart);
    54     assert(sawProgress);
    55     assert(lastProgress == e.total);
    56     assert(!sawWrite);
    5742    assert(!sawWriteEnd);
    58     assert(e.type == "write");
    59     assert(e.loaded == e.total);
    60     assert(e.total == toBeWritten);
    61     sawWrite = true;
     43    assert(!sawAbort);
     44    assert(e.type == "abort");
     45    sawAbort = true;
     46    testPassed("Saw abort");
    6247}
    6348
    6449function onWriteEnd(e) {
    6550    assert(writer.readyState == writer.DONE);
     51    assert(writer.error.code == writer.error.ABORT_ERR);
    6652    assert(sawWriteStart);
    67     assert(sawProgress);
    68     assert(sawWrite);
     53    assert(sawAbort);
    6954    assert(!sawWriteEnd);
    7055    assert(e.type == "writeend");
    71     assert(e.loaded == e.total);
    72     assert(e.total == toBeWritten);
    7356    sawWriteEnd = true;
    74     testPassed("Saw all the right events.");
     57    testPassed("Saw writeend.");
     58    writer.abort();  // Verify that this does nothing in readyState DONE.
    7559    cleanUp();
    7660}
    … …  
    8569    blob = tenXBlob(blob);
    8670    blob = tenXBlob(blob);
    87     toBeWritten = blob.size;
    8871    writer = fileWriter;
    8972    fileWriter.onerror = onError;
     73    fileWriter.onabort = onAbort;
    9074    fileWriter.onwritestart = onWriteStart;
    91     fileWriter.onprogress = onProgress;
    9275    fileWriter.onwrite = onWrite;
    9376    fileWriter.onwriteend = onWriteEnd;
     77    fileWriter.abort();  // Verify that this does nothing in readyState INIT.
    9478    fileWriter.write(blob);
    9579}
    … …  
    9983}
    10084var jsTestIsAsync = true;
    101 setupAndRunTest(2*1024*1024, 'file-writer-gc-blob', runTest);
     85setupAndRunTest(2*1024*1024, 'file-writer-abort', runTest);
    10286var successfullyParsed = true;

  • trunk/LayoutTests/fast/filesystem/resources/file-writer-events.js

    r85556 r96177  
    9999}
    100100var jsTestIsAsync = true;
    101 setupAndRunTest(2*1024*1024, 'file-writer-gc-blob', runTest);
     101setupAndRunTest(2*1024*1024, 'file-writer-events', runTest);
    102102var successfullyParsed = true;

  • trunk/Source/WebCore/ChangeLog

    r96173 r96177  
     12011-09-27  Eric Uhrhane  <ericu@chromium.org>
     2
     3        [Chromium/FileWriter] race condition in FileWriter completion can lead to assert
     4        https://bugs.webkit.org/show_bug.cgi?id=67684
     5
     6        Reviewed by David Levin.
     7
     8        Tests: fast/filesystem/file-writer-abort-continue.html
     9               fast/filesystem/file-writer-abort.html
     10
     11        Track the state of the backend and be prepared for reentrant user
     12        requests.  Limit recursion depth to an arbitrary small constant.
     13        * fileapi/FileWriter.cpp: Lots of event-handling changes.
     14        * fileapi/FileWriter.h:
     15
    1162011-09-27  Mihai Parparita  <mihaip@chromium.org>
    217

  • trunk/Source/WebCore/fileapi/FileWriter.cpp

    r95901 r96177  
    4444namespace WebCore {
    4545
     46static const int kMaxRecursionDepth = 3;
     47
    4648FileWriter::FileWriter(ScriptExecutionContext* context)
    4749    : ActiveDOMObject(context, this)
    4850    , m_readyState(INIT)
    49     , m_startedWriting(false)
     51    , m_isOperationInProgress(false)
     52    , m_queuedOperation(OperationNone)
    5053    , m_bytesWritten(0)
    5154    , m_bytesToWrite(0)
    5255    , m_truncateLength(-1)
     56    , m_numAborts(0)
     57    , m_recursionDepth(0)
    5358{
    5459}
    … …  
    5661FileWriter::~FileWriter()
    5762{
     63    ASSERT(!m_recursionDepth);
    5864    if (m_readyState == WRITING)
    5965        stop();
    … …  
    7379void FileWriter::stop()
    7480{
    75     if (writer() && m_readyState == WRITING)
     81    // Make sure we've actually got something to stop, and haven't already called abort().
     82    if (writer() && m_readyState == WRITING && m_isOperationInProgress && m_queuedOperation == OperationNone)
    7683        writer()->abort();
    7784    m_blobBeingWritten.clear();
    … …  
    8289{
    8390    ASSERT(writer());
     91    ASSERT(data);
     92    ASSERT(m_truncateLength == -1);
    8493    if (m_readyState == WRITING) {
    8594        setError(FileError::INVALID_STATE_ERR, ec);
    … …  
    9099        return;
    91100    }
    92 
     101    if (m_recursionDepth > kMaxRecursionDepth) {
     102        setError(FileError::SECURITY_ERR, ec);
     103        return;
     104    }
    93105    m_blobBeingWritten = data;
    94106    m_readyState = WRITING;
    95     m_startedWriting = false;
    96107    m_bytesWritten = 0;
    97108    m_bytesToWrite = data->size();
    98     writer()->write(position(), data);
     109    ASSERT(m_queuedOperation == OperationNone);
     110    if (m_isOperationInProgress) // We must be waiting for an abort to complete, since m_readyState wasn't WRITING.
     111        m_queuedOperation = OperationWrite;
     112    else
     113        writer()->write(position(), m_blobBeingWritten.get());
     114    m_isOperationInProgress = true;
     115    fireEvent(eventNames().writestartEvent);
    99116}
    100117
    … …  
    107124    }
    108125
    109     m_bytesWritten = 0;
    110     m_bytesToWrite = 0;
     126    ASSERT(m_truncateLength == -1);
     127    ASSERT(m_queuedOperation == OperationNone);
    111128    seekInternal(position);
    112129}
    … …  
    115132{
    116133    ASSERT(writer());
     134    ASSERT(m_truncateLength == -1);
    117135    if (m_readyState == WRITING || position < 0) {
    118136        setError(FileError::INVALID_STATE_ERR, ec);
     137        return;
     138    }
     139    if (m_recursionDepth > kMaxRecursionDepth) {
     140        setError(FileError::SECURITY_ERR, ec);
    119141        return;
    120142    }
    … …  
    123145    m_bytesToWrite = 0;
    124146    m_truncateLength = position;
    125     writer()->truncate(position);
     147    ASSERT(m_queuedOperation == OperationNone);
     148    if (m_isOperationInProgress) // We must be waiting for an abort to complete.
     149        m_queuedOperation = OperationTruncate;
     150    else
     151        writer()->truncate(m_truncateLength);
     152    m_isOperationInProgress = true;
     153    fireEvent(eventNames().writestartEvent);
    126154}
    127155
    … …  
    130158    ASSERT(writer());
    131159    if (m_readyState != WRITING) {
    132         setError(FileError::INVALID_STATE_ERR, ec);
    133         return;
    134     }
    135 
    136     writer()->abort();
     160        return;
     161    }
     162    ++m_numAborts;
     163
     164    if (m_isOperationInProgress)
     165        writer()->abort();
     166    m_queuedOperation = OperationNone;
     167    m_blobBeingWritten.clear();
     168    m_truncateLength = -1;
     169    signalCompletion(FileError::ABORT_ERR);
    137170}
    138171
    139172void FileWriter::didWrite(long long bytes, bool complete)
    140173{
     174    ASSERT(m_readyState == WRITING);
     175    ASSERT(m_truncateLength == -1);
     176    ASSERT(m_isOperationInProgress);
    141177    ASSERT(bytes + m_bytesWritten > 0);
    142178    ASSERT(bytes + m_bytesWritten <= m_bytesToWrite);
    143     if (!m_startedWriting) {
    144         fireEvent(eventNames().writestartEvent);
    145         m_startedWriting = true;
    146     }
    147179    m_bytesWritten += bytes;
    148180    ASSERT((m_bytesWritten == m_bytesToWrite) || !complete);
    … …  
    150182    if (position() > length())
    151183        setLength(position());
     184    // TODO: Throttle to no more frequently than every 50ms.
     185    int numAborts = m_numAborts;
    152186    fireEvent(eventNames().progressEvent);
    153     if (complete) {
     187    // We could get an abort in the handler for this event. If we do, it's
     188    // already handled the cleanup and signalCompletion call.
     189    if (complete && numAborts == m_numAborts) {
    154190        m_blobBeingWritten.clear();
    155         scriptExecutionContext()->postTask(FileWriterCompletionEventTask::create(this, FileError::OK));
     191        m_isOperationInProgress = false;
     192        signalCompletion(FileError::OK);
    156193    }
    157194}
    … …  
    160197{
    161198    ASSERT(m_truncateLength >= 0);
    162     fireEvent(eventNames().writestartEvent);
    163199    setLength(m_truncateLength);
    164200    if (position() > length())
    165201        setPosition(length());
     202    m_isOperationInProgress = false;
     203    signalCompletion(FileError::OK);
     204}
     205
     206void FileWriter::didFail(FileError::ErrorCode code)
     207{
     208    ASSERT(m_isOperationInProgress);
     209    m_isOperationInProgress = false;
     210    ASSERT(code != FileError::OK);
     211    if (code == FileError::ABORT_ERR) {
     212        Operation operation = m_queuedOperation;
     213        m_queuedOperation = OperationNone;
     214        doOperation(operation);
     215    } else {
     216        ASSERT(m_queuedOperation == OperationNone);
     217        ASSERT(m_readyState == WRITING);
     218        m_blobBeingWritten.clear();
     219        signalCompletion(code);
     220    }
     221}
     222
     223void FileWriter::doOperation(Operation operation)
     224{
     225    ASSERT(m_queuedOperation == OperationNone);
     226    ASSERT(!m_isOperationInProgress);
     227    ASSERT(operation == OperationNone || operation == OperationWrite || operation == OperationTruncate);
     228    switch (operation) {
     229    case OperationWrite:
     230        ASSERT(m_truncateLength == -1);
     231        ASSERT(m_blobBeingWritten.get());
     232        ASSERT(m_readyState == WRITING);
     233        writer()->write(position(), m_blobBeingWritten.get());
     234        m_isOperationInProgress = true;
     235        break;
     236    case OperationTruncate:
     237        ASSERT(m_truncateLength >= 0);
     238        ASSERT(m_readyState == WRITING);
     239        writer()->truncate(m_truncateLength);
     240        m_isOperationInProgress = true;
     241        break;
     242    case OperationNone:
     243        ASSERT(m_truncateLength == -1);
     244        ASSERT(!m_blobBeingWritten.get());
     245        ASSERT(m_readyState == DONE);
     246        break;
     247    }
     248}
     249
     250void FileWriter::signalCompletion(FileError::ErrorCode code)
     251{
     252    m_readyState = DONE;
    166253    m_truncateLength = -1;
    167     scriptExecutionContext()->postTask(FileWriterCompletionEventTask::create(this, FileError::OK));
    168 }
    169 
    170 void FileWriter::didFail(FileError::ErrorCode code)
    171 {
    172     ASSERT(code != FileError::OK);
    173     m_blobBeingWritten.clear();
    174     scriptExecutionContext()->postTask(FileWriterCompletionEventTask::create(this, code));
    175 }
    176 
    177 void FileWriter::signalCompletion(FileError::ErrorCode code)
    178 {
    179     m_readyState = DONE;
    180254    if (FileError::OK != code) {
    181255        m_error = FileError::create(code);
    182         fireEvent(eventNames().errorEvent);
    183256        if (FileError::ABORT_ERR == code)
    184257            fireEvent(eventNames().abortEvent);
     258        else
     259            fireEvent(eventNames().errorEvent);
    185260    } else
    186261        fireEvent(eventNames().writeEvent);
    … …  
    190265void FileWriter::fireEvent(const AtomicString& type)
    191266{
     267    ++m_recursionDepth;
    192268    dispatchEvent(ProgressEvent::create(type, true, m_bytesWritten, m_bytesToWrite));
     269    --m_recursionDepth;
     270    ASSERT(m_recursionDepth >= 0);
    193271}
    194272

  • trunk/Source/WebCore/fileapi/FileWriter.h

    r95901 r96177  
    9191
    9292private:
     93    enum Operation {
     94        OperationNone,
     95        OperationWrite,
     96        OperationTruncate
     97    };
     98
    9399    FileWriter(ScriptExecutionContext*);
    94100
    … …  
    101107    virtual EventTargetData* ensureEventTargetData() { return &m_eventTargetData; }
    102108
     109    void doOperation(Operation);
     110
     111    void signalCompletion(FileError::ErrorCode);
     112
    103113    void fireEvent(const AtomicString& type);
    104114
    105115    void setError(FileError::ErrorCode, ExceptionCode&);
    106116
    107     void signalCompletion(FileError::ErrorCode);
    108 
    109     class FileWriterCompletionEventTask : public ScriptExecutionContext::Task {
    110     public:
    111         static PassOwnPtr<FileWriterCompletionEventTask> create(PassRefPtr<FileWriter> fileWriter, FileError::ErrorCode code)
    112         {
    113             return adoptPtr(new FileWriterCompletionEventTask(fileWriter, code));
    114         }
    115 
    116 
    117         virtual void performTask(ScriptExecutionContext*)
    118         {
    119             m_fileWriter->signalCompletion(m_code);
    120         }
    121     private:
    122         FileWriterCompletionEventTask(PassRefPtr<FileWriter> fileWriter, FileError::ErrorCode code)
    123             : m_fileWriter(fileWriter)
    124             , m_code(code)
    125         {
    126         }
    127 
    128         RefPtr<FileWriter> m_fileWriter;
    129         FileError::ErrorCode m_code;
    130     };
    131 
    132117    RefPtr<FileError> m_error;
    133118    EventTargetData m_eventTargetData;
    134119    ReadyState m_readyState;
    135     bool m_startedWriting;
     120    bool m_isOperationInProgress;
     121    Operation m_queuedOperation;
    136122    long long m_bytesWritten;
    137123    long long m_bytesToWrite;
    138124    long long m_truncateLength;
     125    long long m_numAborts;
     126    long long m_recursionDepth;
    139127    RefPtr<Blob> m_blobBeingWritten;
    140128};
Note: See TracChangeset for help on using the changeset viewer.