Bug 6272 – XMLHttpRequest freezes on getting a missing document with overridden Content-Length

• Component: XML
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Sync GET requests for missing documents freeze Safari if Content-Length header was set. When Safari un-
freezes on timeout, timeout, the request state is invalid (bogus status).

Steps to reproduce: open the attached test case.

[Alexey Proskuryakov]

Created attachment 5342 [details]
test case

[Alexey Proskuryakov]

rdar://4409209 for NSURLConnection bug

[Alexey Proskuryakov]

Created attachment 5692 [details]
proposed fix

No regression test, because the actual behavior (silently ignoring vs. throwing
an exception) is waiting to be defined in bug 6212.

[Darin Adler]

I'm having trouble understanding this. The code prevents setting a Content-Length header in the request. 
which means that we send a request that will look "incomplete" to some servers. If the reason we freeze is 
that the server doesn't reply, then I don't think that we can blame the freeze on the ability to set that 
header. Other servers might not respond to a reqeust for other reasons.

So it seems like we should fix that "request state is invalid (bogus status)" issue you mentioned first, 
before we disallow this header.

[Darin Adler]

Comment on attachment 5692 [details]
proposed fix

I don't think the right way to fix this bug is to disallow this header. We
might need to disallow setting the header for other reasons (compatibility with
other browsers' implementations perhaps), but the loader should handles cases
where the server doesn't respond because it doesn't like our request, and
that's not specific to this header.

[Alexey Proskuryakov]

Actually, I've misinterpreted what I saw in tcpdump (thought that the server replied correctly, but 
NSURLConnection didn't handle the response).

Thanks for catching this!

[Alexey Proskuryakov]

Created attachment 5699 [details]
tests that use a bad local port

WinIE throws an exception in sync case, but misbehaves in async case (reaches
Completed state with status 12029).

[Alexey Proskuryakov]

See <http://www.w3.org/2006/webapi/track/issues/101> for a discussion of expected behavior (not decided upon yet).

[Alexey Proskuryakov]

Created attachment 12416 [details]
proposed fix

Firefox trunk forbids a different set of headers than the draft says, so there may be some issues to discuss with the WG if we don't like what's in the draft. See <http://lxr.mozilla.org/seamonkey/source/content/base/src/nsXMLHttpRequest.cpp> (currently, it's "host", "content-length", "transfer-encoding", "via", "upgrade").

[Darin Adler]

Comment on attachment 12416 [details]
proposed fix

+        void loadResourceSynchronously(const ResourceRequest& request, ResourceError& error, ResourceResponse& r, Vector<char>& data);

Really should leave out those parameter names ("request", "error", "r").

It's a real shame that canSetRequestHeader has to call lower on each string passed into it. Because of that, we lose much if not all of the performance advantage of using AtomicString. I could see just using a HashSet<StringImpl*, CaseInsensitiveHash<StringImpl*> > instead.

I think it's a little ugly and unnecessarily costly to have all those AtomicString globals. Instead you could just have a little helper function that makes an atomic string, adds it to the set and ref's the atomic string's impl. Like this:

    static void addString(HashSet<AtomicStringImpl*>& set, const char* string)
    {
        AtomicString atomicString(string);
        set.add(atomicString.impl());
        atomicString.impl()->ref();
    }

    ...

    if (forbiddenHeaders.isEmpty()) {
        addString(forbiddenHeaders, "accept-charset");
        ...

Where did the list of forbidden headers come from? How are you sure you have the right list?

+        // FIXME: Report a warning to the user.

We could put something on the console there. Why not do that, instead of having the FIXME?

Why not add SECURITY_ERR with this patch? Is there a risk that if after add it to DOM Core, the value will change?

There are enough questions here that I'm going to say review-, but I really approve of everything in here!

[Darin Adler]

Comment on attachment 12416 [details]
proposed fix

Sorry, I meant review-.

And one more comment: I think we may need a Dashboard quirk to not raise an exception in one or both of these cases, and instead fail silently.

[Alexey Proskuryakov]

Created attachment 12426 [details]
proposed fix

> Really should leave out those parameter names ("request", "error", "r").

  Done.

> I could see just using a HashSet<StringImpl*, CaseInsensitiveHash<
> StringImpl*> > instead.

  Changed (used String instead of StringImpl* though).

> Where did the list of forbidden headers come from? How are you sure you have
> the right list?

  They come from the XHR spec draft, plus Via from the Firefox implementation. As mentioned in comment 9, we may want to discuss this list on our own.

> We could put something on the console there. Why not do that, instead of having
> the FIXME?

  My thought was that we'd need to know the script line and URL to properly log the warning. But this is a general problem elsewhere in WebCore, and logging "something" is indeed better. Added.

> Why not add SECURITY_ERR with this patch? Is there a risk that if after add it
> to DOM Core, the value will change?

  Yes, this was my concern. In Hixie's proposal, it's just has the next unused code (18), so a confict seems possible.

> And one more comment: I think we may need a Dashboard quirk to not raise an
> exception in one or both of these cases, and instead fail silently.

  I very much hope that it won't be needed. Unlike previous quirks that deal with coding mistakes, this would create a genuine difference in runtime error handling. Please note that blocking dangerous headers is silent per the standard anyway, so there's just the NETWORK_ERR case to worry about (admittedly, a rather significant one).

[Darin Adler]

Comment on attachment 12426 [details]
proposed fix

If the code is > XMLHttpRequestExceptionOffset, but < NETWORK_ERR, it looks like the index into the nameTable is going to underflow. We need to add a check for values < 0 to the line that gets the value from the nameTable or somehow ensure the number is not negative.

r=me (but would be nice to fix that)

[Alexey Proskuryakov]

Committed revision 18863.

(In reply to comment #13)
> We need to add a check
> for values < 0 to the line that gets the value from the nameTable or somehow
> ensure the number is not negative.

  Fixed (this used to be an issue with XPath, too).