Bug 31080 – WebInspector: Handle turning TimelineAgent on/off in the middle of an event dispatch

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

[Eric Ayers]

Currently, If you enable or disable the timeline agent in the middle of a dispatch, you get undefined behavior when it hits the end of an instrumentation point.  The InspectorTimelineAgent* is being saved from the time the event started to the time it ended.  

To fix the problem when timeline profiling is turned off in the middle of processing an event, the pointer needs to be retrieved each time to make sure it is still valid.  For the case where timeline profiling is turned on in the middle of an event, the case where the stack of instrumented events is empty should be silently ignore, not asserted.

This patch doesn't fix each instrumentation point, but gives an example of what I propose.  I can add on to this patch or submit a subsequent patch.

I made an unrelated change to make the check-webkit-style script run cleanly on ScriptController.cpp

[Eric Ayers]

Created attachment 42408 [details]
WebInspector: Handle turning timeline profiling on/off in the middle of event dispatch

[Eric Ayers]

Created attachment 42413 [details]
WebInspector: Adds test to bug 31080, Handle turning profiling on/off in the middle of event dispatch

[Pavel Feldman]

Comment on attachment 42413 [details]
WebInspector: Adds test to bug 31080, Handle turning profiling on/off in the middle of event dispatch

Please refer to a bug from the ChangeLog (s)!


> -            type == eventNames().clickEvent || type == eventNames().mousedownEvent ||
> -            type == eventNames().mouseupEvent || type == eventNames().dblclickEvent ||
> +            type == eventNames().clickEvent || type == eventNames().mousedownEvent 
> +            || type == eventNames().mouseupEvent || type == eventNames().dblclickEvent 
>              // keyboard events
> -            type == eventNames().keydownEvent || type == eventNames().keypressEvent ||
> -            type == eventNames().keyupEvent ||
> +            || type == eventNames().keydownEvent || type == eventNames().keypressEvent
> +            || type == eventNames().keyupEvent
>              // other accepted events
> -            type == eventNames().selectEvent || type == eventNames().changeEvent ||
> -            type == eventNames().focusEvent || type == eventNames().blurEvent ||
> -            type == eventNames().submitEvent)
> +            || type == eventNames().selectEvent || type == eventNames().changeEvent
> +            || type == eventNames().focusEvent || type == eventNames().blurEvent
> +            || type == eventNames().submitEvent)
>              return true;

Nothing changed, right?

> +    // An empty stack could merely mean that the timeline agent was turned on in the middle of
> +    // an event.  Don't treat as an error.

We are now forgiving too much and it could be error-prone. I think it is Ok for now though.

> +    if (!m_recordStack.isEmpty()) {
> +        TimelineRecordEntry entry = m_recordStack.last();
> +        m_recordStack.removeLast();
> +        ASSERT(entry.type == type);

There is no guarantee that the top of the stack is the matching event, is there?
Counter and a map would save the world, but will make things more complex. Returning timelineagent "version" from will* methods and passing it back to did* method would be Ok.

[Timothy Hatcher]

Comment on attachment 42413 [details]
WebInspector: Adds test to bug 31080, Handle turning profiling on/off in the middle of event dispatch

> +    if (InspectorTimelineAgent* timelineAgent = m_frame->page() ? m_frame->page()->inspectorTimelineAgent() : 0)
> +            timelineAgent->didEvaluateScript();

Bad indentation.


> +  <div id="frontend-script" style="display:none">
> +function startTimeline() {
> +  /* This test seems silly, but originally it tickled bug 31080 */
> +  InspectorController.startTimelineProfiler();
> +  return true;
> +}
> +  </div>

Indent to match the </div>. Also use 4 space indents in the test.

[Timothy Hatcher]

You could also habd back a TimelineAgent** and have it nulled out when disabled.

We do this in JSC for the profiler.

        static Profiler** enabledProfilerReference()
        {
            return &s_sharedEnabledProfilerReference;
        }


    Profiler** profiler = Profiler::enabledProfilerReference();
    if (*profiler)
      …

[Eric Ayers]

Created attachment 42423 [details]
WebInspector: bug 31080, Handle turning profiling on/off in the middle of event dispatch

Addresses review feedback:

ScriptController.cpp: Change of || characters was just to satisfy lint, no real change)

- Fix formatting of test case

Discussed alternative schemes for tracking a change in the InspectorTimelineAgent but in the end decided to leave  that code alone.

[Timothy Hatcher]

Comment on attachment 42423 [details]
WebInspector: bug 31080, Handle turning profiling on/off in the middle of event dispatch


> -        if (timelineAgent)
> -            timelineAgent->didFireTimer();
> +    if (InspectorTimelineAgent* timelineAgent = InspectorTimelineAgent::retrieve(context))
> +        timelineAgent->didFireTimer();
>  #endif
>          return;

Bad indent.

[Eric Ayers]

Created attachment 42428 [details]
WebInspector: bug 31080, Handle turning profiling on/off in the middle of event dispatch

Corrects formatting glitch in DOMTimer.cpp

[WebKit Commit Bot]

Comment on attachment 42428 [details]
WebInspector: bug 31080, Handle turning profiling on/off in the middle of event dispatch

Rejecting patch 42428 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
orTimelineAgent.cpp
	M	WebCore/page/DOMTimer.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:

        trunk/LayoutTests/ChangeLog

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

[Eric Ayers]

Created attachment 42431 [details]
WebInspector: bug 31080, Handle turning profiling on/off in the middle of event dispatch

gets rid of Tabs in Layout/ChangeLog

[WebKit Commit Bot]

Comment on attachment 42431 [details]
WebInspector: bug 31080, Handle turning profiling on/off in the middle of event dispatch

Clearing flags on attachment: 42431

Committed r50489: <http://trac.webkit.org/changeset/50489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.