[webkit-reviews] review denied: [Bug 25290] REGRESSION(r41732): Crash when constructing XMLHttpRequest in a detached document : [Attachment 29621] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 25290: REGRESSION(r41732): Crash when constructing XMLHttpRequest in a
detached document
https://bugs.webkit.org/show_bug.cgi?id=25290

Attachment 29621: proposed patch
https://bugs.webkit.org/attachment.cgi?id=29621&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    KURL url = this->url();
> +    NSURL* nsURL;
> +    if (m_responseContentDispositionEncodingFallbackArray.isEmpty())
> +	   nsURL = url;
> +    else {
> +	   CString urlString =
TextEncoding(m_responseContentDispositionEncodingFallbackArray[0]).encode(url.s
tring().characters(), url.string().length(),
URLEncodedEntitiesForUnencodables);
> +	   RetainPtr<CFURLRef> cfURL =
CFURLCreateAbsoluteURLWithBytes(kCFAllocatorDefault, (const
UInt8*)urlString.data(), urlString.length(),
CFStringConvertIANACharSetNameToEncoding(m_responseContentDispositionEncodingFa
llbackArray[0].createCFString()), 0, false);
> +	   nsURL = (NSURL*)cfURL.get();
> +	   [[nsURL retain] autorelease];;
> +    }
> +
>      if (nsRequest)
> -	   [nsRequest setURL:url()];
> +	   [nsRequest setURL:nsURL];
>      else
> -	   nsRequest = [[NSMutableURLRequest alloc] initWithURL:url()];
> +	   nsRequest = [[NSMutableURLRequest alloc] initWithURL:nsURL];

I suggest making the new nsURL variable a RetainPtr<NSURL*>, and therefore
avoiding autorelease.

Is there a way we could avoid 

There are double semicolons on the line that calls autorelease.

A local variable for the encoding name would make the expression easier to
read.

This code leaks the string created by createCFString; probably best to fix that
using a RetainPtr<CFStringRef>.

review- because of the leak