Bug 217333 – FileReader should transition to readyState DONE after last onprogress event

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

FileReader should transition to readyState DONE after last onprogress event

[Alex Christensen]

Created attachment 410548 [details]
Patch

[Alex Christensen]

Created attachment 410555 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

(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

[Alex Christensen]

Created attachment 410567 [details]
Patch

[EWS Watchlist]

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

[Chris Dumez]

EWS seems unhappy.

[Alex Christensen]

Created attachment 410589 [details]
Patch

[youenn fablet]

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.

[Alex Christensen]

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")

[Alex Christensen]

Created attachment 410640 [details]
Patch

[youenn fablet]

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.

[Chris Dumez]

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?

[Alex Christensen]

Created attachment 410644 [details]
Patch

[EWS]

Committed r268054: <https://trac.webkit.org/changeset/268054>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410644 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70004095>