Bug 71539 – [Mac] ResourceRequest's nsURLRequest() does not differentiate null and empty URLs with CFNetwork

• Component: Page Loading
• Status: RESOLVED FIXED

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 113622 [details]
Patch

[Benjamin Poulain]

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.

[David Kilzer (:ddkilzer)]

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?

[David Kilzer (:ddkilzer)]

<rdar://problem/10041448>

[Benjamin Poulain]

Created attachment 113717 [details]
Patch

[Benjamin Poulain]

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

[David Kilzer (:ddkilzer)]

Comment on attachment 113717 [details]
Patch

r=me!

[Pratik Solanki]

Comment on attachment 113717 [details]
Patch

Patch looks good to me as well.

[WebKit Review Bot]

Comment on attachment 113717 [details]
Patch

Clearing flags on attachment: 113717

Committed r99334: <http://trac.webkit.org/changeset/99334>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

.

[Benjamin Poulain]

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

[Csaba Osztrogonác]

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

[Tor Arne Vestbø]

(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