REGRESSION (r87229): Lots of tests crashing in CFNetwork!URLResponse::createFilenameFromResponseHeaders on Windows XP

[Adam Roben (:aroben)]

Lots of tests have been crashing in CFNetwork!URLResponse::createFilenameFromResponseHeaders on Windows XP since r87229.

http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/28950 did not crash
http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/28952 did crash

Here's one particular backtrace:

http://build.webkit.org/results/Windows%20XP%20Debug%20%28Tests%29/r87293%20(28975)/fast/workers/storage/execute-sql-args-worker-crash-log.txt


0cf9eefc 007e5d7a 0cf9f538 0cf9f024 0cf9f538 CFNetwork!URLResponse::createFilenameFromResponseHeaders
0cf9f018 013f1592 00000000 0cf9f63c 0cf9f598 CFNetwork!URLResponse::copySuggestedFilename+0x1f
0cf9f538 013fc78a 00000003 0b9701c0 0cf9f558 WebKit!WebCore::ResourceResponse::platformLazyInit+0x5f2 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\platform\network\cf\resourceresponsecfnet.cpp @ 133]
0cf9f548 013faf88 00000003 0b9701c0 0cf9f588 WebKit!WebCore::ResourceResponseBase::lazyInit+0x1a [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\platform\network\resourceresponsebase.cpp @ 564]
0cf9f558 013fa8c9 0c636174 0b9701c0 cccccccc WebKit!WebCore::ResourceResponseBase::setSuggestedFilename+0x18 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\platform\network\resourceresponsebase.cpp @ 216]
0cf9f588 021539fd 0cf9f59c 0c636130 0cf9f5dc WebKit!WebCore::ResourceResponseBase::adopt+0xf9 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\platform\network\resourceresponsebase.cpp @ 108]
0cf9f5b0 02155b9a 0c0682a8 0c5768d0 00000000 WebKit!WebCore::workerContextDidReceiveResponse+0x8d [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\loader\workerthreadableloader.cpp @ 183]
0cf9f5d0 0203af56 0c0682a8 00001524 0c4bd888 WebKit!WebCore::CrossThreadTask2<WTF::PassRefPtr<WebCore::ThreadableLoaderClientWrapper>,WTF::RefPtr<WebCore::ThreadableLoaderClientWrapper>,WTF::PassOwnPtr<WebCore::CrossThreadResourceResponseData>,WTF::PassOwnPtr<WebCore::CrossThreadResourceResponseData> >::performTask+0x4a [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\crossthreadtask.h @ 112]
0cf9f5f0 0203acdc 0c0682a8 0cf9f678 0cf9f6d4 WebKit!WebCore::WorkerRunLoop::Task::performTask+0x76 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerrunloop.cpp @ 199]
0cf9f63c 0203aab3 0c0682a8 0cf9f65c 0cf9f6bc WebKit!WebCore::WorkerRunLoop::runInMode+0x1ac [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerrunloop.cpp @ 164]
0cf9f678 02153010 0c0682a8 0cf9f6b0 0cf9f704 WebKit!WebCore::WorkerRunLoop::runInMode+0x43 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerrunloop.cpp @ 142]
0cf9f6bc 0203ed3e 0c0682a8 0b87fb38 0cf9f78c WebKit!WebCore::WorkerThreadableLoader::loadResourceSynchronously+0xc0 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\loader\workerthreadableloader.cpp @ 80]
0cf9f704 01d395d3 0c0682a8 0c4c21c0 00000002 WebKit!WebCore::WorkerScriptLoader::loadSynchronously+0xee [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerscriptloader.cpp @ 69]
0cf9f878 01d67f18 0cf9f8d4 0cf9f8c4 0cf9f8a4 WebKit!WebCore::WorkerContext::importScripts+0x143 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workercontext.cpp @ 252]
0cf9f894 01369d1e 0cf9f8d4 0cf9f8c4 0d0b0040 WebKit!WebCore::DedicatedWorkerContext::importScripts+0x28 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\dedicatedworkercontext.cpp @ 75]
0cf9f8e8 01a14819 0cf9f8fc 0d0b00b0 00000200 WebKit!WebCore::JSWorkerContext::importScripts+0xfe [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\bindings\js\jsworkercontextcustom.cpp @ 119]
0cf9f918 0cfa1799 0cf9f930 00bb35ab 0cf9f978 WebKit!WebCore::jsWorkerContextPrototypeFunctionImportScripts+0x129 [c:\cygwin\home\buildbot\slave\win-debug\build\webkitbuild\debug\obj\webcore\derivedsources\jsworkercontext.cpp @ 519]
WARNING: Frame IP not in any known module. Following frames may be wrong.
0cf9f978 00bd394b 0cfa0568 0c5e2c0c 0d0b00b0 0xcfa1799
0cf9f9bc 00bcfeb9 0cf9f9f8 0c5e2c0c 0d0b0040 JavaScriptCore!JSC::JITCode::execute+0x4b [c:\cygwin\home\buildbot\slave\win-debug\build\source\javascriptcore\jit\jitcode.h @ 77]
0cf9fa94 00a52fe9 0cf9fb28 0d0001a8 0d020670 JavaScriptCore!JSC::Interpreter::executeCall+0x3c9 [c:\cygwin\home\buildbot\slave\win-debug\build\source\javascriptcore\interpreter\interpreter.cpp @ 852]
0cf9fac0 01359b19 0cf9fb28 0d0001a8 0d020670 JavaScriptCore!JSC::call+0x89 [c:\cygwin\home\buildbot\slave\win-debug\build\source\javascriptcore\runtime\calldata.cpp @ 38]
0cf9fca8 01543bd5 0c587008 01543bd5 0c0682a8 WebKit!WebCore::JSEventListener::handleEvent+0x4f9 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\bindings\js\jseventlistener.cpp @ 128]
0cf9fd04 01543a64 0c56e5d0 0c0683b8 0c69d390 WebKit!WebCore::EventTarget::fireEventListeners+0x105 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\eventtarget.cpp @ 389]
0cf9fd60 015438de 0c56e5d0 cccccccc cccccccc WebKit!WebCore::EventTarget::fireEventListeners+0x144 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\eventtarget.cpp @ 360]
0cf9fd78 0206e36c 0c56e5d0 0cf9fe34 0cf9fdd4 WebKit!WebCore::EventTarget::dispatchEvent+0x6e [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\eventtarget.cpp @ 340]
0cf9fdc8 0203af56 0c0682a8 00001524 0c74a340 WebKit!WebCore::MessageWorkerContextTask::performTask+0x11c [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workermessagingproxy.cpp @ 67]
0cf9fde8 0203acdc 0c0682a8 0cf9fe70 0cf9fe88 WebKit!WebCore::WorkerRunLoop::Task::performTask+0x76 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerrunloop.cpp @ 199]
0cf9fe34 0203a9f4 0c0682a8 0cf9fe54 0cf9ff10 WebKit!WebCore::WorkerRunLoop::runInMode+0x1ac [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerrunloop.cpp @ 164]
0cf9fe70 0203dd45 0c0682a8 0c3a1aa0 0cf9fe90 WebKit!WebCore::WorkerRunLoop::run+0x54 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerrunloop.cpp @ 134]
0cf9fe80 021640bb 0cf9fe98 0c3a1aa0 0cf9ff10 WebKit!WebCore::WorkerThread::runEventLoop+0x25 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerthread.cpp @ 164]
0cf9fe90 0203dc42 0012ef44 0cf9ff24 02988320 WebKit!WebCore::DedicatedWorkerThread::runEventLoop+0x4b [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\dedicatedworkerthread.cpp @ 67]
0cf9ff10 0203dafb 0cf9ff4c 00c7c9c5 0c3a1aa0 WebKit!WebCore::WorkerThread::workerThread+0x132 [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerthread.cpp @ 141]
0cf9ff18 00c7c9c5 0c3a1aa0 0cf9ff58 0c5cb9d0 WebKit!WebCore::WorkerThread::workerThreadStart+0xb [c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\workers\workerthread.cpp @ 119]
0cf9ff4c 00c6e5c9 0c5cb9d0 7c90e920 cccccccc JavaScriptCore!WTF::threadEntryPoint+0x95 [c:\cygwin\home\buildbot\slave\win-debug\build\source\javascriptcore\wtf\threading.cpp @ 67]
0cf9ff74 781329bb 0c56d468 52f06d93 0012ef44 JavaScriptCore!WTF::wtfThreadEntryPoint+0x59 [c:\cygwin\home\buildbot\slave\win-debug\build\source\javascriptcore\wtf\threadingwin.cpp @ 197]
0cf9ffac 78132a47 7c90e920 7c80b729 0c587008 MSVCR80!_callthreadstartex+0x1b [f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c @ 348]
0cf9ffb4 7c80b729 0c587008 0012ef44 7c90e920 MSVCR80!_threadstartex+0x66 [f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c @ 326]
0cf9ffec 00000000 781329e1 0c587008 00000000 kernel32!BaseThreadStart+0x37

[Adam Roben (:aroben)]

<rdar://problem/9500626>

[Adam Roben (:aroben)]

Most of the crashing tests are workers-related.

[Adam Roben (:aroben)]

The crashing line in WebKit is this one, in ResourceResponse::performLazyInit:

        RetainPtr<CFStringRef> suggestedFilename(AdoptCF, CFURLResponseCopySuggestedFilename(m_cfResponse.get()));

m_cfResponse is null.

[Adam Roben (:aroben)]

You can see the call to setSuggestedFilename that ends up crashing here:


PassOwnPtr<ResourceResponse> ResourceResponseBase::adopt(PassOwnPtr<CrossThreadResourceResponseData> data)
{
    OwnPtr<ResourceResponse> response = adoptPtr(new ResourceResponse);
    response->setURL(data->m_url);
    response->setMimeType(data->m_mimeType);
    response->setExpectedContentLength(data->m_expectedContentLength);
    response->setTextEncodingName(data->m_textEncodingName);
    response->setSuggestedFilename(data->m_suggestedFilename);

All of these setters end up calling lazyInit(). But setTextEncodingName sets m_isNull to false after calling lazyInit. So when setSuggestedFilename calls lazyInit, we no longer think the ResourceResponse is null, and thus assume that we have a non-null CFURLResponse and crash.

[Adam Roben (:aroben)]

There doesn't seem to be a clear definition of what m_isNull means. ResourceResponseCFNet and ResourceResponseMac seem to think that m_isNull == !m_cfURLResponse (or !m_nsURLResponse). But other code (e.g., setTextEncodingName) seems to think that it means "all fields are empty".

[Adam Roben (:aroben)]

m_isNull was added in <http://trac.webkit.org/changeset/18586>. At that point it seemed to represent whether the underlying NSURLResponse was null.

<http://trac.webkit.org/changeset/24372> seems to have introduced the other meaning, where it was possible for m_isNull to be false even though there was no underlying NSURLResponse.

[Brady Eidson]

(In reply to comment #6)
> m_isNull was added in <http://trac.webkit.org/changeset/18586>. At that point it seemed to represent whether the underlying NSURLResponse was null.
> 
> <http://trac.webkit.org/changeset/24372> seems to have introduced the other meaning, where it was possible for m_isNull to be false even though there was no underlying NSURLResponse.

24372's change seems like a mistake.  The original purpose of "m_isNull" is to track whether the platform response is null, and it's somewhat apparent the class most uses it that way internally - except for this exception.

When this is fixed, the patch should take the opportunity to rename m_isNull to m_isPlatformResponseNull so that type of mistake would be more obvious in the future.

[Adam Roben (:aroben)]

*** Bug 61445 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

I "fixed" this by rolling out r87229 (see bug 61445 for the rollout).

[Stephanie Lewis]

I agree with Brady in ResourceResponseBase m_inNull means some fields have been initialized.  In ResourceResponseCFNet and ResourceResponseMac m_isNull means that the platform is null.  Unfortunately ResourceResponseBase exposes m_isNull to other places with an isNull() call so the first definition has spread. 

It seems like it would be easier to make ResourceResponse adopt ResourceResponseBase's idea of isNull than to revert back to the original meaning, or we could add a flag to ResourceResponse to stand for platform is null.

[Stephanie Lewis]

m_isNull is only used in the platform sense in 4 places 
a) and b) when initializing a ResourceResponse.  I think it is Ok to set a ResourceResponse as not null if it has a platform base so nothing changes
c)  when returning the platform response.  This already checks whether the platform response exists so no change
d) when initializing.  Where I got into trouble.  There is even an ASSERT here that says if we are null we should also not have a response.  If it had gone the other way I would have caught the crash earlier :( (if we aren't null, do we have a platform response?)  Either way,  adding a check here for the platform response should fix the bug.

Working up a patch now.