[Webkit-unassigned] [Bug 159827] run-webkit-tests should trigger a spindump when WebContent process is unresponsive

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159827

--- Comment #53 from Jonathan Bedard <jbedard at apple.com> ---
(In reply to comment #51)
> (In reply to comment #50)
> > Comment on attachment 285120 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=285120&action=review
> > 
> > >> Tools/Scripts/webkitpy/port/ios.py:423
> > >> +            ], return_exit_code=True)
> > > 
> > > Notice that Executive.run_command() only throws a ScriptError exception if return_exit_code := False. It is good programming practice to limit the scope of a try-except to the minimum code that can raise an exception to help make it straightforward to debug the cause of an exception. In comment #25 I gave an example of one way we can write this code such that we limit the scope of the try-except block.
> > 
> > This perhaps brings up another question, then, about sample_file_path(...),
> > which calls filesystem.join(...).  In the original sample_process code,
> > finding the report was in the try catch block, which is why I kept it there
> > after adding spindump.  If filesystem.join(...) throws an exception (or
> > results_directory(), actually), what is our desired functionality? 
> 
> Filesystem.join() does not raise an exception because it turns around and
> calls os.path.join(). And I do not expect os.path.join() to raise an
> exception because its documentation,
> <https://docs.python.org/2/library/os.path.html>, as of 08/02 makes no
> mention that it raises an exception.
> 
> > I think for the time being, keeping the generation of the hang_report file path
> > inside the try-except block is prudent
> 
> Notice that the try-except block only catches a
> webkitpy.common.system.executive.ScriptError exception. That is, it does not
> catch a general-purpose Python Exception.
> 
> > , as the tests can continue if an
> > error happens while attempting to generate either version of the crash
> > report, and 
> 
> > it would seem unhelpful to be to have two try-except blocks in
> > this function. 
> > 
> 
> How did you come to the conclusion that we need to try-except blocks in this
> function? Notice that I have exactly one try-except block in the example I
> gave in comment #25.
> 
> As aforementioned above, the try-except block in sample_process() only
> catches a webkitpy.common.system.executive.ScriptError exception. The
> current code allows any other exception to bubble up. Maybe we should revise
> this code to be more resilient to other kinds of exceptions. If so, such
> consideration and work is more appropriate in a separate bug as the focus of
> this bug is to add an enhancement to our sampling machinery: use spindump if
> we can.

(In reply to comment #52)
> (In reply to comment #51)
> [...]
> > > it would seem unhelpful to be to have two try-except blocks in
> > > this function. 
> > > 
> > 
> > How did you come to the conclusion that we need to try-except blocks in this function?
> 
> This should read:
> 
> How did you come to the conclusion that we need two try-except blocks in
> this function?

Sorry, I think my comment was not very clear.

What I was saying is that I kept the hang_report assignment in the try-except block because it had been in it's previous iteration, and I wasn't quite sure what kind of exceptions (if any) are thrown by sample_file_path(...) (as you point out, probably none, even when including results_directory()).  Essentially, I was trying to keep the same structure as had existed previously, and taking sample_file_path out of the try-except would have broken that pattern.

Since it does seem like sample_file_path(...) won't be throwing any exceptions, I will change this in the re-factor.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160802/d7a42a54/attachment.html>