Bug 25290

Summary: REGRESSION(r41732): Crash when constructing XMLHttpRequest in a detached document
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore JavaScriptAssignee: Alexey Proskuryakov <ap>
Severity: Normal Keywords: Regression
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
proposed patch
darin: review-
proposed patch darin: review+

Description Alexey Proskuryakov 2009-04-20 01:51:09 PDT
Now that JSXMLHttpRequestConstructor (and other constructors) are tied to a global object, not to a script execution context, we need to take care of edge cases where converting from ScriptExecutionContext to JSDOMGlobalObject doesn't work. Namely, both exist for a detached document, but toJSDOMGlobalObject(scriptExecutionContext) returns 0.

Patch forthcoming.
Comment 1 Alexey Proskuryakov 2009-04-20 12:58:17 PDT
Created attachment 29621 [details]
proposed patch
Comment 2 Darin Adler 2009-04-20 13:55:36 PDT
Comment on attachment 29621 [details]
proposed patch

> +    KURL url = this->url();
> +    NSURL* nsURL;
> +    if (m_responseContentDispositionEncodingFallbackArray.isEmpty())
> +        nsURL = url;
> +    else {
> +        CString urlString = TextEncoding(m_responseContentDispositionEncodingFallbackArray[0]).encode(url.string().characters(), url.string().length(), URLEncodedEntitiesForUnencodables);
> +        RetainPtr<CFURLRef> cfURL = CFURLCreateAbsoluteURLWithBytes(kCFAllocatorDefault, (const UInt8*)urlString.data(), urlString.length(), CFStringConvertIANACharSetNameToEncoding(m_responseContentDispositionEncodingFallbackArray[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
Comment 3 Alexey Proskuryakov 2009-04-20 14:05:21 PDT
Created attachment 29624 [details]
proposed patch

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

Oops! Sorry, this code is completely unrelated, and certainly not ready for review. Submitting a hopefully clean patch...
Comment 4 Darin Adler 2009-04-20 14:18:44 PDT
Comment on attachment 29624 [details]
proposed patch

Comment 5 Alexey Proskuryakov 2009-04-21 01:26:32 PDT
Committed <http://trac.webkit.org/changeset/42700>.