Bug 19578 – [CURL] problem in parseDataUrl

• Component: Platform
• Status: RESOLVED FIXED

[Mario Bensi]

I use the curl ResourceHandleManager without USE_GLIB_BASE64 and when I test the base64.html page, the image data are not rendered correctly.

[Mario Bensi]

Created attachment 21726 [details]
page test

[Mario Bensi]

Created attachment 21727 [details]
fix the base64 problem

[Darin Adler]

Adding [CURL] to title to help reviewers know who should review this.

[Eric Seidel (no email)]

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. :)

[Mario Bensi]

Created attachment 22628 [details]
fix the base64 problem ( using fastMalloc/fastFree )

[Holger Freyther]

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?

[Mario Bensi]

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.

[Jan Alonzo]

Hi Mario. Should the review flag be '?' rather than a '+'? Thanks.

[Eric Seidel (no email)]

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.

[Mario Bensi]

(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...

[Mario Bensi]

Created attachment 22936 [details]
keep only the vector out

I removed the char* and the string conversion, i have keep only the vector

[Mario Bensi]

Created attachment 22937 [details]
keep vector ( real patch )

[Darin Adler]

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

[Mario Bensi]

Created attachment 22956 [details]
fix ChangeLog

[Eric Seidel (no email)]

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.

[Mario Bensi]

Created attachment 22996 [details]
remove glib part and remove tab in changeLog

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

r35954

[Alp Toker]

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

[Mario Bensi]

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
> 

[Clemmitt Sigler]

(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

[Mario Bensi]

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
> 

[Jan Alonzo]

Closing as bug #20924 was fixed in r37314 which includes a fix for this regression.