Bug 115888 - [BlackBerry] Unable to download blob resource
Summary: [BlackBerry] Unable to download blob resource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mary Wu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-09 20:30 PDT by Mary Wu
Modified: 2013-05-16 17:11 PDT (History)
6 users (show)

See Also:


Attachments
patch (10.34 KB, patch)
2013-05-10 00:06 PDT, Mary Wu
rwlbuis: review-
Details | Formatted Diff | Diff
update patch (10.32 KB, patch)
2013-05-14 20:20 PDT, Mary Wu
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
update patch (10.47 KB, patch)
2013-05-15 02:27 PDT, Mary Wu
benjamin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
rebase the patch (10.48 KB, patch)
2013-05-15 19:46 PDT, Mary Wu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mary Wu 2013-05-09 20:30:21 PDT
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.
Comment 1 Mary Wu 2013-05-10 00:06:27 PDT
Created attachment 201322 [details]
patch
Comment 2 WebKit Commit Bot 2013-05-10 00:08:02 PDT
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.
Comment 3 Leo Yang 2013-05-10 07:07:34 PDT
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.
Comment 4 Rob Buis 2013-05-10 11:19:38 PDT
Comment on attachment 201322 [details]
patch

Leo's issue should be addressed and there are also some style issues.
Comment 5 Mary Wu 2013-05-14 20:20:28 PDT
Created attachment 201784 [details]
update patch
Comment 6 Benjamin Poulain 2013-05-15 01:02:52 PDT
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.
Comment 7 Mary Wu 2013-05-15 02:09:54 PDT
(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
Comment 8 Mary Wu 2013-05-15 02:27:54 PDT
Created attachment 201809 [details]
update patch
Comment 9 Benjamin Poulain 2013-05-15 13:47:04 PDT
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?
Comment 10 Joe Mason 2013-05-15 14:02:21 PDT
(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!)
Comment 11 Leo Yang 2013-05-15 14:06:39 PDT
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.
Comment 12 WebKit Commit Bot 2013-05-15 18:58:01 PDT
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
Comment 13 Mary Wu 2013-05-15 19:13:56 PDT
(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.
Comment 14 WebKit Commit Bot 2013-05-15 19:24:17 PDT
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
Comment 15 Mary Wu 2013-05-15 19:46:53 PDT
Created attachment 201912 [details]
rebase the patch
Comment 16 WebKit Commit Bot 2013-05-16 17:11:24 PDT
Comment on attachment 201912 [details]
rebase the patch

Clearing flags on attachment: 201912

Committed r150217: <http://trac.webkit.org/changeset/150217>
Comment 17 WebKit Commit Bot 2013-05-16 17:11:27 PDT
All reviewed patches have been landed.  Closing bug.