Bug 82386 – [Win] Some Blob tests crash in CFNetwork in advanceCurrentStream(FormStreamFields*)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

The code in FormDataStreamCFNet.cpp relies on CFNetwork body parts implementation, which doesn't know about blobs. We need the same lower level approach we took in FormDataStreamMac.mm originally.

<rdar://problem/11121501>

[Alexey Proskuryakov]

Created attachment 134147 [details]
proposed fix

[WebKit Review Bot]

Attachment 134147 [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/FormDataStreamCFNet.cpp:60:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:72:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:72:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:104:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:106:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:159:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:189:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:287:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:337:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:400:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 11 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134147 [details]
proposed fix

Attachment 134147 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12148668

[Alexey Proskuryakov]

Created attachment 134351 [details]
build fix attempt

[WebKit Review Bot]

Attachment 134351 [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/FormDataStreamCFNet.cpp:84:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:84:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:85:  The parameter name "alloc" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:127:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:129:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:182:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:212:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:310:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:422:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 11 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134351 [details]
build fix attempt

Attachment 134351 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12156252

[Alexey Proskuryakov]

Created attachment 134366 [details]
more build fix attempt

[WebKit Review Bot]

Attachment 134366 [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/FormDataStreamCFNet.cpp:84:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:84:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:85:  The parameter name "alloc" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134366 [details]
more build fix attempt

Attachment 134366 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12159246

[Alexey Proskuryakov]

Created attachment 134386 [details]
more build fixing

[WebKit Review Bot]

Attachment 134386 [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/FormDataStreamCFNet.cpp:84:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:84:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:85:  The parameter name "alloc" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134386 [details]
more build fixing

Attachment 134386 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12152403

[Alexey Proskuryakov]

Created attachment 134414 [details]
moar!

[WebKit Review Bot]

Attachment 134414 [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/FormDataStreamCFNet.cpp:84:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:84:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:85:  The parameter name "alloc" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134414 [details]
moar!

Attachment 134414 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12152435

[Alexey Proskuryakov]

Created attachment 134428 [details]
moarrrrr!

Wow, so we've been building with broken fileSystemRepresentation all the time.

[WebKit Review Bot]

Attachment 134428 [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/FormDataStreamCFNet.cpp:84:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:84:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:85:  The parameter name "alloc" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "stream" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:95:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 134428 [details]
moarrrrr!

Plz to be fixing the style bot gripes

[Alexey Proskuryakov]

Comment on attachment 134428 [details]
moarrrrr!

Actually, I think that we're keeping all argument names in CF-style code generally.

[WebKit Review Bot]

Comment on attachment 134428 [details]
moarrrrr!

Clearing flags on attachment: 134428

Committed r112482: <http://trac.webkit.org/changeset/112482>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

Attempted Windows build fix in http://trac.webkit.org/changeset/112498.

Alexey, you should probably look it over to make sure it is right. I tried to go by the error codes in http://msdn.microsoft.com/en-us/library/5814770t.aspx.

[Alexey Proskuryakov]

What were the build failures? As you can see, EWS built this patch successfully.

It is difficult to tell whether using a different error code will affect anything. CFStreamError structure contains a domain and an error code. Domain for OSStatus codes like fnfErr is kCFStreamErrorDomainMacOSStatus (numerically, it's 2), but the existing code doesn't set the domain either. This CFStreamError code is not used immediately, but it's stored inside CFReamStream for later use.

I think that it would be better to find a fix that doesn't involve using an unexpected error code. Is there any header in your setup that defines fnfErr? Why does it not have the CoreServices framework?

[Jessie Berlin]

(In reply to comment #23)
> What were the build failures? As you can see, EWS built this patch successfully.

6>..\platform\network\cf\FormDataStreamCFNet.cpp(48) : fatal error C1083: Cannot open include file: 'CoreServices/CoreServices.h': No such file or directory

When I resolved that by #defining it out on Windows, I ran into fnfErr not found.

> 
> It is difficult to tell whether using a different error code will affect anything. CFStreamError structure contains a domain and an error code. Domain for OSStatus codes like fnfErr is kCFStreamErrorDomainMacOSStatus (numerically, it's 2), but the existing code doesn't set the domain either. This CFStreamError code is not used immediately, but it's stored inside CFReamStream for later use.
> 
> I think that it would be better to find a fix that doesn't involve using an unexpected error code. Is there any header in your setup that defines fnfErr? Why does it not have the CoreServices framework?

There was no header on Windows that defines fnfErr.

Also, you will notice that all uses of #include <CoreServices/CoreServices.h> are not defined on Windows.  I don't see that header on Windows, and I am confused as to how this passed EWS.