Bug 45410 – Decouple Blobs from ScriptExecutionContext.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Nordman]

There's no need to have this coupling. It makes it awkward to create instances, invites "layering violations", and present problems with passing Blob references from context to context.

[Michael Nordman]

Created attachment 66930 [details]
decouple

[Jian Li]

Comment on attachment 66930 [details]
decouple

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

> WebCore/ChangeLog:6
> +        Decouple Blob from ScriptExecutionContext.
Normally we put the description line before the bug link line.

> WebCore/ChangeLog:8
> +        * Removes ScriptExecutionContext's collection of Blobs instance in that context.
Removes => Removed

> WebCore/ChangeLog:14
> +        No new tests. (OOPS!)
Please remove this line.

> WebCore/bindings/js/SerializedScriptValue.cpp:957
> +        file = File::create(String(path.ustring().impl()), KURL(KURL(), String(url.ustring().impl())), String(type.ustring().impl()));
This file is updated. Please merge and resolve.

> WebCore/fileapi/FileReader.cpp:184
> +    ThreadableBlobRegistry::unregisterBlobURL(readableURL);
I think we should unregister this temporary URL after we finish the reading or encounter an error. This is because otherwise we have to assume that the un-registration always happens after the loading. This might be risky depending on how ThreadableLoader is implemented. For example, suppose we invoke FileReader in workers. If ThreadableLoader implementation needs to call into main thread once and then call other method asynchronously, we will have a problem.

[Michael Nordman]

(In reply to comment #2)
> (From update of attachment 66930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66930&action=prettypatch
> 
> > WebCore/ChangeLog:6
> > +        Decouple Blob from ScriptExecutionContext.
> Normally we put the description line before the bug link line.
> 
> > WebCore/ChangeLog:8
> > +        * Removes ScriptExecutionContext's collection of Blobs instance in that context.
> Removes => Removed
> 
> > WebCore/ChangeLog:14
> > +        No new tests. (OOPS!)
> Please remove this line.
> 
> > WebCore/bindings/js/SerializedScriptValue.cpp:957
> > +        file = File::create(String(path.ustring().impl()), KURL(KURL(), String(url.ustring().impl())), String(type.ustring().impl()));
> This file is updated. Please merge and resolve.
> 
> > WebCore/fileapi/FileReader.cpp:184
> > +    ThreadableBlobRegistry::unregisterBlobURL(readableURL);
> I think we should unregister this temporary URL after we finish the reading or encounter an error. This is because otherwise we have to assume that the un-registration always happens after the loading. This might be risky depending on how ThreadableLoader is implemented. For example, suppose we invoke FileReader in workers. If ThreadableLoader implementation needs to call into main thread once and then call other method asynchronously, we will have a problem.

I buy that... more robust

[Michael Nordman]

Created attachment 66966 [details]
decouple

[Jian Li]

Comment on attachment 66966 [details]
decouple

Looks good except some minor issues.

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

> WebCore/bindings/js/SerializedScriptValue.cpp:977
> +        file = File::create(String(path.ustring().impl()), KURL(KURL(), String(url.ustring().impl())), String(type.ustring().impl()));
Do we need to keep m_isDOMGlobalObject check here? The blob reading part has this logic kept.
    if (!m_isDOMGlobalObject)
        return jsNull();

> WebCore/fileapi/FileReader.cpp:164
> +void FileReader::clearLoaderAndReadableURL()
Probably it is better to call it cleanup since we might put other cleanup logic in this function in the future.

> WebCore/fileapi/FileReader.h:145
> +    KURL m_readableURL;
m_readableURL sounds like to mean the URL is legible. Might be better to name it something like m_urlForReading. If you make this change, do not forget to update ChangeLog.

> WebCore/fileapi/FileReaderSync.cpp:154
> +    // The blob is read by routing through the request handling layer given the tempory public url.
tempory => temporary

[Michael Nordman]

Comment on attachment 66966 [details]
decouple

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

> WebCore/bindings/js/SerializedScriptValue.cpp:977
> +        file = File::create(String(path.ustring().impl()), KURL(KURL(), String(url.ustring().impl())), String(type.ustring().impl()));
I couldn't tell you if this check is needed or not anymore in this code path. There was no way to construct an File w/o it previously, now there is. The 'blob' reading calls toJS, which takes the m_globlObject as a param which explains why the test is needed in that code path.

I'll put the test base in to keep the deltas smaller.

> WebCore/fileapi/FileReader.cpp:164
> +void FileReader::clearLoaderAndReadableURL()
rename sounds good

> WebCore/fileapi/FileReader.h:145
> +    KURL m_readableURL;
rename sounds good here too

> WebCore/fileapi/FileReaderSync.cpp:154
> +    // The blob is read by routing through the request handling layer given the tempory public url.
spelling error, thnx

[Michael Nordman]

Comment on attachment 66966 [details]
decouple

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

> WebCore/fileapi/FileReader.cpp:177
> +    ThreadableBlobRegistry::registerBlobURL(readableURL, m_blob->url());
ooops 'readableURL'... that's a compile error

(of course, this isn't being compiled in regular webkit builds so i don't see that when poking at this in a std webkit client, i really despise moving files/patches around)

[Jian Li]

(In reply to comment #7)
> (From update of attachment 66966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66966&action=prettypatch
> 
> > WebCore/fileapi/FileReader.cpp:177
> > +    ThreadableBlobRegistry::registerBlobURL(readableURL, m_blob->url());
> ooops 'readableURL'... that's a compile error
> 
> (of course, this isn't being compiled in regular webkit builds so i don't see that when poking at this in a std webkit client, i really despise moving files/patches around)

ENABLE(BLOB) is also turned on for WebKit Mac. If you have a mac machine, it is easy to build and test it.

[Michael Nordman]

> ENABLE(BLOB) is also turned on for WebKit Mac. If you have a mac machine, it is easy to build and test it.

Of course, i'll just move the patch there in order to see compile errors... did i mention how much i despise moving patches around :)

[Michael Nordman]

Created attachment 66983 [details]
decouple

minor mods mentioned above

[Michael Nordman]

> minor mods mentioned above

doh... forgot one of the minor mods

[Michael Nordman]

Created attachment 66985 [details]
decouple

including the minor mod to only deserialize Files if (m_isDOMGlobalObject) for jsc

[Jian Li]

Comment on attachment 66985 [details]
decouple

r=me

Please fix the minor typo issue before you land.

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

> WebCore/fileapi/FileReaderSync.cpp:154
> +    // The blob is read by routing through the request handling layer given the tempory public url.
tempory => temporary

[Michael Nordman]

Created attachment 67079 [details]
decouple

> tempory => temporary

Fixed. Also renamed the local var in that method from readableURL to urlForReading.

[Dumitru Daniliuc]

Comment on attachment 67079 [details]
decouple

rs=me. previously r+'d by jianli.

[WebKit Commit Bot]

Comment on attachment 67079 [details]
decouple

Clearing flags on attachment: 67079

Committed r67208: <http://trac.webkit.org/changeset/67208>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.