[webkit-reviews] review denied: [Bug 15743] run-webkit-tests hangs when WebCore tries to log too much : [Attachment 35030] Patch v1

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 15743: run-webkit-tests hangs when WebCore tries to log too much
https://bugs.webkit.org/show_bug.cgi?id=15743

Attachment 35030: Patch v1
https://bugs.webkit.org/attachment.cgi?id=35030&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
I do think the original issue may have been blocked write(2) calls due to too
much STDERR output.  Do you have an easy way to reproduce this?  Can you just
add a "fprintf(stderr, "Blah\n");" to a function in WebKit that's called a lot
to see the bad behavior?

Overall, this change looks good, but I have a couple of concerns.

> @@ -1984,14 +1985,16 @@ sub readFromDumpToolWithTimer(*;$)
>	       last;
>	   }
>  
> -	   my $line = readline($fh);
> -	   if (!defined($line)) {
> +	   # Once we've seen the EOF, we must not read anymore.
> +	   my $lineIn = readline($fhIn) unless $haveSeenEof;
> +	   my $lineError = readline($fhError);

It's an interesting strategy to read a line of STDERR with a line of STDOUT
from DumpRenderTree, but what guarantees that all of STDERR is read before
going to the next test?  Would reading all STDERR each time work as well?

    my $lineError = <$fhError>;

> @@ -2001,20 +2004,29 @@ sub readFromDumpToolWithTimer(*;$)
>	   }
>  
>	   $timeOfLastSuccessfulRead = time;
> -	   
> -	   if (!$haveSeenContentType && $line =~ /^Content-Type: (\S+)$/) {
> -	       $mimeType = $1;
> -	       $haveSeenContentType = 1;
> -	       next;
> +
> +	   if (defined($lineIn)) {
> +	       if (!$haveSeenContentType && $lineIn =~ /^Content-Type: (\S+)$/)
{
> +		   $mimeType = $1;
> +		   $haveSeenContentType = 1;
> +		   next;
> +	       }
> +
> +	       if ($lineIn =~ /#EOF/) {
> +		   $haveSeenEof = 1;
> +		   next;
> +	       }
> +
> +	       push @output, $lineIn;
>	   }
> -	   last  if ($line =~ /#EOF/);
> -	   
> -	   push @output, $line;
> +	   push @error, $lineError if defined($lineError);
>      }

If any STDERR is printed at the same time as the "Content-Type" line or the
"#EOF" line, it appears that whatever is in $lineError will be discarded.  I
think the logic here should be restructured to use if () {} elif () {} else {}
within if (defined($lineIn)) so that $lineError always gets appended to @error
if it's defined.

r- to address the above issues.