Bug 99084 – [Chromium-Android] Exception when the layout test driver is stopped

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Xianzhu Wang]

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

[Xianzhu Wang]

Created attachment 168254 [details]
Patch

[Dirk Pranke]

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

[Xianzhu Wang]

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

[Dirk Pranke]

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

[Xianzhu Wang]

(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 :)

[Xianzhu Wang]

Created attachment 168270 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168270 [details]
Patch

Clearing flags on attachment: 168270

Committed r131099: <http://trac.webkit.org/changeset/131099>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.