Bug 19578 - [CURL] problem in parseDataUrl
: [CURL] problem in parseDataUrl
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All Linux
: P2 Normal
Assigned To:
:
: Curl, Gtk
:
: 20924
  Show dependency treegraph
 
Reported: 2008-06-16 07:03 PST by
Modified: 2008-10-04 23:16 PST (History)


Attachments
page test (6.20 KB, text/html)
2008-06-16 07:05 PST, Mario Bensi
no flags Details
fix the base64 problem (1.63 KB, patch)
2008-06-16 07:06 PST, Mario Bensi
eric: review-
Review Patch | Details | Formatted Diff | Diff
fix the base64 problem ( using fastMalloc/fastFree ) (1.64 KB, patch)
2008-08-04 01:10 PST, Mario Bensi
zecke: review-
Review Patch | Details | Formatted Diff | Diff
fix the base64 problem ( fix coding style ) (1.64 KB, patch)
2008-08-13 00:50 PST, Mario Bensi
eric: review-
Review Patch | Details | Formatted Diff | Diff
keep only the vector out (1.64 KB, patch)
2008-08-22 08:19 PST, Mario Bensi
mario.bensi: review-
Review Patch | Details | Formatted Diff | Diff
keep vector ( real patch ) (1.90 KB, patch)
2008-08-22 08:21 PST, Mario Bensi
darin: review+
Review Patch | Details | Formatted Diff | Diff
fix ChangeLog (2.03 KB, patch)
2008-08-23 23:51 PST, Mario Bensi
eric: review+
Review Patch | Details | Formatted Diff | Diff
remove glib part and remove tab in changeLog (2.80 KB, patch)
2008-08-26 01:37 PST, Mario Bensi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-06-16 07:03:50 PST
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 From 2008-06-16 07:05:12 PST -------
Created an attachment (id=21726) [details]
page test
------- Comment #2 From 2008-06-16 07:06:44 PST -------
Created an attachment (id=21727) [details]
fix the base64 problem
------- Comment #3 From 2008-06-23 10:57:41 PST -------
Adding [CURL] to title to help reviewers know who should review this.
------- Comment #4 From 2008-08-01 16:50:21 PST -------
(From update of attachment 21727 [details])
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 From 2008-08-04 01:10:01 PST -------
Created an attachment (id=22628) [details]
fix the base64 problem ( using fastMalloc/fastFree )
------- Comment #6 From 2008-08-12 14:30:43 PST -------
(From update of attachment 22628 [details])
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 From 2008-08-13 00:50:38 PST -------
Created an attachment (id=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 From 2008-08-13 05:00:46 PST -------
Hi Mario. Should the review flag be '?' rather than a '+'? Thanks.
------- Comment #9 From 2008-08-22 02:34:59 PST -------
(From update of attachment 22771 [details])
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 From 2008-08-22 02:45:39 PST -------
(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 From 2008-08-22 08:19:48 PST -------
Created an attachment (id=22936) [details]
keep only the vector out

I removed the char* and the string conversion, i have keep only the vector
------- Comment #12 From 2008-08-22 08:21:47 PST -------
Created an attachment (id=22937) [details]
keep vector ( real patch )
------- Comment #13 From 2008-08-22 10:57:24 PST -------
(From update of attachment 22937 [details])
+        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 From 2008-08-23 23:51:09 PST -------
Created an attachment (id=22956) [details]
fix ChangeLog
------- Comment #15 From 2008-08-25 21:01:27 PST -------
(From update of attachment 22956 [details])
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 From 2008-08-26 01:37:27 PST -------
Created an attachment (id=22996) [details]
remove glib part and remove tab in changeLog
------- Comment #17 From 2008-08-27 16:58:50 PST -------
(From update of attachment 22996 [details])
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 From 2008-08-27 17:05:29 PST -------
r35954
------- Comment #19 From 2008-09-24 14:37:30 PST -------
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 From 2008-09-25 01:30:16 PST -------
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 From 2008-09-25 14:34:22 PST -------
(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 From 2008-09-28 09:02:28 PST -------
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 From 2008-10-04 23:16:47 PST -------
Closing as bug #20924 was fixed in r37314 which includes a fix for this regression.