Bug 53656 - new-run-webkit-tests: split out thread stack logging code into a sharable module
Summary: new-run-webkit-tests: split out thread stack logging code into a sharable module
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 49566
  Show dependency treegraph
 
Reported: 2011-02-02 19:31 PST by Dirk Pranke
Modified: 2011-02-08 17:24 PST (History)
6 users (show)

See Also:


Attachments
Patch (10.99 KB, patch)
2011-02-02 19:33 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
add log_traceback() (10.13 KB, patch)
2011-02-04 19:05 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Update w/ review feedback from mihaip (10.56 KB, patch)
2011-02-07 22:43 PST, Dirk Pranke
mihaip: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-02 19:31:59 PST
new-run-webkit-tests: split out thread stack logging code into a sharable module
Comment 1 Dirk Pranke 2011-02-02 19:33:20 PST
Created attachment 81023 [details]
Patch
Comment 2 Dirk Pranke 2011-02-04 19:05:30 PST
Created attachment 81334 [details]
add log_traceback()
Comment 3 Eric Seidel (no email) 2011-02-05 01:01:19 PST
I'm entertained that this hack is now an official tested module. :)
Comment 4 Mihai Parparita 2011-02-07 15:18:42 PST
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.
Comment 5 Dirk Pranke 2011-02-07 22:32:12 PST
(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.
Comment 6 Dirk Pranke 2011-02-07 22:43:46 PST
Created attachment 81588 [details]
Update w/ review feedback from mihaip
Comment 7 Dirk Pranke 2011-02-08 16:48:49 PST
Committed r77994: <http://trac.webkit.org/changeset/77994>
Comment 8 WebKit Review Bot 2011-02-08 17:24:16 PST
http://trac.webkit.org/changeset/77994 might have broken Qt Linux Release