Bug 30028 – Web Inspector: Multiple calls to SetFrontendProxyObject can leave an InspectorTimelineAgent with a bad InspectorFrontend

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Kelly Norton]

Just a minor fix to the InspectorTimeline stuff that I added a few weeks back.

[Kelly Norton]

Created attachment 40549 [details]
Path to fix this issue.

[Pavel Feldman]

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.

[Kelly Norton]

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?

[Pavel Feldman]

(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!

[Kelly Norton]

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.

[WebKit Commit Bot]

Comment on attachment 40549 [details]
Path to fix this issue.

Clearing flags on attachment: 40549

Committed r49204: <http://trac.webkit.org/changeset/49204>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

You should not have needed a get() in this patch.

[Kelly Norton]

(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.

[Darin Adler]

(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.