Summary: | REGRESSION (r87229): Lots of tests crashing in CFNetwork!URLResponse::createFilenameFromResponseHeaders on Windows XP | ||
---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> |
Component: | Platform | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | ap, beidson, ggaren, slewis, webkit.review.bot |
Priority: | P2 | Keywords: | InRadar, LayoutTestFailure, MakingBotsRed, PlatformOnly, Regression |
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | Windows XP |
Description
Adam Roben (:aroben)
2011-05-25 08:13:25 PDT
Most of the crashing tests are workers-related. The crashing line in WebKit is this one, in ResourceResponse::performLazyInit: RetainPtr<CFStringRef> suggestedFilename(AdoptCF, CFURLResponseCopySuggestedFilename(m_cfResponse.get())); m_cfResponse is null. 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. 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". 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. (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. *** Bug 61445 has been marked as a duplicate of this bug. *** 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. 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. |