Bug 46543 – Fix passing blob data with string data item between threads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

We need to fix string data passing problem in order to cross thread safely.

[Jian Li]

Created attachment 68794 [details]
Proposed Patch

[David Levin]

I'll look at this. Jian and I already discussed one issue.

[Adam Barth]

Comment on attachment 68794 [details]
Proposed Patch

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

> WebCore/fileapi/BlobBuilder.cpp:52
> +    DEFINE_STATIC_LOCAL(AtomicString, transparent, ("transparent"));
> +    DEFINE_STATIC_LOCAL(AtomicString, native, ("native"));

Why AtomicString?  You're comparing against a non-Atomic string.

> WebCore/fileapi/BlobBuilder.cpp:59
> +    CString ctext = UTF8Encoding().encode(text.characters(), text.length(), EntitiesForUnencodables);

ctext?  Can we use a more descriptive variable name?

> WebCore/fileapi/BlobBuilder.cpp:73
> +            buffer.grow(buffer.size() + ctext.length());
> +            memcpy(buffer.data() + oldSize, ctext.data(), ctext.length());

Why not just use buffer.append() ?

> WebCore/platform/network/BlobData.cpp:46
> +    : m_data(data.leakPtr())

leakPtr ? So sad.

> WebCore/platform/network/BlobData.h:62
> +        Vector<char>* m_data;

Why can't this be an OwnPtr?

> WebCore/platform/text/LineEnding.cpp:73
> +#if OS(WINDOWS)

Why is this windows-specific?

> WebCore/platform/text/LineEnding.cpp:93
> +        unsigned oldSize = m_buffer.size();
> +        m_buffer.grow(oldSize + src.length());
> +        memcpy(m_buffer.data() + oldSize, src.data(), src.length());

again, why not just m_buffer.append() ?

[Michael Nordman]

Comment on attachment 68794 [details]
Proposed Patch

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

> WebCore/fileapi/BlobBuilder.cpp:66
> +        Vector<char>& buffer = *m_items[m_items.size() - 1].data.mutableData();

Is this the only use case for mutableData? Maybe instead of mutating an existing 'DataItem', just dump new data into a plain old Vector<char> that's owned by the builder and defer adding a DataItem to the list until you have a fully formed thing... buffer stuff up in a Vector, then flush it out as a DataItem.

> WebCore/platform/network/BlobData.h:63
> +        RefPtr<SharedRawData> m_sharedData;

Why have the m_data ptr at all, you can access the vector thru m_sharedData as needed, maybe a helper to return a const Vector<char>*

[David Levin]

Comment on attachment 68794 [details]
Proposed Patch

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

There are enough things in here to be addressed at the moment that I'm r-'ing.  I think once they are fixed some items will be a little clearer and there may be a few more comments.

> WebCore/platform/network/BlobData.h:36
> +#include <wtf/CrossThreadRefCounted.h>

I'd strongly discourage using CrossThreadRefCounted. It has a very special use case when the ref counting must live outside of the class (and probably a few other things that I don't remember right off).

Instead create a class that derives from ThreadSafeShared.  I suspect this change will make this code much easier to read and may clear up some issues that others were concerned about.

> WebCore/platform/network/BlobData.h:54
> +        void makeCrossThread();

makeCrossThread is a terrible name that I think I started.  I like the suggestion of detachFromCurrentThread (or something like that).

> WebCore/platform/text/LineEnding.cpp:89
> +    virtual void copy(const CString& src)

avoid abbreviations "src"

> WebKit/chromium/public/WebBlobData.h:31
>  #ifndef WebBlobData_h

If you break out this part of the change in to a separate patch, it would be better as Darin could review just that patch and likely do it quickly.

[Jian Li]

Created attachment 70179 [details]
Proposed Patch

[Jian Li]

All others are fixed.

(In reply to comment #3) 
> > WebCore/platform/text/LineEnding.cpp:73
> > +#if OS(WINDOWS)
> 
> Why is this windows-specific?
> 

This is because if the ending type is specified as "native", we need to do the line ending normalization per the platform-specific logic.

[Jian Li]

(In reply to comment #4)
> (From update of attachment 68794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review
> 
> > WebCore/fileapi/BlobBuilder.cpp:66
> > +        Vector<char>& buffer = *m_items[m_items.size() - 1].data.mutableData();
> 
> Is this the only use case for mutableData? Maybe instead of mutating an existing 'DataItem', just dump new data into a plain old Vector<char> that's owned by the builder and defer adding a DataItem to the list until you have a fully formed thing... buffer stuff up in a Vector, then flush it out as a DataItem.

Since now I put a Vector<char> directly in RawData, it will add up some more complexity to change as suggest, though it does simply the logic in BlobBuilder, but it would add up the complexity in BlobData. I rather keep it as is now.

> 
> > WebCore/platform/network/BlobData.h:63
> > +        RefPtr<SharedRawData> m_sharedData;
> 
> Why have the m_data ptr at all, you can access the vector thru m_sharedData as needed, maybe a helper to return a const Vector<char>*

Removed due to the new structure change.

[Jian Li]

All fixed. WebBlobData is stripped out and it will be included in another patch for review.

(In reply to comment #5)
> (From update of attachment 68794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68794&action=review
> 
> There are enough things in here to be addressed at the moment that I'm r-'ing.  I think once they are fixed some items will be a little clearer and there may be a few more comments.
>

[WebKit Review Bot]

Attachment 70179 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4259033

[David Levin]

Comment on attachment 70179 [details]
Proposed Patch

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

> WebCore/fileapi/BlobBuilder.cpp:53
> +    if (!endingType.isEmpty() && !isEndingTypeTransparent && !isEndingTypeNative) {

This looks like an interesting error. Is there a test case for it? Could there be one?

> WebCore/platform/text/LineEnding.cpp:73
> +#if OS(WINDOWS)

I wouldn't ifdef this because it make it confusing to try to guess why it is ifdef'ed out.

> WebCore/platform/text/LineEnding.cpp:128
> +    char* q = buffer.allocate(newLen);

It would be nice to have a more descriptive variable name than "q".

[WebKit Review Bot]

http://trac.webkit.org/changeset/69610 might have broken Chromium Win Release

[Jian Li]

Committed as http://trac.webkit.org/changeset/69610.