Bug 19578

Summary: [CURL] problem in parseDataUrl
Product: WebKit Reporter: Mario Bensi <mario.bensi>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ahya365, alp, eric, jmalonzo
Priority: P2 Keywords: Curl, Gtk
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Bug Depends on:    
Bug Blocks: 20924    
Attachments:
Description Flags
page test
none
fix the base64 problem
eric: review-
fix the base64 problem ( using fastMalloc/fastFree )
zecke: review-
fix the base64 problem ( fix coding style )
eric: review-
keep only the vector out
mario.bensi: review-
keep vector ( real patch )
darin: review+
fix ChangeLog
eric: review+
remove glib part and remove tab in changeLog none

Description Mario Bensi 2008-06-16 07:03:50 PDT
I use the curl ResourceHandleManager without USE_GLIB_BASE64 and when I test the base64.html page, the image data are not rendered correctly.
Comment 1 Mario Bensi 2008-06-16 07:05:12 PDT
Created attachment 21726 [details]
page test
Comment 2 Mario Bensi 2008-06-16 07:06:44 PDT
Created attachment 21727 [details]
fix the base64 problem
Comment 3 Darin Adler 2008-06-23 10:57:41 PDT
Adding [CURL] to title to help reviewers know who should review this.
Comment 4 Eric Seidel (no email) 2008-08-01 16:50:21 PDT
Comment on attachment 21727 [details]
fix the base64 problem

We don't use malloc() and free directly.  We use fastMalloc() and fastFree() when possible, or new/delete.

Also, this should just be held in an OwnPtr or an OwnArrayPtr so that it's automatically cleaned up when the block is exited.

No reason to use manual c-style memory management when we have nicer c++ tools. :)
Comment 5 Mario Bensi 2008-08-04 01:10:01 PDT
Created attachment 22628 [details]
fix the base64 problem ( using fastMalloc/fastFree )
Comment 6 Holger Freyther 2008-08-12 14:30:43 PDT
Comment on attachment 22628 [details]
fix the base64 problem ( using fastMalloc/fastFree )

Thanks for the patch. The for loop needs some more spaces to comply with our Coding Style Guidelines and I wonder why memcpy can not be used?

I also wonder what problem is getting fixed? Vector<char> out is a temporary variable, and String(out.data(), out.size()) will make a deep copy of the string. So how does this fail?
Comment 7 Mario Bensi 2008-08-13 00:50:38 PDT
Created attachment 22771 [details]
fix the base64 problem ( fix coding style )

I have fix the coding style

I have not test with a memcopy

the value in out is good but when you copy this in String the value are interpreted or other but is not the same. With a char* it's correct.
Comment 8 Jan Alonzo 2008-08-13 05:00:46 PDT
Hi Mario. Should the review flag be '?' rather than a '+'? Thanks.
Comment 9 Eric Seidel (no email) 2008-08-22 02:34:59 PDT
Comment on attachment 22771 [details]
fix the base64 problem ( fix coding style )

Please use a Vector<char> instead of using manual memory management.

Even OwnPtr would be better than manual use of fastFree.

Also, else should be on the same line as your }

In fact!  out is already a Vector<char>!! So you're just making a needless copy here. :(

Please move the Vector<char> out declaration outside of the if block.  And heck, just remove the USE_GLIB_BASE64 path, there is no reason to use g_base64_decode even if it's available.  Just remove the g_base64_decode codepath entirely.
Comment 10 Mario Bensi 2008-08-22 02:45:39 PDT
(In reply to comment #9)
> (From update of attachment 22771 [details] [edit])
> Please use a Vector<char> instead of using manual memory management.
> 
> Even OwnPtr would be better than manual use of fastFree.
> 
> Also, else should be on the same line as your }
> 
> In fact!  out is already a Vector<char>!! So you're just making a needless copy
> here. :(
> 
> Please move the Vector<char> out declaration outside of the if block.  And
> heck, just remove the USE_GLIB_BASE64 path, there is no reason to use
> g_base64_decode even if it's available.  Just remove the g_base64_decode
> codepath entirely.
> 

I was my first test (move the Vector<char> out declaration outside of the if block) but the result is not good, i don't see google image...
Comment 11 Mario Bensi 2008-08-22 08:19:48 PDT
Created attachment 22936 [details]
keep only the vector out

I removed the char* and the string conversion, i have keep only the vector
Comment 12 Mario Bensi 2008-08-22 08:21:47 PDT
Created attachment 22937 [details]
keep vector ( real patch )
Comment 13 Darin Adler 2008-08-22 10:57:24 PDT
Comment on attachment 22937 [details]
keep vector ( real patch )

+        fix the String conversion on base64 data in parseDataUrl

This is a kind of vague description of what's being changed. And it doesn't contain a bugs.webkit.org URL; it should.

+        WARNING: NO TEST CASES ADDED OR CHANGED

That should have been removed from the ChangeLog.

+        * platform/network/curl/ResourceHandleManager.cpp:
+        (WebCore::parseDataUrl):

It's better if the ChangeLog actually says what the change is.

The ifdefs in this function are kind of strange. Some of the code using outData and out is inside #ifdefs and others are outside and unconditional. It would be much better to be consistent.

But I'll say r=me, as-is
Comment 14 Mario Bensi 2008-08-23 23:51:09 PDT
Created attachment 22956 [details]
fix ChangeLog
Comment 15 Eric Seidel (no email) 2008-08-25 21:01:27 PDT
Comment on attachment 22956 [details]
fix ChangeLog

I know Darin r+'d the old one, but I think this patch still could use one more round.

The changelog contains tabs.  I still see no reason why the USE_GLIB_BASE64 path needs to exist.  I suggest we remove it.  r- for the tabs in the changelog and lack of explanation as to why we should keep the USE_GLIB_BASE64 path.

Otherwise the patch looks fine.  Thanks again!  I look forward to seeing the final patch.
Comment 16 Mario Bensi 2008-08-26 01:37:27 PDT
Created attachment 22996 [details]
remove glib part and remove tab in changeLog
Comment 17 Eric Seidel (no email) 2008-08-27 16:58:50 PDT
Comment on attachment 22996 [details]
remove glib part and remove tab in changeLog

I'm so sorry.  I think you were probably right to leave it in the first place.  I've been convinced otherwise.  I'll mark your previous patch as r+ (and land it for you).  Sorry for the extra work
Comment 18 Eric Seidel (no email) 2008-08-27 17:05:29 PDT
r35954
Comment 19 Alp Toker 2008-09-24 14:37:30 PDT
Please back this change out ASAP. Causes all data URL parsing tests and Acid2/Acid3 fail.

If for some reason you can't get DumpRenderTree working, there are manual tests at http://www-archive.mozilla.org/quality/networking/testing/datatests

Regarding the Base64 decoding question, there's a comment right in the code explaining why we chose g_base64_decode() over base64Decode():

        // Use the GLib Base64 if available, since WebCore's decoder isn't
        // general-purpose and fails on Acid3 test 97 (whitespace).

It would be best if changes to the network stack are reviewed by someone who can test them in future. Thanks
Comment 20 Mario Bensi 2008-09-25 01:30:16 PDT
If you see the patch applied (fix ChangeLog), there is not impact on the GTK part, I think the problem on Acid2 and Acid3 is not here.



(In reply to comment #19)
> Please back this change out ASAP. Causes all data URL parsing tests and
> Acid2/Acid3 fail.
> 
> If for some reason you can't get DumpRenderTree working, there are manual tests
> at http://www-archive.mozilla.org/quality/networking/testing/datatests
> 
> Regarding the Base64 decoding question, there's a comment right in the code
> explaining why we chose g_base64_decode() over base64Decode():
> 
>         // Use the GLib Base64 if available, since WebCore's decoder isn't
>         // general-purpose and fails on Acid3 test 97 (whitespace).
> 
> It would be best if changes to the network stack are reviewed by someone who
> can test them in future. Thanks
> 
Comment 21 Clemmitt Sigler 2008-09-25 14:34:22 PDT
(In reply to comment #20)
> If you see the patch applied (fix ChangeLog), there is not impact on the GTK
> part, I think the problem on Acid2 and Acid3 is not here.

Hi Mario (et al),

I looked at the attached patch above you reference (called "fix ChangeLog") and it appears to be exactly what was applied in r35954.  I must contradict what you say, "I think the problem on Acid2 and Acid3 is not here."  It is there, in that very patch.

Please see my bug https://bugs.webkit.org/show_bug.cgi?id=20924 for resolution of the Acid2/Acid3 breakage (which, for me, showed up in the GTK/Linux build).  It gives the one-liner which causes these errors.

Please contact Eric and ask him to back out r35954 (as requested above by Alp), or submit a second patch which fixes the error debugged in Bug 20924, at once.  TIA.

Clemmitt
Comment 22 Mario Bensi 2008-09-28 09:02:28 PDT
I added a patch in bug 20924 to fix the acid regression, this don't remove the change done in the old patch. With this fix acid2 is correct and you have 100% in acid3

Is correct for you ?

I wait a review here : https://bugs.webkit.org/show_bug.cgi?id=20924

Mario

(In reply to comment #21)
> (In reply to comment #20)
> > If you see the patch applied (fix ChangeLog), there is not impact on the GTK
> > part, I think the problem on Acid2 and Acid3 is not here.
> 
> Hi Mario (et al),
> 
> I looked at the attached patch above you reference (called "fix ChangeLog") and
> it appears to be exactly what was applied in r35954.  I must contradict what
> you say, "I think the problem on Acid2 and Acid3 is not here."  It is there, in
> that very patch.
> 
> Please see my bug https://bugs.webkit.org/show_bug.cgi?id=20924 for resolution
> of the Acid2/Acid3 breakage (which, for me, showed up in the GTK/Linux build). 
> It gives the one-liner which causes these errors.
> 
> Please contact Eric and ask him to back out r35954 (as requested above by Alp),
> or submit a second patch which fixes the error debugged in Bug 20924, at once. 
> TIA.
> 
> Clemmitt
> 

Comment 23 Jan Alonzo 2008-10-04 23:16:47 PDT
Closing as bug #20924 was fixed in r37314 which includes a fix for this regression.