Bug 161218 - The same crash shows up in both "Tests that crashed" and "Other Crashes"
Summary: The same crash shows up in both "Tests that crashed" and "Other Crashes"
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Minor
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-25 15:45 PDT by Jonathan Bedard
Modified: 2016-08-29 10:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.09 KB, patch)
2016-08-25 15:58 PDT, Jonathan Bedard
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Jonathan Bedard 2016-08-25 15:58:07 PDT
Created attachment 287038 [details]
Patch
Comment 2 Alexey Proskuryakov 2016-08-29 09:34:46 PDT
Comment on attachment 287038 [details]
Patch

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

> Tools/ChangeLog:12
> +        (Manager._look_for_new_crash_logs): Tied search to PID and test name, instead of just test name.

Will there be confusion with regards to UI process vs. web process PID?

My suspicion was that this was a logic error related to tests that are expected crashes per TestExpectations.
Comment 3 Jonathan Bedard 2016-08-29 09:53:58 PDT
Comment on attachment 287038 [details]
Patch

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

>> Tools/ChangeLog:12
>> +        (Manager._look_for_new_crash_logs): Tied search to PID and test name, instead of just test name.
> 
> Will there be confusion with regards to UI process vs. web process PID?
> 
> My suspicion was that this was a logic error related to tests that are expected crashes per TestExpectations.

Well, in the specific crash that prompted this change, the crash-log was explicitly added twice (i.e., two "Adding results for other crash: ..." messages were in standard out). Previously, we were comparing the entire process name to all previously recognized crashed processes, assuming that our list of crashlogs had no doubles in it.  First, this fix eliminates a problem when a crashlog is somehow included twice in the list of crash logs, since that's the only way to get two "Adding results for other crash: ..." Messages.

Second, it bases comparison off of process names and ID instead of assuming that the test name will be the process name.  I don't think that there should be any issues confusing the UI process and web process, since we are still tracking the process name (that's process[1]) .
Comment 4 Jonathan Bedard 2016-08-29 10:41:43 PDT
This is a symptom of a larger issue with Webkitpy confusing process names and test names.  Marking resolved, will revisit in a later over-haul of crash handling in Webkitpy.