Bug 116223

Summary: Remove the incorrectly implemented and unused SQLiteStatement::getColumnBlob().
Product: WebKit Reporter: Lyon Chen <liachen>
Component: PlatformAssignee: Lyon Chen <liachen>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: beidson, darin, joenotcharles
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch for 116223.
none
Remove it instead of fixing it. benjamin: review+

Description Lyon Chen 2013-05-16 07:40:55 PDT
In SQLiteStatement::getColumnBlob(), it calls finalize(), prepare(), and step() unconditionally, which makes user can never get blob data out through this method. It should be changed to match method like getColumnBlobAsString() and getColumnBlobAsVector(), which assumes when m_statement is there then prepare() and step() have been called before this being called. And this will match what bindBlob() does, saving any binary data with defined length.
Comment 1 Lyon Chen 2013-05-16 07:43:22 PDT
cc: Joe Mason, Darin, Beidson.
Comment 2 Lyon Chen 2013-05-16 07:55:45 PDT
Created attachment 201957 [details]
Patch for 116223.
Comment 3 Joe Mason 2013-05-16 08:31:18 PDT
Comment on attachment 201957 [details]
Patch for 116223.

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

> Source/WebCore/ChangeLog:15
> +        No new tests needed as no one use SQLiteStatement::getColumnBlob() yet.

Still needs a test, to make sure it's actually working when someone wants to use it.
Comment 4 Lyon Chen 2013-05-16 08:39:02 PDT
(In reply to comment #3)
> (From update of attachment 201957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201957&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        No new tests needed as no one use SQLiteStatement::getColumnBlob() yet.
> 
> Still needs a test, to make sure it's actually working when someone wants to use it.

So it will be a unit test, instead of layout test?
Comment 5 Alexey Proskuryakov 2013-05-16 09:42:38 PDT
I'd say that if SQLiteStatement::getColumnBlob() is never called, we should strongly consider removing it completely.
Comment 6 Lyon Chen 2013-05-16 09:43:51 PDT
(In reply to comment #5)
> I'd say that if SQLiteStatement::getColumnBlob() is never called, we should strongly consider removing it completely.

But I am planning to use it, and then found the issue.
Comment 7 Lyon Chen 2013-05-16 10:44:28 PDT
Created attachment 201969 [details]
Remove it instead of fixing it.
Comment 8 Joe Mason 2013-05-16 11:52:44 PDT
(In reply to comment #7)
> Created an attachment (id=201969) [details]
> Remove it instead of fixing it.

Are you planning to re-add it along with our platform specific patch once we're finished the code that uses it?
Comment 9 Joe Mason 2013-05-16 11:53:37 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=201969) [details] [details]
> > Remove it instead of fixing it.
> 
> Are you planning to re-add it along with our platform specific patch once we're finished the code that uses it?

Never mind, I actually looked at the patch and I see your comment now. :)
Comment 10 Lyon Chen 2013-05-16 12:01:38 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Created an attachment (id=201969) [details] [details] [details]
> > > Remove it instead of fixing it.
> > 
> > Are you planning to re-add it along with our platform specific patch once we're finished the code that uses it?
> 
> Never mind, I actually looked at the patch and I see your comment now. :)

Yeah, I thought getColumnBlob name more correct than getColumnBlobAsVector, and can avoid unnecessary full copy. But then I think maybe this is not that terrible. At least IconDatabase::getImageDataForIconURLFromSQLDatabase() is doing it this way. (I think it should use SharedBuffer::adoptVector() instead of SharedBuffer::create(), but it is another story), and our password is short.
Comment 11 Benjamin Poulain 2013-05-16 16:27:54 PDT
Comment on attachment 201969 [details]
Remove it instead of fixing it.

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

> Source/WebCore/ChangeLog:12
> +        No new tests needed.

Whaaaaat? ;)
Comment 12 Benjamin Poulain 2013-05-16 16:30:00 PDT
Comment on attachment 201969 [details]
Remove it instead of fixing it.

Let's cq- actually:
-Please change the title to reflect what is in the patch.
Comment 13 Lyon Chen 2013-05-16 18:40:15 PDT
Comment on attachment 201969 [details]
Remove it instead of fixing it.

Re-set the commit-queue flag after changing the bug title to reflect the patch.

Ben: do you feel the changed title is OK now?
Comment 14 Tim Nguyen (:ntim) 2022-03-14 16:35:44 PDT
Comment on attachment 201969 [details]
Remove it instead of fixing it.

Bug 127868 made the same change.
Comment 15 Tim Nguyen (:ntim) 2022-03-14 16:36:02 PDT

*** This bug has been marked as a duplicate of bug 127868 ***