Bug 138976

Summary: Web Inspector: timeline probe records have inaccurate per-probe hit counts
Product: WebKit Reporter: Brian Burg <burg>
Component: Web InspectorAssignee: Brian Burg <burg>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, graouts, joepeck, madonnk, rniwa, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 132073    
Attachments:
Description Flags
Patch
none
Proposed Patch
none
Proposed Fix
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Proposed Fix (fixed ChangeLog) joepeck: review+

Description Brian Burg 2014-11-21 12:12:37 PST
Given a timeline record for breakpointActionProbe, we need to be able to find the relevant probe sample so that the user can click on a timeline row and jump to the specific probe sample associated with that timeline row.

The current implementation uses a per-JSContext m_hitCount variable, which is similar to the global sampleId but not guaranteed to be the same. These should be combined into the same thing and renamed in the protocol.

This design might be revisited once the probe UI becomes replay-aware, but it's fine for now.
Comment 1 Radar WebKit Bug Importer 2014-11-21 12:12:49 PST
<rdar://problem/19061273>
Comment 2 Brian Burg 2014-11-21 13:18:05 PST
    I have a patch forthcoming to fix the backend parts.

    madonk will write a test to be written to cover sampleId behavior. In particular, the samples obtained from the timeline record should match what is seen in the probe manager.
Comment 3 Brian Burg 2014-11-21 16:52:34 PST
Created attachment 242094 [details]
Patch
Comment 4 Katie Madonna 2014-12-02 10:49:06 PST
Created attachment 242428 [details]
Proposed Patch
Comment 5 Brian Burg 2014-12-02 12:02:38 PST
Created attachment 242436 [details]
Proposed Fix
Comment 6 Build Bot 2014-12-02 14:53:20 PST
Comment on attachment 242436 [details]
Proposed Fix

Attachment 242436 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5347395973939200

New failing tests:
transitions/default-timing-function.html
Comment 7 Build Bot 2014-12-02 14:53:24 PST
Created attachment 242452 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Brian Burg 2014-12-03 10:13:24 PST
Created attachment 242498 [details]
Proposed Fix (fixed ChangeLog)
Comment 9 Joseph Pecoraro 2014-12-03 17:19:15 PST
Comment on attachment 242498 [details]
Proposed Fix (fixed ChangeLog)

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

r=me

> LayoutTests/inspector-protocol/debugger/didSampleProbe-multiple-probes.html:42
> +                return samples.every(function(sample, idx) { return sample.sampleId = idx + 1; });

I think the every predicate callback should be:

    return sample.sampleId === idx + 1;

Otherwise it will always be returning true!
Comment 10 Brian Burg 2014-12-03 22:25:09 PST
(In reply to comment #9)
> Comment on attachment 242498 [details]
> Proposed Fix (fixed ChangeLog)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242498&action=review
> 
> r=me
> 
> > LayoutTests/inspector-protocol/debugger/didSampleProbe-multiple-probes.html:42
> > +                return samples.every(function(sample, idx) { return sample.sampleId = idx + 1; });
> 
> I think the every predicate callback should be:
> 
>     return sample.sampleId === idx + 1;
> 
> Otherwise it will always be returning true!

Doh, will fix before landing.
Comment 11 Brian Burg 2014-12-04 14:19:38 PST
Committed r176817: <http://trac.webkit.org/changeset/176817>
Comment 12 Joseph Pecoraro 2014-12-10 17:54:43 PST
This test fails when run multiple times, the sample ids seem to increase (5-8) instead of (1-4).

shell> run-webkit-tests inspector-protocol/debugger/didSampleProbe-multiple-probes.html --repeat-each 2 -v --no-retry -g --additional-env-var="JSC_slowPathAllocsBetweenGCs=1"