Bug 99084 - [Chromium-Android] Exception when the layout test driver is stopped
Summary: [Chromium-Android] Exception when the layout test driver is stopped
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords: NRWT
Depends on:
Blocks:
 
Reported: 2012-10-11 10:31 PDT by Xianzhu Wang
Modified: 2012-10-11 15:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.49 KB, patch)
2012-10-11 10:47 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (2.52 KB, patch)
2012-10-11 13:41 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2012-10-11 10:31:33 PDT
When the driver is stopped (when it finishes running all tests or some test timeout/crash), following exception raises and layout test script quits:

ValueError raised: I/O operation on closed file
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 110, in run
      unexpected_result_count = manager.run(args)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 411, in run
      self._run_tests(self._test_names, result_summary, int(self._options.child_processes))
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 464, in _run_tests
      return self._runner.run_tests(test_inputs, self._expectations, result_summary, num_workers, needs_http, needs_websockets, self._retrying)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 142, in run_tests
      pool.run(('test_list', shard.name, shard.test_inputs) for shard in all_shards)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/common/message_pool.py", line 97, in run
      self.wait()
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/common/message_pool.py", line 127, in wait
      self._workers[0].run()
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/common/message_pool.py", line 267, in run
      self._raise(sys.exc_info())
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/common/message_pool.py", line 255, in run
      worker.handle(message.name, message.src, *message.args)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 304, in handle
      self._run_test(test_input)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 335, in _run_test
      self._clean_up_after_test(test_input, result)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 381, in _clean_up_after_test
      self._kill_driver()
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 367, in _kill_driver
      driver.stop()
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py", line 645, in stop
      self._read_stdout_process.kill()
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/port/server_process.py", line 362, in kill
      self.stop(0.0)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/port/server_process.py", line 352, in stop
      self._wait_for_data_and_update_buffers_using_select(now, stopping=True)
    File "/usr/local/google/home/wangxianzhu/webkit/Tools/Scripts/webkitpy/layout_tests/port/server_process.py", line 229, in _wait_for_data_and_update_buffers_using_select
      out_fd = self._proc.stdout.fileno()
Exception ValueError: ValueError('I/O operation on closed file',) in <bound method ChromiumAndroidDriver.__del__ of <webkitpy.layout_tests.port.chromium_android.ChromiumAndroidDriver object at 0x37e8f10>> ignored
Comment 1 Xianzhu Wang 2012-10-11 10:47:10 PDT
Created attachment 168254 [details]
Patch
Comment 2 Dirk Pranke 2012-10-11 12:48:56 PDT
Comment on attachment 168254 [details]
Patch

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

This seems reasonable. One

> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:334
>          self._proc.stdin.close()

Can you add a "self._proc.stdin = None" here as well? I think w/o this we can end up trying to close the handle twice in self._reset() on line 357 (this isn't your fault, but I noticed this while reviewing the patch).
Comment 3 Xianzhu Wang 2012-10-11 13:18:36 PDT
(In reply to comment #2)
> (From update of attachment 168254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168254&action=review
> 
> This seems reasonable. One
> 
> > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:334
> >          self._proc.stdin.close()
> 
> Can you add a "self._proc.stdin = None" here as well? I think w/o this we can end up trying to close the handle twice in self._reset() on line 357 (this isn't your fault, but I noticed this while reviewing the patch).

The problem seems more complex than I thought after I noticed this line. For all ports in the later _wait_for_data_and_update_buffers_using_select() stdin.fileno() will be called while stdin has been closed. On chromium-android, this causes the exception (though the situation is a bit different).

I wonder what the intent of the line is. Can we just remove it?
Comment 4 Dirk Pranke 2012-10-11 13:28:59 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 168254 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=168254&action=review
> > 
> > This seems reasonable. One
> > 
> > > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:334
> > >          self._proc.stdin.close()
> > 
> > Can you add a "self._proc.stdin = None" here as well? I think w/o this we can end up trying to close the handle twice in self._reset() on line 357 (this isn't your fault, but I noticed this while reviewing the patch).
> 
> The problem seems more complex than I thought after I noticed this line. For all ports in the later _wait_for_data_and_update_buffers_using_select() stdin.fileno() will be called while stdin has been closed. On chromium-android, this causes the exception (though the situation is a bit different).
> 
> I wonder what the intent of the line is. Can we just remove it?

closing stdin is the way DRT is supposed to be notified to shut down cleanly, so, no.

But, _wait_for_data doesn't access stdin? Did you misread something?
Comment 5 Xianzhu Wang 2012-10-11 13:39:17 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 168254 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=168254&action=review
> > > 
> > > This seems reasonable. One
> > > 
> > > > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:334
> > > >          self._proc.stdin.close()
> > > 
> > > Can you add a "self._proc.stdin = None" here as well? I think w/o this we can end up trying to close the handle twice in self._reset() on line 357 (this isn't your fault, but I noticed this while reviewing the patch).
> > 
> > The problem seems more complex than I thought after I noticed this line. For all ports in the later _wait_for_data_and_update_buffers_using_select() stdin.fileno() will be called while stdin has been closed. On chromium-android, this causes the exception (though the situation is a bit different).
> > 
> > I wonder what the intent of the line is. Can we just remove it?
> 
> closing stdin is the way DRT is supposed to be notified to shut down cleanly, so, no.
> 
> But, _wait_for_data doesn't access stdin? Did you misread something?

Yes, I mixed up stdin and stdout :)
Comment 6 Xianzhu Wang 2012-10-11 13:41:40 PDT
Created attachment 168270 [details]
Patch
Comment 7 WebKit Review Bot 2012-10-11 15:25:51 PDT
Comment on attachment 168270 [details]
Patch

Clearing flags on attachment: 168270

Committed r131099: <http://trac.webkit.org/changeset/131099>
Comment 8 WebKit Review Bot 2012-10-11 15:25:54 PDT
All reviewed patches have been landed.  Closing bug.