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
Created attachment 42408 [details] WebInspector: Handle turning timeline profiling on/off in the middle of event dispatch
Created attachment 42413 [details] WebInspector: Adds test to bug 31080, Handle turning profiling on/off in the middle of event dispatch
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.
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.
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) …
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.
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.
Created attachment 42428 [details] WebInspector: bug 31080, Handle turning profiling on/off in the middle of event dispatch Corrects formatting glitch in DOMTimer.cpp
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
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
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>
All reviewed patches have been landed. Closing bug.