Bug 71539 - [Mac] ResourceRequest's nsURLRequest() does not differentiate null and empty URLs with CFNetwork
Summary: [Mac] ResourceRequest's nsURLRequest() does not differentiate null and empty ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-11-03 22:19 PDT by Benjamin Poulain
Modified: 2011-11-07 09:08 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.81 KB, patch)
2011-11-03 22:25 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2011-11-04 14:38 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2011-11-03 22:19:48 PDT
CFURL C API does not handle empty string. Creating a CFURL with an empty CFString gives you a NULL CFURLRef.

WebKit's KURL and NSURL make the distinction between null and empty URL.

When using CFNetwork internally and the Foundation classes for the API, this gives us inconsistency with OS X and crashes when accessing the null URL. We shall support empty URL when we use both CFURL and NSURL and convert between the two in the network layer.
Comment 1 Benjamin Poulain 2011-11-03 22:25:03 PDT
Created attachment 113622 [details]
Patch
Comment 2 Benjamin Poulain 2011-11-03 22:26:53 PDT
The next step is to get rid of copyToBuffer() (we are the only one using that) and fix the warning. I did not do that here to not fix two bugs in one patch.
Comment 3 David Kilzer (:ddkilzer) 2011-11-04 10:34:49 PDT
Comment on attachment 113622 [details]
Patch

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

r=me, but setting the r- flag since you wanted this to go through the commit queue.  (Also, it seems the win EWS bot is building for Mac OS X?!)

> Source/WebCore/platform/cf/KURLCFNet.cpp:82
> +    CharBuffer buffer;

I don't think you can use CharBuffer here, can you?  It only seems to be typedef-ed in KURL.cpp.

Might be useful to move that typedef to KURL.h in another patch, though.

> Source/WebCore/platform/mac/KURLMac.mm:75
> +        return (CFURLRef)[[NSURL alloc] initWithString:@""];

Can you use reinterpret_cast<CFURLRef>() instead of a C-style cast here?
Comment 4 David Kilzer (:ddkilzer) 2011-11-04 14:09:20 PDT
<rdar://problem/10041448>
Comment 5 Benjamin Poulain 2011-11-04 14:38:08 PDT
Created attachment 113717 [details]
Patch
Comment 6 Benjamin Poulain 2011-11-04 14:40:10 PDT
> > Source/WebCore/platform/cf/KURLCFNet.cpp:82
> > +    CharBuffer buffer;
> 
> I don't think you can use CharBuffer here, can you?  It only seems to be typedef-ed in KURL.cpp.

Yep, good catch.

> Might be useful to move that typedef to KURL.h in another patch, though.

I would prefer not to spread the CharBuffer typedef. I am trying to fix the 2 FIXME in a follow up and that should get rid of the CharBuffer in the Mac specific files.
Comment 7 David Kilzer (:ddkilzer) 2011-11-04 16:22:24 PDT
Comment on attachment 113717 [details]
Patch

r=me!
Comment 8 Pratik Solanki 2011-11-04 16:24:04 PDT
Comment on attachment 113717 [details]
Patch

Patch looks good to me as well.
Comment 9 WebKit Review Bot 2011-11-04 17:03:01 PDT
Comment on attachment 113717 [details]
Patch

Clearing flags on attachment: 113717

Committed r99334: <http://trac.webkit.org/changeset/99334>
Comment 10 WebKit Review Bot 2011-11-04 17:03:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Csaba Osztrogonác 2011-11-06 14:53:59 PST
Comment on attachment 113717 [details]
Patch

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

It broke QtWebKit on Mac:

ld: duplicate symbol WebCore::KURL::createCFURL() constin /buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/WebKitBuild/Release/Source/WebCore/release/libwebcore.a(KURLCFNet.o) and /buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/WebKitBuild/Release/Source/WebCore/release/libwebcore.a(KURLMac.o)

> Source/WebCore/platform/cf/KURLCFNet.cpp:79
> +#if !PLATFORM(MAC)
> +CFURLRef KURL::createCFURL() const

It is true if you build QtWebKit on Mac.

> Source/WebCore/platform/mac/KURLMac.mm:70
> +CFURLRef KURL::createCFURL() const

.
Comment 12 Benjamin Poulain 2011-11-06 15:08:39 PST
(In reply to comment #11)
> (From update of attachment 113717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113717&action=review
> 
> It broke QtWebKit on Mac:

Do you need this file on QtWebKit? It is to convert KURL to/from CFURL. Maybe this was added by accident?
Comment 13 Csaba Osztrogonác 2011-11-07 08:43:16 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 113717 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=113717&action=review
> > 
> > It broke QtWebKit on Mac:
> 
> Do you need this file on QtWebKit? It is to convert KURL to/from CFURL. Maybe this was added by accident?

Alexis, have you got any idea? You usually hack QtWebKit on Mac.
Comment 14 Tor Arne Vestbø 2011-11-07 09:08:42 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 113717 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=113717&action=review
> > 
> > It broke QtWebKit on Mac:
> 
> Do you need this file on QtWebKit? It is to convert KURL to/from CFURL. Maybe this was added by accident?

We use it when we use the QuickTime media backend :/ Landed a buildfix in 

http://trac.webkit.org/changeset/99429