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+

Brian Burg
Reported 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.
Attachments
Patch (15.18 KB, patch)
2014-11-21 16:52 PST, Brian Burg 		no flags
DetailsFormatted DiffDiff
Proposed Patch (41.84 KB, patch)
2014-12-02 10:49 PST, Katie Madonna 		no flags
DetailsFormatted DiffDiff
Proposed Fix (28.60 KB, patch)
2014-12-02 12:02 PST, Brian Burg 		no flags
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2014-11-21 12:12:49 PST
<rdar://problem/19061273>
Brian Burg
Comment 2 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.
Brian Burg
Comment 3 2014-11-21 16:52:34 PST
Created attachment 242094 [details] Patch
Katie Madonna
Comment 4 2014-12-02 10:49:06 PST
Created attachment 242428 [details] Proposed Patch
Brian Burg
Comment 5 2014-12-02 12:02:38 PST
Created attachment 242436 [details] Proposed Fix
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Brian Burg
Comment 8 2014-12-03 10:13:24 PST
Created attachment 242498 [details] Proposed Fix (fixed ChangeLog)
Joseph Pecoraro
Comment 9 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!
Brian Burg
Comment 10 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.
Brian Burg
Comment 11 2014-12-04 14:19:38 PST
Committed r176817: <http://trac.webkit.org/changeset/176817>
Joseph Pecoraro
Comment 12 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"
Note You need to log in before you can comment on or make changes to this bug.