Bug 61345

Summary: lazily init ResourceResponse suggested filename.
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, aroben, ggaren, slewis, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 61445    
Bug Blocks:    
Attachments:
Description Flags
patch
slewis: review-
patch
ggaren: review+
patch ggaren: review+

Description Stephanie Lewis 2011-05-23 23:21:07 PDT
lazily init ResourceResponse suggested filename.   Reading it in causes us to read in and parse a plist so put off setting it until we need it.  Part of <rdar://problem/8675177>
Comment 1 Stephanie Lewis 2011-05-23 23:31:48 PDT
Created attachment 94572 [details]
patch
Comment 2 WebKit Review Bot 2011-05-23 23:34:11 PDT
Attachment 94572 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:136:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Geoffrey Garen 2011-05-24 14:17:38 PDT
Stephanie, why did you r- this?
Comment 4 Stephanie Lewis 2011-05-24 15:08:48 PDT
Created attachment 94700 [details]
patch

Bad window copy/paste.  New patch attached
Comment 5 Alexey Proskuryakov 2011-05-24 15:13:50 PDT
Comment on attachment 94700 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94700&action=review

> Source/WebCore/platform/network/ResourceResponseBase.h:136
>          Uninitialized,
>          CommonFieldsOnly,
> -        AllFields
> +        AllHeaderFields,
> +        SuggestedFilename

If I understand the patch correctly, SuggestedFilename actually initializes everything. I think that it should be named accordingly.
Comment 6 Geoffrey Garen 2011-05-24 15:21:41 PDT
Comment on attachment 94700 [details]
patch

I agree with ap -- better names would help indicate that each successive level of the enum is inclusive of previous levels. I'd suggest:

        CommonFields,
        CommonAndUncommonFields,
        AllFields

r=me with that change
Comment 7 Stephanie Lewis 2011-05-24 16:48:21 PDT
committed http://trac.webkit.org/changeset/87229 with suggested changes.
Comment 8 Stephanie Lewis 2011-05-24 16:52:26 PDT
<rdar://problem/9497562>
Comment 9 Adam Roben (:aroben) 2011-05-25 08:13:47 PDT
This caused lots of tests to start crashing on Windows. See bug 61439.
Comment 10 Geoffrey Garen 2011-05-25 12:09:38 PDT
Should we roll out the change, or is there a better way?
Comment 11 Adam Roben (:aroben) 2011-05-25 12:23:42 PDT
I rolled out the change in r87301, since fixing the bug correctly seems a little tricky. See bug 61439 for details.
Comment 12 Stephanie Lewis 2011-05-26 11:13:55 PDT
Copying some comments over from the bug about the crashes

Comment #7 From Brady Eidson 2011-05-25 08:47:29 PST (-) [reply]
(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.


Comment #10 From Stephanie Lewis 2011-05-25 15:48:51 PST (-) [reply]
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.


Comment #11 From Stephanie Lewis 2011-05-25 16:25:07 PST (-) [reply]
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.
Comment 13 Stephanie Lewis 2011-05-26 11:27:28 PDT
Created attachment 95009 [details]
patch
Comment 14 Geoffrey Garen 2011-05-26 15:42:08 PDT
Comment on attachment 95009 [details]
patch

r=me
Comment 15 Stephanie Lewis 2011-05-26 18:41:09 PDT
committed http://trac.webkit.org/projects/webkit/changeset/87460