|Summary:||Web Inspector: Multiple calls to SetFrontendProxyObject can leave an InspectorTimelineAgent with a bad InspectorFrontend|
|Product:||WebKit||Reporter:||Kelly Norton <knorton>|
|Component:||Web Inspector (Deprecated)||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||commit-queue, darin, eric, pfeldman, timothy|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Kelly Norton 2009-10-02 15:04:37 PDT
Just a minor fix to the InspectorTimeline stuff that I added a few weeks back.
Comment 1 Kelly Norton 2009-10-02 15:11:01 PDT
Created attachment 40549 [details] Path to fix this issue.
Comment 2 Pavel Feldman 2009-10-03 10:28:41 PDT
Comment on attachment 40549 [details] 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.
Comment 3 Kelly Norton 2009-10-05 11:03:29 PDT
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?
Comment 4 Pavel Feldman 2009-10-05 11:53:30 PDT
(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!
Comment 5 Kelly Norton 2009-10-05 12:01:38 PDT
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.
Comment 6 WebKit Commit Bot 2009-10-06 12:57:09 PDT
Comment on attachment 40549 [details] Path to fix this issue. Clearing flags on attachment: 40549 Committed r49204: <http://trac.webkit.org/changeset/49204>
Comment 7 WebKit Commit Bot 2009-10-06 12:57:13 PDT
All reviewed patches have been landed. Closing bug.
Comment 8 Darin Adler 2009-10-06 13:01:58 PDT
You should not have needed a get() in this patch.
Comment 9 Kelly Norton 2009-10-06 13:22:44 PDT
(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.