FileReader should transition to readyState DONE after last onprogress event
Created attachment 410548 [details] Patch
Created attachment 410555 [details] Patch
Comment on attachment 410555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410555&action=review > Source/WebCore/ChangeLog:8 > + This matches the behavior of Chrome and Firefox. This does not quite match Chrome I think. In particular, Chrome has a separate m_loadingState flag so that if abort() gets called in the progress event, it is a no-op. With your change, I do not believe it would be a no-op.
(In reply to Chris Dumez from comment #3) > Comment on attachment 410555 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410555&action=review > > > Source/WebCore/ChangeLog:8 > > + This matches the behavior of Chrome and Firefox. > > This does not quite match Chrome I think. In particular, Chrome has a > separate m_loadingState flag so that if abort() gets called in the progress > event, it is a no-op. With your change, I do not believe it would be a no-op. You can see the Chromium logic is slightly different here: https://github.com/chromium/chromium/blob/master/third_party/blink/renderer/core/fileapi/file_reader.cc
Created attachment 410567 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
EWS seems unhappy.
Created attachment 410589 [details] Patch
Comment on attachment 410589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410589&action=review > Source/WebCore/ChangeLog:8 > + I also bring the abort function in alignment with https://www.w3.org/TR/FileAPI/#abort Maybe point to the latest spec: https://w3c.github.io/FileAPI/#abort > Source/WebCore/fileapi/FileReader.cpp:157 > return; It seems the spec is firing the abort event even in case m_state is not loading. Can you clarify? > Source/WebCore/fileapi/FileReader.cpp:159 > // Schedule to have the abort done later since abort() might be called from the event handler and we do not want the resource loading code to be in the stack. This comment should be removed, we no longer schedule a task. The spec tells us to not enqueue a task to fire an event. Do we have coverage for that? We usually try to always enqueue a task to fire events. A test like the following might be good to add: let flag = true; reader.onabort = () => assert_true(flag); reader.abort(); flag = false; > Source/WebCore/fileapi/FileReader.cpp:167 > + fireEvent(eventNames().abortEvent); Previously, enqueueTask was protecting us from being collected. We should probably now protect ourselves since JS might free the FileReader object. > Source/WebCore/fileapi/FileReader.cpp:195 > enqueueTask([this] { If didFinishLoading is called, then abort is called before this enqueued task is executed, we might have some issues. It seems we could hit ASSERT(m_state != DONE); > Source/WebCore/fileapi/FileReader.cpp:207 > enqueueTask([this, errorCode] { Ditto here for aborting just after didFail enqueued this task.
Comment on attachment 410589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410589&action=review >> Source/WebCore/fileapi/FileReader.cpp:159 >> // Schedule to have the abort done later since abort() might be called from the event handler and we do not want the resource loading code to be in the stack. > > This comment should be removed, we no longer schedule a task. > The spec tells us to not enqueue a task to fire an event. Do we have coverage for that? We usually try to always enqueue a task to fire events. > A test like the following might be good to add: > > let flag = true; > reader.onabort = () => assert_true(flag); > reader.abort(); > flag = false; fileReader.html already covered this. That's how I discovered that Chrome and Firefox fire synchronously. fileReader.abort(); fileReader.onabort = this.unreached_func("abort event should fire sync")
Created attachment 410640 [details] Patch
Comment on attachment 410640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410640&action=review > Source/WebCore/ChangeLog:8 > + I also bring the abort function in alignment with https://www.w3.org/FileAPI/#abort Let's replace with https://w3c.github.io/FileAPI/. > Source/WebCore/fileapi/FileReader.cpp:160 > + ASSERT(m_state != DONE); ASSERT not needed anymore > Source/WebCore/fileapi/FileReader.cpp:168 > + fireEvent(eventNames().loadendEvent); The spec says: If context object's state is not "loading", fire a progress event called loadend at the context object. I do not really see how that could happen in practice. Maybe the spec could be simplified.
Comment on attachment 410640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410640&action=review >> Source/WebCore/fileapi/FileReader.cpp:168 >> + fireEvent(eventNames().loadendEvent); > > The spec says: > If context object's state is not "loading", fire a progress event called loadend at the context object. > I do not really see how that could happen in practice. Maybe the spec could be simplified. Cannot the JS start loading again by reading again from its abort event listener?
Created attachment 410644 [details] Patch
Committed r268054: <https://trac.webkit.org/changeset/268054> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410644 [details].
<rdar://problem/70004095>