142189 2015-03-02 14:54:57 -0800 The InspectorTimelineAgent should gracefully handle attempts to start more than once. 2015-03-02 15:42:04 -0800 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 142191 1 mark.lam mark.lam burg graouts joepeck jonowells mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1073451 0 mark.lam 2015-03-02 14:54:57 -0800 InspectorTimelineAgent::internalStop() already checks for redundant calls to it in case the InspectorTimelineAgent is already disabled. Similarly, InspectorTimelineAgent::internalStart() should check if the InspectorTimelineAgent is already enabled before proceeding to do work to enable it. Though wasteful, it is legal for clients of the InspectorTimelineAgent to invoke start on it more than once. Hence, this check is needed. This check fixes the debug assertion failure when running the inspector/timeline/debugger-paused-while-recording.html test. The test can now be unskipped. 1073452 1 webkit-bug-importer 2015-03-02 14:55:15 -0800 <rdar://problem/20012601> 1073470 2 joepeck 2015-03-02 15:26:26 -0800 "should not be allowed to start twice" => "should gracefully handling attempting to start twice". 1073474 3 247705 mark.lam 2015-03-02 15:31:26 -0800 Created attachment 247705 the patch. 1073475 4 247705 joepeck 2015-03-02 15:33:57 -0800 Comment on attachment 247705 the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=247705&action=review r=me > Source/WebCore/ChangeLog:13 > + is already enabled before proceeding to do work to enable it. Though wasteful, Nit: No need for more than one space after ".". > LayoutTests/TestExpectations:-100 > -webkit.org/b/137131 inspector/timeline [ Skip ] I hope this wasn't flakey for other reasons. 1073476 5 mark.lam 2015-03-02 15:35:31 -0800 (In reply to comment #4) > Comment on attachment 247705 [details] > the patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247705&action=review > > r=me > > > Source/WebCore/ChangeLog:13 > > + is already enabled before proceeding to do work to enable it. Though wasteful, > > Nit: No need for more than one space after ".". =) I always preferred 2 spaces for readability, but I can remove it. > > LayoutTests/TestExpectations:-100 > > -webkit.org/b/137131 inspector/timeline [ Skip ] > > I hope this wasn't flakey for other reasons. I didn't see any, but if the bots complain, we can skip this again. 1073479 6 247705 burg 2015-03-02 15:40:12 -0800 Comment on attachment 247705 the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=247705&action=review I wonder why some clients call start and stop multiple times. Is it because they assume some initial on/off state? > Source/WebCore/ChangeLog:8 > + No new tests. Unskipped an existing test that already asserts this. Test line should be below main description. > Source/WebCore/ChangeLog:10 > + InspectorTimelineAgent::internalStop() already checks for redundant calls to it in This paragraph is longer than necessary. You can simply state that you added a missing guard that prevents the timeline agent from starting if it has already started, matching the logic that guards against duplicated calls to internalStop(). 1073480 7 247705 burg 2015-03-02 15:40:42 -0800 Comment on attachment 247705 the patch. Accidentally cleared Joe's r+ 1073481 8 mark.lam 2015-03-02 15:42:04 -0800 Thanks for the review. Landed in r180901: <http://trac.webkit.org/r180901>. 247705 2015-03-02 15:31:26 -0800 2015-03-02 15:40:42 -0800 the patch. bug-142189.patch text/plain 3210 mark.lam SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4MDg5OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI2IEBACisyMDE1LTAzLTAyICBNYXJrIExh bSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBUaGUgSW5zcGVjdG9yVGltZWxpbmVB Z2VudCBzaG91bGQgZ3JhY2VmdWxseSBoYW5kbGUgYXR0ZW1wdHMgdG8gc3RhcnQgbW9yZSB0aGFu IG9uY2UuCisgICAgICAgIDxodHRwczovL3dlYmtpdC5vcmcvYi8xNDIxODk+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiAgVW5z a2lwcGVkIGFuIGV4aXN0aW5nIHRlc3QgdGhhdCBhbHJlYWR5IGFzc2VydHMgdGhpcy4KKworICAg ICAgICBJbnNwZWN0b3JUaW1lbGluZUFnZW50OjppbnRlcm5hbFN0b3AoKSBhbHJlYWR5IGNoZWNr cyBmb3IgcmVkdW5kYW50IGNhbGxzIHRvIGl0IGluCisgICAgICAgIGNhc2UgdGhlIEluc3BlY3Rv clRpbWVsaW5lQWdlbnQgaXMgYWxyZWFkeSBkaXNhYmxlZC4gIFNpbWlsYXJseSwKKyAgICAgICAg SW5zcGVjdG9yVGltZWxpbmVBZ2VudDo6aW50ZXJuYWxTdGFydCgpIHNob3VsZCBjaGVjayBpZiB0 aGUgSW5zcGVjdG9yVGltZWxpbmVBZ2VudAorICAgICAgICBpcyBhbHJlYWR5IGVuYWJsZWQgYmVm b3JlIHByb2NlZWRpbmcgdG8gZG8gd29yayB0byBlbmFibGUgaXQuICAgVGhvdWdoIHdhc3RlZnVs LAorICAgICAgICBpdCBpcyBsZWdhbCBmb3IgY2xpZW50cyBvZiB0aGUgSW5zcGVjdG9yVGltZWxp bmVBZ2VudCB0byBpbnZva2Ugc3RhcnQgb24gaXQgbW9yZQorICAgICAgICB0aGFuIG9uY2UuICBI ZW5jZSwgdGhpcyBjaGVjayBpcyBuZWVkZWQuCisKKyAgICAgICAgVGhpcyBjaGVjayBmaXhlcyB0 aGUgZGVidWcgYXNzZXJ0aW9uIGZhaWx1cmUgd2hlbiBydW5uaW5nIHRoZQorICAgICAgICBpbnNw ZWN0b3IvdGltZWxpbmUvZGVidWdnZXItcGF1c2VkLXdoaWxlLXJlY29yZGluZy5odG1sIHRlc3Qu ICBUaGUgdGVzdCBjYW4gbm93CisgICAgICAgIGJlIHVuc2tpcHBlZC4KKworICAgICAgICAqIGlu c3BlY3Rvci9JbnNwZWN0b3JUaW1lbGluZUFnZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Oklu c3BlY3RvclRpbWVsaW5lQWdlbnQ6OmludGVybmFsU3RhcnQpOgorCiAyMDE1LTAzLTAyICBSb2dl ciBGb25nICA8cm9nZXJfZm9uZ0BhcHBsZS5jb20+CiAKICAgICAgICAgVXBkYXRlIGJhY2tncm91 bmRzIG9mIHNsaWRlcnMgZm9yIGlubGluZSBtZWRpYSBjb250cm9scyBvbiBPUyBYLgpJbmRleDog U291cmNlL1dlYkNvcmUvaW5zcGVjdG9yL0luc3BlY3RvclRpbWVsaW5lQWdlbnQuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2luc3BlY3Rvci9JbnNwZWN0b3JUaW1lbGluZUFnZW50 LmNwcAkocmV2aXNpb24gMTgwODk2KQorKysgU291cmNlL1dlYkNvcmUvaW5zcGVjdG9yL0luc3Bl Y3RvclRpbWVsaW5lQWdlbnQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMDUsNiArMTA1LDkgQEAg dm9pZCBJbnNwZWN0b3JUaW1lbGluZUFnZW50OjpzdG9wKEVycm9yUwogCiB2b2lkIEluc3BlY3Rv clRpbWVsaW5lQWdlbnQ6OmludGVybmFsU3RhcnQoY29uc3QgaW50KiBtYXhDYWxsU3RhY2tEZXB0 aCkKIHsKKyAgICBpZiAobV9lbmFibGVkKQorICAgICAgICByZXR1cm47CisKICAgICBpZiAobWF4 Q2FsbFN0YWNrRGVwdGggJiYgKm1heENhbGxTdGFja0RlcHRoID4gMCkKICAgICAgICAgbV9tYXhD YWxsU3RhY2tEZXB0aCA9ICptYXhDYWxsU3RhY2tEZXB0aDsKICAgICBlbHNlCkluZGV4OiBMYXlv dXRUZXN0cy9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCShy ZXZpc2lvbiAxODA4OTgpCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkK QEAgLTEsMyArMSwxMyBAQAorMjAxNS0wMy0wMiAgTWFyayBMYW0gIDxtYXJrLmxhbUBhcHBsZS5j b20+CisKKyAgICAgICAgVGhlIEluc3BlY3RvclRpbWVsaW5lQWdlbnQgc2hvdWxkIGdyYWNlZnVs bHkgaGFuZGxlIGF0dGVtcHRzIHRvIHN0YXJ0IG1vcmUgdGhhbiBvbmNlLgorICAgICAgICA8aHR0 cHM6Ly93ZWJraXQub3JnL2IvMTQyMTg5PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgICogVGVzdEV4cGVjdGF0aW9uczoKKyAgICAgICAgLSBVbnNraXBw ZWQgaW5zcGVjdG9yL3RpbWVsaW5lIHRlc3RzLgorCiAyMDE1LTAzLTAyICBCcmVudCBGdWxnaGFt ICA8YmZ1bGdoYW1AYXBwbGUuY29tPgogCiAgICAgICAgIFtXaW5dIERvY3VtZW50IG1vcmUgZGVi dWcgYXNzZXJ0aW9ucy4KSW5kZXg6IExheW91dFRlc3RzL1Rlc3RFeHBlY3RhdGlvbnMKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gTGF5b3V0VGVzdHMvVGVzdEV4cGVjdGF0aW9ucwkocmV2aXNpb24gMTgwODk2KQor KysgTGF5b3V0VGVzdHMvVGVzdEV4cGVjdGF0aW9ucwkod29ya2luZyBjb3B5KQpAQCAtOTcsNyAr OTcsNiBAQCB3ZWJraXQub3JnL2IvMTMxOTE5IGluc3BlY3Rvci9kb20gWyBTa2lwCiAjIFRoZXNl IHRlc3RzIHN0aWxsIG15c3RlcmlvdXNseSBmbGFrZSBhbmQvb3IgdGltZSBvdXQuCiB3ZWJraXQu b3JnL2IvMTM3MTMxIGluc3BlY3Rvci9kZWJ1Z2dlciBbIFNraXAgXQogd2Via2l0Lm9yZy9iLzEz NzEzMCBpbnNwZWN0b3IvcmVwbGF5IFsgU2tpcCBdCi13ZWJraXQub3JnL2IvMTM3MTMxIGluc3Bl Y3Rvci90aW1lbGluZSBbIFNraXAgXQogaW5zcGVjdG9yL21vZGVsL3JlbW90ZS1vYmplY3QtZ2V0 LXByb3BlcnRpZXMuaHRtbCBbIFNraXAgXQogaW5zcGVjdG9yL21vZGVsL3JlbW90ZS1vYmplY3Qt d2Vhay1jb2xsZWN0aW9uLmh0bWwgWyBTa2lwIF0KIGluc3BlY3Rvci9tb2RlbC9yZW1vdGUtb2Jq ZWN0Lmh0bWwgWyBTa2lwIF0K