Bug 45410 - Decouple Blobs from ScriptExecutionContext.
: Decouple Blobs from ScriptExecutionContext.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Michael Nordman
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-08 12:42 PDT by Michael Nordman
Modified: 2010-09-10 09:20 PDT (History)
2 users (show)

See Also:


Attachments
decouple (37.54 KB, patch)
2010-09-08 12:47 PDT, Michael Nordman
jianli: review-
Details | Formatted Diff | Diff
decouple (38.97 KB, patch)
2010-09-08 16:58 PDT, Michael Nordman
jianli: review+
Details | Formatted Diff | Diff
decouple (39.07 KB, patch)
2010-09-08 18:27 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
decouple (39.11 KB, patch)
2010-09-08 18:32 PDT, Michael Nordman
jianli: review+
Details | Formatted Diff | Diff
decouple (39.12 KB, patch)
2010-09-09 12:26 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2010-09-08 12:42:02 PDT
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.
Comment 1 Michael Nordman 2010-09-08 12:47:21 PDT
Created attachment 66930 [details]
decouple
Comment 2 Jian Li 2010-09-08 13:35:10 PDT
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.
Comment 3 Michael Nordman 2010-09-08 14:32:06 PDT
(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
Comment 4 Michael Nordman 2010-09-08 16:58:07 PDT
Created attachment 66966 [details]
decouple
Comment 5 Jian Li 2010-09-08 17:31:14 PDT
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
Comment 6 Michael Nordman 2010-09-08 17:44:22 PDT
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
Comment 7 Michael Nordman 2010-09-08 17:56:19 PDT
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)
Comment 8 Jian Li 2010-09-08 18:06:53 PDT
(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.
Comment 9 Michael Nordman 2010-09-08 18:22:05 PDT
> 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 :)
Comment 10 Michael Nordman 2010-09-08 18:27:21 PDT
Created attachment 66983 [details]
decouple

minor mods mentioned above
Comment 11 Michael Nordman 2010-09-08 18:28:39 PDT
> minor mods mentioned above

doh... forgot one of the minor mods
Comment 12 Michael Nordman 2010-09-08 18:32:26 PDT
Created attachment 66985 [details]
decouple

including the minor mod to only deserialize Files if (m_isDOMGlobalObject) for jsc
Comment 13 Jian Li 2010-09-08 19:04:01 PDT
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
Comment 14 Michael Nordman 2010-09-09 12:26:14 PDT
Created attachment 67079 [details]
decouple

> tempory => temporary

Fixed. Also renamed the local var in that method from readableURL to urlForReading.
Comment 15 Dumitru Daniliuc 2010-09-09 12:31:16 PDT
Comment on attachment 67079 [details]
decouple

rs=me. previously r+'d by jianli.
Comment 16 WebKit Commit Bot 2010-09-10 09:20:23 PDT
Comment on attachment 67079 [details]
decouple

Clearing flags on attachment: 67079

Committed r67208: <http://trac.webkit.org/changeset/67208>
Comment 17 WebKit Commit Bot 2010-09-10 09:20:28 PDT
All reviewed patches have been landed.  Closing bug.