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

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

Summary: [Win] Some Blob tests crash in CFNetwork in advanceCurrentStream(FormStreamFi...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebCore Misc. (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Windows 7

Importance:	P1 Normal
Assignee:	Alexey Proskuryakov

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2012-03-27 15:03 PDT by Alexey Proskuryakov
Modified:	2012-03-29 12:37 PDT (History)
CC List:	7 users (show)

See Also:

Attachments
proposed fix (38.31 KB, patch) 2012-03-27 15:08 PDT, Alexey Proskuryakov	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
build fix attempt (40.24 KB, patch) 2012-03-28 11:31 PDT, Alexey Proskuryakov	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
more build fix attempt (40.20 KB, patch) 2012-03-28 12:21 PDT, Alexey Proskuryakov	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
more build fixing (41.03 KB, patch) 2012-03-28 13:44 PDT, Alexey Proskuryakov	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
moar! (41.69 KB, patch) 2012-03-28 15:08 PDT, Alexey Proskuryakov	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
moarrrrr! (42.37 KB, patch) 2012-03-28 15:42 PDT, Alexey Proskuryakov	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Alexey Proskuryakov 2012-03-27 15:03:10 PDT

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>

Comment 1 Alexey Proskuryakov 2012-03-27 15:08:15 PDT

Created attachment 134147 [details]
proposed fix

Comment 2 WebKit Review Bot 2012-03-27 15:22:48 PDT

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.

Comment 3 Build Bot 2012-03-27 15:34:41 PDT

Comment on attachment 134147 [details]
proposed fix

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

Comment 4 Alexey Proskuryakov 2012-03-28 11:31:29 PDT

Created attachment 134351 [details]
build fix attempt

Comment 5 WebKit Review Bot 2012-03-28 11:35:48 PDT

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.

Comment 6 Build Bot 2012-03-28 12:13:40 PDT

Comment on attachment 134351 [details]
build fix attempt

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

Comment 7 Alexey Proskuryakov 2012-03-28 12:21:23 PDT

Created attachment 134366 [details]
more build fix attempt

Comment 8 WebKit Review Bot 2012-03-28 12:23:55 PDT

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.

Comment 9 Build Bot 2012-03-28 12:47:58 PDT

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

Comment 10 Alexey Proskuryakov 2012-03-28 13:44:18 PDT

Created attachment 134386 [details]
more build fixing

Comment 11 WebKit Review Bot 2012-03-28 13:49:18 PDT

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.

Comment 12 Build Bot 2012-03-28 13:57:09 PDT

Comment on attachment 134386 [details]
more build fixing

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

Comment 13 Alexey Proskuryakov 2012-03-28 15:08:42 PDT

Created attachment 134414 [details]
moar!

Comment 14 WebKit Review Bot 2012-03-28 15:13:38 PDT

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.

Comment 15 Build Bot 2012-03-28 15:35:26 PDT

Comment on attachment 134414 [details]
moar!

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

Comment 16 Alexey Proskuryakov 2012-03-28 15:42:26 PDT

Created attachment 134428 [details]
moarrrrr!

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

Comment 17 WebKit Review Bot 2012-03-28 15:45:32 PDT

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.

Comment 18 Brady Eidson 2012-03-28 17:03:03 PDT

Comment on attachment 134428 [details]
moarrrrr!

Plz to be fixing the style bot gripes

Comment 19 Alexey Proskuryakov 2012-03-28 17:07:35 PDT

Comment on attachment 134428 [details]
moarrrrr!

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

Comment 20 WebKit Review Bot 2012-03-28 17:59:00 PDT

Comment on attachment 134428 [details]
moarrrrr!

Clearing flags on attachment: 134428

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

Comment 21 WebKit Review Bot 2012-03-28 17:59:06 PDT

All reviewed patches have been landed.  Closing bug.

Comment 22 Jessie Berlin 2012-03-28 21:09:53 PDT

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.

Comment 23 Alexey Proskuryakov 2012-03-28 21:46:38 PDT

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?

Comment 24 Jessie Berlin 2012-03-29 08:32:39 PDT

(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.