Summary: | [Chromium-Android] Exception when the layout test driver is stopped | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||||
Component: | Tools / Tests | Assignee: | Xianzhu Wang <wangxianzhu> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dpranke, ojan, peter, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | NRWT | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Xianzhu Wang
2012-10-11 10:31:33 PDT
Created attachment 168254 [details]
Patch
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). (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? (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? (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 :) Created attachment 168270 [details]
Patch
Comment on attachment 168270 [details] Patch Clearing flags on attachment: 168270 Committed r131099: <http://trac.webkit.org/changeset/131099> All reviewed patches have been landed. Closing bug. |