Bug 115888 – [BlackBerry] Unable to download blob resource

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Mary Wu]

For blob resource (blob:http....), it's not suitable to go to NetworkStream which don't handle "blob" protocol at all. since blob data already handled in BlobResourceHandle, simply get the data out to download stream.

[Mary Wu]

Created attachment 201322 [details]
patch

[WebKit Commit Bot]

Attachment 201322 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/PlatformBlackBerry.cmake', u'Source/WebCore/platform/network/blackberry/BlobStream.cpp', u'Source/WebCore/platform/network/blackberry/BlobStream.h', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp']" exit_code: 1
Source/WebCore/platform/network/blackberry/BlobStream.h:32:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/blackberry/BlobStream.h:33:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/network/blackberry/BlobStream.h:42:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Leo Yang]

Comment on attachment 201322 [details]
patch

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

> Source/WebCore/platform/network/blackberry/BlobStream.cpp:42
> +    notifyDataReceived(BlackBerry::Platform::createNetworkBufferByWrappingData((char*)data, len));

I'd like to the cast of from const char* to char* inside the platform layer because it knows more about the details.

[Rob Buis]

Comment on attachment 201322 [details]
patch

Leo's issue should be addressed and there are also some style issues.

[Mary Wu]

Created attachment 201784 [details]
update patch

[Benjamin Poulain]

Comment on attachment 201784 [details]
update patch

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

> Source/WebCore/platform/network/blackberry/BlobStream.h:36
> +    // From class BlackBerry::Platform::FilterStream

Missing period.

> Source/WebCore/platform/network/blackberry/BlobStream.h:38
> +    virtual BlackBerry::Platform::String url() const;
> +    virtual const BlackBerry::Platform::String mimeType() const;

Missing OVERRIDE.

> Source/WebCore/platform/network/blackberry/BlobStream.h:40
> +    // From class ResourceHandleClient

Missing period.

> Source/WebCore/platform/network/blackberry/BlobStream.h:43
> +    virtual void didReceiveData(ResourceHandle*, const char*, int, int /*encodedDataLength*/);
> +    virtual void didFinishLoading(ResourceHandle*, double /*finishTime*/);
> +    virtual void didFail(ResourceHandle*, const ResourceError&);

Missing override + don't put variable names into comments like that.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1220
> +            NetworkRequest req;

req is a bad variable name.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1221
> +            req.setRequestUrl(BlackBerry::Platform::String::fromUtf8(request.url().string().utf8().data()));

There is an implicit conversion operator for String -> BlackBerry::Platform::String.

[Mary Wu]

(In reply to comment #6)
> (From update of attachment 201784 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201784&action=review

thanks for comments, update soon

[Mary Wu]

Created attachment 201809 [details]
update patch

[Benjamin Poulain]

Comment on attachment 201809 [details]
update patch

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

No problem with me on WebKit side. The blackberry bits were already reviewed internally according to the changelog.

Just a question bellow.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1240
> +    BlackBerry::Platform::DataStream *stream = new BlackBerry::Platform::DataStream(netRequest);
> +    m_webPagePrivate->m_client->downloadRequested(stream, filename);
>      stream->streamOpen();

Who owns that memory?

[Joe Mason]

(In reply to comment #9)
> Who owns that memory?

A singleton in our network stack called DownloadManager. (Thanks for making me check that, because I found a bug in it!)

[Leo Yang]

Comment on attachment 201809 [details]
update patch

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

>> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1240
>>      stream->streamOpen();
> 
> Who owns that memory?

downloadRequested(). Mary please check downloadRequested() which may have leaks on early return branches.

[WebKit Commit Bot]

Comment on attachment 201809 [details]
update patch

Rejecting attachment 201809 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 201809, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
atformBlackBerry.cmake.rej
patching file Source/WebCore/platform/network/blackberry/BlobStream.cpp
patching file Source/WebCore/platform/network/blackberry/BlobStream.h
patching file Source/WebKit/blackberry/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Benjamin Poulain']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/478667

[Mary Wu]

(In reply to comment #10)
> (In reply to comment #9)
> > Who owns that memory?
> 
> A singleton in our network stack called DownloadManager. (Thanks for making me check that, because I found a bug in it!)

thanks, Benjamin, Joe and Leo, an internal PR track the possible leak issue.

[WebKit Commit Bot]

Comment on attachment 201809 [details]
update patch

Rejecting attachment 201809 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 201809, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
atformBlackBerry.cmake.rej
patching file Source/WebCore/platform/network/blackberry/BlobStream.cpp
patching file Source/WebCore/platform/network/blackberry/BlobStream.h
patching file Source/WebKit/blackberry/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Benjamin Poulain']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/480265

[Mary Wu]

Created attachment 201912 [details]
rebase the patch

[WebKit Commit Bot]

Comment on attachment 201912 [details]
rebase the patch

Clearing flags on attachment: 201912

Committed r150217: <http://trac.webkit.org/changeset/150217>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.