DOMFileSystem's reference should be kept while there're any active Entries/callbacks

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

DOMFileSystem's reference should be kept while there're any pending Entries/callbacks.

Right now the reference needs to be kept in the script -- otherwise it crashes.

[Kinuko Yasuda]

Or Entries and callbacks should hold a reference to the DOMFileSystem in a proper way.

[Michael Nordman]

FYI: There is an easy workaround to the crashes, hold a script reference to the resulting fileSystem object returned as a result of the requestFileSystem() call.

[Kinuko Yasuda]

Created attachment 71455 [details]
Patch

[Eric U.]

Comment on attachment 71455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71455&action=review

> WebCore/fileapi/FileSystemCallbacks.cpp:141
> +PassOwnPtr<EntriesCallbacks> EntriesCallbacks::create(PassRefPtr<EntriesCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback, PassRefPtr<DirectoryReaderBase> directoryReader, const String& basePath)

What about EntryCallbacks?

[Kinuko Yasuda]

Created attachment 71512 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #4)
> (From update of attachment 71455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71455&action=review
> 
> > WebCore/fileapi/FileSystemCallbacks.cpp:141
> > +PassOwnPtr<EntriesCallbacks> EntriesCallbacks::create(PassRefPtr<EntriesCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback, PassRefPtr<DirectoryReaderBase> directoryReader, const String& basePath)
> 
> What about EntryCallbacks?

Forgotten... fixed.

[Eric U.]

Comment on attachment 71512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71512&action=review

LGTM

> LayoutTests/fast/filesystem/script-tests/filesystem-reference.js:8
> +    testFailed("Error occured: " + error.code);

typo: occurred

[Kinuko Yasuda]

Created attachment 71966 [details]
Patch

Fixed the typo.

[Kinuko Yasuda]

Adding 'real' reviewers... could someone take a look at it?

[Dumitru Daniliuc]

Comment on attachment 71966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71966&action=review

r=me, but please address my comments before submitting.

> LayoutTests/fast/filesystem/script-tests/filesystem-reference.js:44
> +function runTest1(root)

nit/optional: i'd make runTest2() come after runTest1(), and runTest3() come after runTest2(). this way, it's easier (i think) to read the test from top to bottom.

> WebCore/fileapi/DOMFileSystemBase.cpp:38
> +#include "DirectoryReaderBase.h"

why do we need this #include? Changing DirectoryReaderBase* to PassRefPtr<DirectoryReaderBase> shouldn't require any additional #includes (and the list of #includes in DOMFileSystemBase.h didn't change either).

> WebCore/fileapi/DirectoryEntry.cpp:51
> +    return DirectoryReader::create(filesystem(), m_fullPath);

i think we should be consistent in how we use the fields declared in base classes (EntryBase): either keep them protected, and use m_fileSystem.get() and m_fullPath here, or we should make them private and add public/protected getters and use filesystem() and fullPath() here.

> WebCore/fileapi/DirectoryEntrySync.cpp:51
> +    return DirectoryReaderSync::create(filesystem(), m_fullPath);

same comment: let's make this consistent; either use the members directly, or provide getters for all of them.

> WebCore/fileapi/EntrySync.cpp:50
> +        return adoptRef(new FileEntrySync(entry->filesystem(), entry->m_fullPath));

ditto.

> WebCore/fileapi/EntrySync.cpp:102
> +    return DirectoryEntrySync::create(filesystem(), parentPath);

ditto.

[Kinuko Yasuda]

Committed r70741: <http://trac.webkit.org/changeset/70741>