30028 2009-10-02 15:04:37 -0700 Web Inspector: Multiple calls to SetFrontendProxyObject can leave an InspectorTimelineAgent with a bad InspectorFrontend 2009-10-06 13:23:41 -0700 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 0 knorton webkit-unassigned commit-queue darin eric pfeldman timothy oldest_to_newest 151979 0 knorton 2009-10-02 15:04:37 -0700 Just a minor fix to the InspectorTimeline stuff that I added a few weeks back. 151984 1 40549 knorton 2009-10-02 15:11:01 -0700 Created attachment 40549 Path to fix this issue. 152083 2 40549 pfeldman 2009-10-03 10:28:41 -0700 Comment on attachment 40549 Path to fix this issue. > - if (timelineEnabled.type() == Setting::BooleanType && timelineEnabled.booleanValue()) > + if (m_timelineAgent.get() || (timelineEnabled.type() == Setting::BooleanType && timelineEnabled.booleanValue())) Given that you initialize m_timelineAgent in the setFrontendProxyObject, it will only collect the data while inspector is opened (has been opened for the page at least once in case of WebKit). Is it so by design? I am asking because Inspector uses a bit different models for existing types of inspected objects: 1) It tracks resources iff tracking is enabled, regardless of the frontend state. It then dumps all the data collected into the frontend upon its availability. When user enables this tracking for the first time, page reload takes place in order to gather the data. 2) There are explicit "start/stop profiling" buttons that start/stop collecting data and dump results into the profiler page. We are currently extending profiling tab so that it could gather different types of profiles. I think you should choose one of the models above to look consistent within WebKit. If you think some users can have timeline agent turned on constantly (it produces minimal overhead, there are times when user would like to see what has been happening without reloading the page), you should go (1). Otherwise, if you consider your agent to be of a profiling nature, you should choose (2). (2) implies that timeline agent results are presented as a profile result on the profiles page. We can chat about it and discuss the ways you will push the gathered information further into your framework. I'll be happy to help you out with the WebKit frontend piece as well. 152368 3 knorton 2009-10-05 11:03:29 -0700 Thanks Pavel, Upon re-reading this, I think maybe we should just talk and figure this out. I do think the TimelineAgent is of a profiling nature in that it should not be enabled all the time. However, I'd like to find some way to keep it turned on across refreshes. In fact, I've been trying to find a way inside of chromium to keep it turned on across page transitions. We're trying to use this thing in Chromium to keep up the illusion of "profiling a tab" not just a page. So I think the best thing would be for us to chat about the best way to proceed. Also, this fix is actually a crasher if you turn on the TimelineAgent in Chrome. Is there any interest in landing this and having me start a new bug to better organize InspectorTimelineAgent? 152402 4 pfeldman 2009-10-05 11:53:30 -0700 (In reply to comment #3) I have no problems submitting it given that it fixes a crash. However, please note that WebKit bugs should be submitted in WebKit terms and scenarios unless they are about bindings / engine specifics. So please avoid using the Chromium-based scenarios in your bug reports / patches! 152404 5 knorton 2009-10-05 12:01:38 -0700 Sounds good. And you're right the crashing scenario is actually any case where setFrontendProxyObject is called twice, so this is a general WebKit issue. I'll file another bug for re-organizing this TimelineAgent. Thanks. 152827 6 40549 commit-queue 2009-10-06 12:57:09 -0700 Comment on attachment 40549 Path to fix this issue. Clearing flags on attachment: 40549 Committed r49204: <http://trac.webkit.org/changeset/49204> 152828 7 commit-queue 2009-10-06 12:57:13 -0700 All reviewed patches have been landed. Closing bug. 152830 8 darin 2009-10-06 13:01:58 -0700 You should not have needed a get() in this patch. 152841 9 knorton 2009-10-06 13:22:44 -0700 (In reply to comment #8) > You should not have needed a get() in this patch. I'll remove that in a subsequent patch unless you would prefer that I go ahead and fix it now. 152843 10 darin 2009-10-06 13:23:41 -0700 (In reply to comment #9) > I'll remove that in a subsequent patch unless you would prefer that I go ahead > and fix it now. OK, sounds fine. 40549 2009-10-02 15:11:01 -0700 2009-10-06 12:57:09 -0700 Path to fix this issue. 30028.diff text/plain 1421 knorton ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MGFlODgyMy4uNWRhZTRkOSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAwOS0xMC0wMiAgS2VsbHkgTm9ydG9u ICA8a25vcnRvbkBnb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEZpeGVzIDxodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MzAwMjg+CisgICAgICAgIE11bHRpcGxlIGNhbGxzIHRvIFNldEZyb250ZW5kUHJveHlP YmplY3QgY2FuIGxlYXZlIGFuIEluc3BlY3RvclRpbWVsaW5lQWdlbnQgd2l0aCBhbiBpbnZhbGlk CisgICAgICAgIEluc3BlY3RvckZyb250ZW5kLgorCisgICAgICAgICogaW5zcGVjdG9yL0luc3Bl Y3RvckNvbnRyb2xsZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SW5zcGVjdG9yQ29udHJvbGxl cjo6c2V0RnJvbnRlbmRQcm94eU9iamVjdCk6IEFkZHMgYSBjaGVjayBmb3IgYW4gZXhpc3Rpbmcg dGltZWxpbmUgYWdlbnQuCisKIDIwMDktMTAtMDIgIEpvc2VwaCBQZWNvcmFybyAgPGpvZXBlY2tA d2Via2l0Lm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBUaW1vdGh5IEhhdGNoZXIuCmRpZmYg LS1naXQgYS9XZWJDb3JlL2luc3BlY3Rvci9JbnNwZWN0b3JDb250cm9sbGVyLmNwcCBiL1dlYkNv cmUvaW5zcGVjdG9yL0luc3BlY3RvckNvbnRyb2xsZXIuY3BwCmluZGV4IDZjMjIyOTUuLjNjNDhh YWUgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvaW5zcGVjdG9yL0luc3BlY3RvckNvbnRyb2xsZXIuY3Bw CisrKyBiL1dlYkNvcmUvaW5zcGVjdG9yL0luc3BlY3RvckNvbnRyb2xsZXIuY3BwCkBAIC01NTgs NyArNTU4LDcgQEAgdm9pZCBJbnNwZWN0b3JDb250cm9sbGVyOjpzZXRGcm9udGVuZFByb3h5T2Jq ZWN0KFNjcmlwdFN0YXRlKiBzY3JpcHRTdGF0ZSwgU2NyaXAKICAgICBtX2RvbUFnZW50ID0gbmV3 IEluc3BlY3RvckRPTUFnZW50KG1fZnJvbnRlbmQuZ2V0KCkpOwogCiAgICAgU2V0dGluZyB0aW1l bGluZUVuYWJsZWQgPSBzZXR0aW5nKHRpbWVsaW5lRW5hYmxlZFNldHRpbmdOYW1lKTsKLSAgICBp ZiAodGltZWxpbmVFbmFibGVkLnR5cGUoKSA9PSBTZXR0aW5nOjpCb29sZWFuVHlwZSAmJiB0aW1l bGluZUVuYWJsZWQuYm9vbGVhblZhbHVlKCkpCisgICAgaWYgKG1fdGltZWxpbmVBZ2VudC5nZXQo KSB8fCAodGltZWxpbmVFbmFibGVkLnR5cGUoKSA9PSBTZXR0aW5nOjpCb29sZWFuVHlwZSAmJiB0 aW1lbGluZUVuYWJsZWQuYm9vbGVhblZhbHVlKCkpKQogICAgICAgICBtX3RpbWVsaW5lQWdlbnQg PSBuZXcgSW5zcGVjdG9yVGltZWxpbmVBZ2VudChtX2Zyb250ZW5kLmdldCgpKTsKIH0KIAo=