Bug 53656 – new-run-webkit-tests: split out thread stack logging code into a sharable module

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: split out thread stack logging code into a sharable module

[Dirk Pranke]

Created attachment 81023 [details]
Patch

[Dirk Pranke]

Created attachment 81334 [details]
add log_traceback()

[Eric Seidel (no email)]

I'm entertained that this hack is now an official tested module. :)

[Mihai Parparita]

Comment on attachment 81334 [details]
add log_traceback()

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

> Tools/ChangeLog:14
> +        * Scripts/webkitpy/common/system/stack_utils.py: Added.

webkitpy/common/thread may be a more appropriate location for this.

> Tools/Scripts/webkitpy/common/system/stack_utils.py:35
> +def log_wedged_thread(logger, name, id, msg=''):

Rename id to thread_id, so that it's more obvious what kind of ID this refers to?

Also, nothing about this method seems specific to wedged threads, log_thread_stack might be a better name.

> Tools/Scripts/webkitpy/common/system/stack_utils.py:45
> +def find_thread_stack(id):

Can this be called _find_thread_stack, to indicate that it's an internal-only method.

> Tools/Scripts/webkitpy/common/system/stack_utils.py:62
> +def log_stack(logger, stack):

Assuming you don't need this in your other patches, can this be called _log_stack?

> Tools/Scripts/webkitpy/common/system/stack_utils.py:63
> +    """Log a stack trace to log.error()."""

Doesn't actually log to log.error anymore.

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 81334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81334&action=review
> 
> > Tools/ChangeLog:14
> > +        * Scripts/webkitpy/common/system/stack_utils.py: Added.
> 
> webkitpy/common/thread may be a more appropriate location for this.
> 

it looks like common/thread has stuff that uses threads. It might be a better place for the message_broker classes if we moved them out of layout_tests. I think this stuff is more appropriately located with other system-specific code.

> > Tools/Scripts/webkitpy/common/system/stack_utils.py:35
> > +def log_wedged_thread(logger, name, id, msg=''):
> 
> Rename id to thread_id, so that it's more obvious what kind of ID this refers to?
>

Sure.
 
> Also, nothing about this method seems specific to wedged threads, log_thread_stack might be a better name.
>

Well, it does log the string "wedged" :). I'll pull that out as well.
 
> > Tools/Scripts/webkitpy/common/system/stack_utils.py:45
> > +def find_thread_stack(id):
> 
> Can this be called _find_thread_stack, to indicate that it's an internal-only method.
>

Sure.
 
> > Tools/Scripts/webkitpy/common/system/stack_utils.py:62
> > +def log_stack(logger, stack):
> 
> Assuming you don't need this in your other patches, can this be called _log_stack?
> 

Sure.

> > Tools/Scripts/webkitpy/common/system/stack_utils.py:63
> > +    """Log a stack trace to log.error()."""
> 
> Doesn't actually log to log.error anymore.

Will update.

[Dirk Pranke]

Created attachment 81588 [details]
Update w/ review feedback from mihaip

[Dirk Pranke]

Committed r77994: <http://trac.webkit.org/changeset/77994>

[WebKit Review Bot]

http://trac.webkit.org/changeset/77994 might have broken Qt Linux Release