Bug 138976 - Web Inspector: timeline probe records have inaccurate per-probe hit counts
Summary: Web Inspector: timeline probe records have inaccurate per-probe hit counts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 132073
  Show dependency treegraph
 
Reported: 2014-11-21 12:12 PST by Brian Burg
Modified: 2014-12-10 17:54 PST (History)
7 users (show)

See Also:


Attachments
Patch (15.18 KB, patch)
2014-11-21 16:52 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Patch (41.84 KB, patch)
2014-12-02 10:49 PST, Katie Madonna
no flags Details | Formatted Diff | Diff
Proposed Fix (28.60 KB, patch)
2014-12-02 12:02 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (620.48 KB, application/zip)
2014-12-02 14:53 PST, Build Bot
no flags Details
Proposed Fix (fixed ChangeLog) (19.58 KB, patch)
2014-12-03 10:13 PST, Brian Burg
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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"