Tidy up ConsoleMessage callstack inclusion, fix Worker console crashes

Tidy up ConsoleMessage callstack inclusion, fix Worker console crashes

Follow up r181319 (+ r180544) and annotate ConsoleMessages from
console.count() and console.timeEnd() with a call stack.

Not doing so broke the WorkerConsole::reportMessageToConsole()
assumption that a ConsoleMessage has a call stack. An unnecessary
assumption to make, so instead have it mirror how "non-Worker" console
messages are handled when reported, and allow "stackless" messages.

R=kozyatinskiy,aandrey,jochen
BUG=413223
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182056

[sof, 2014-09-14 22:08:28]

sigbjornf@opera.com changed reviewers:
+ horo@chromium.org, vsevik@chromium.org, yurys@chromium.org

[sof, 2014-09-14 22:08:29]

Please take a look.

[aandrey, 2014-09-15 04:12:20]

aandrey@chromium.org changed reviewers:
+ aandrey@chromium.org, kozyatinskiy@google.com

[sof, 2014-09-15 08:21:35]

sigbjornf@opera.com changed reviewers:
+ jochen@chromium.org

[sof, 2014-09-15 08:21:35]

+jochen - seems like other reviewers are enjoying some time off today :-)

[kozyatinskiy1, 2014-09-15 12:57:26]

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
File Source/core/frame/ConsoleBase.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
Source/core/frame/ConsoleBase.cpp:106: RefPtrWillBeRawPtr<ScriptCallStack>
callStack(createScriptCallStackForConsole(1));
createScriptCallStack(1) is equivalent createScriptCallStackForConsole(1). In
createScriptCallStackForConsole if console agent is enabled we will collect
maxStackSize frames otherwise only one frame will be collected but if we pass
one to createScriptCallStack then nothing will change in both cases - we always
collect one frame.
Why do we need this change?

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
Source/core/frame/ConsoleBase.cpp:174:
consoleMessage->setCallStack(createScriptCallStackForConsole(1));
I suppose we can replace createScriptCallStackForConsole(1) to
createScriptCallStack(1)

https://codereview.chromium.org/562683003/diff/1/Source/core/workers/WorkerCo...
File Source/core/workers/WorkerConsole.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/workers/WorkerCo...
Source/core/workers/WorkerConsole.cpp:58: }
We have two places where we call this function: ConsoleBase::time/timeEnd and
internalAddMessage. If you add the collecting stack to time/timeEnd that I
missed then we do not need this check here. Because by default
createScriptCallStackForConsole and createScriptCallStackForConsole always
return at least one frame.

[sof, 2014-09-15 13:09:33]

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
File Source/core/frame/ConsoleBase.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
Source/core/frame/ConsoleBase.cpp:106: RefPtrWillBeRawPtr<ScriptCallStack>
callStack(createScriptCallStackForConsole(1));
On 2014/09/15 12:57:25, kozyatinskiy wrote:
> createScriptCallStack(1) is equivalent createScriptCallStackForConsole(1). In
> createScriptCallStackForConsole if console agent is enabled we will collect
> maxStackSize frames otherwise only one frame will be collected but if we pass
> one to createScriptCallStack then nothing will change in both cases - we
always
> collect one frame.
> Why do we need this change?

Let me answer that with a question :) Why do you not want count() to use the
*Console() version that internalAddMessage() uses? Simple and consistent, and
easier to live with should the *Console() behavior evolve, for instance.

https://codereview.chromium.org/562683003/diff/1/Source/core/workers/WorkerCo...
File Source/core/workers/WorkerConsole.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/workers/WorkerCo...
Source/core/workers/WorkerConsole.cpp:58: }
On 2014/09/15 12:57:25, kozyatinskiy wrote:
> We have two places where we call this function: ConsoleBase::time/timeEnd and
> internalAddMessage. If you add the collecting stack to time/timeEnd that I
> missed then we do not need this check here. Because by default
> createScriptCallStackForConsole and createScriptCallStackForConsole always
> return at least one frame.

That seems quite brittle - by inference, we should also remove it for the other
implementations of reportMessageToConsole() also? Which do not assume that a
callstack is present -- and should not afaict, as
createScriptCallStackForConsole() might return 0.

[kozyatinskiy1, 2014-09-15 13:31:29]

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
File Source/core/frame/ConsoleBase.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
Source/core/frame/ConsoleBase.cpp:106: RefPtrWillBeRawPtr<ScriptCallStack>
callStack(createScriptCallStackForConsole(1));
On 2014/09/15 13:09:32, sof wrote:
> On 2014/09/15 12:57:25, kozyatinskiy wrote:
> > createScriptCallStack(1) is equivalent createScriptCallStackForConsole(1).
In
> > createScriptCallStackForConsole if console agent is enabled we will collect
> > maxStackSize frames otherwise only one frame will be collected but if we
pass
> > one to createScriptCallStack then nothing will change in both cases - we
> always
> > collect one frame.
> > Why do we need this change?
> 
> Let me answer that with a question :) Why do you not want count() to use the
> *Console() version that internalAddMessage() uses? Simple and consistent, and
> easier to live with should the *Console() behavior evolve, for instance.

Because in internalAddMessage in case of tracing message we want collect more
than one frame - 
ScriptCallStack::maxCallStackSizeToCapture. In this call we always collect one
frame and I think that we can not do check is console agent enabled or disabled.
Continue with question :)
Why do extra checking if it can be skipped?

https://codereview.chromium.org/562683003/diff/1/Source/core/workers/WorkerCo...
File Source/core/workers/WorkerConsole.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/workers/WorkerCo...
Source/core/workers/WorkerConsole.cpp:58: }
On 2014/09/15 13:09:33, sof wrote:
> On 2014/09/15 12:57:25, kozyatinskiy wrote:
> > We have two places where we call this function: ConsoleBase::time/timeEnd
and
> > internalAddMessage. If you add the collecting stack to time/timeEnd that I
> > missed then we do not need this check here. Because by default
> > createScriptCallStackForConsole and createScriptCallStackForConsole always
> > return at least one frame.
> 
> That seems quite brittle - by inference, we should also remove it for the
other
> implementations of reportMessageToConsole() also? Which do not assume that a
> callstack is present -- and should not afaict, as
> createScriptCallStackForConsole() might return 0.

Ok, I agree with you but in this case, it is difficult to imagine a scenario in
which v8::Isolate would lose its context immediately after the console call and
createScriptCallStack* returns null.

[sof, 2014-09-15 13:44:18]

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
File Source/core/frame/ConsoleBase.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
Source/core/frame/ConsoleBase.cpp:106: RefPtrWillBeRawPtr<ScriptCallStack>
callStack(createScriptCallStackForConsole(1));
On 2014/09/15 13:31:29, kozyatinskiy wrote:
> On 2014/09/15 13:09:32, sof wrote:
> > On 2014/09/15 12:57:25, kozyatinskiy wrote:
> > > createScriptCallStack(1) is equivalent createScriptCallStackForConsole(1).
> In
> > > createScriptCallStackForConsole if console agent is enabled we will
collect
> > > maxStackSize frames otherwise only one frame will be collected but if we
> pass
> > > one to createScriptCallStack then nothing will change in both cases - we
> > always
> > > collect one frame.
> > > Why do we need this change?
> > 
> > Let me answer that with a question :) Why do you not want count() to use the
> > *Console() version that internalAddMessage() uses? Simple and consistent,
and
> > easier to live with should the *Console() behavior evolve, for instance.
> 
> Because in internalAddMessage in case of tracing message we want collect more
> than one frame - 
> ScriptCallStack::maxCallStackSizeToCapture. In this call we always collect one
> frame and I think that we can not do check is console agent enabled or
disabled.
> Continue with question :)
> Why do extra checking if it can be skipped?

I don't understand why you want to optimize this.

[kozyatinskiy1, 2014-09-15 14:18:21]

lgtm, but please wait for aandrey or vsevik to review.

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
File Source/core/frame/ConsoleBase.cpp (right):

https://codereview.chromium.org/562683003/diff/1/Source/core/frame/ConsoleBas...
Source/core/frame/ConsoleBase.cpp:106: RefPtrWillBeRawPtr<ScriptCallStack>
callStack(createScriptCallStackForConsole(1));
On 2014/09/15 13:44:18, sof wrote:
> On 2014/09/15 13:31:29, kozyatinskiy wrote:
> > On 2014/09/15 13:09:32, sof wrote:
> > > On 2014/09/15 12:57:25, kozyatinskiy wrote:
> > > > createScriptCallStack(1) is equivalent
createScriptCallStackForConsole(1).
> > In
> > > > createScriptCallStackForConsole if console agent is enabled we will
> collect
> > > > maxStackSize frames otherwise only one frame will be collected but if we
> > pass
> > > > one to createScriptCallStack then nothing will change in both cases - we
> > > always
> > > > collect one frame.
> > > > Why do we need this change?
> > > 
> > > Let me answer that with a question :) Why do you not want count() to use
the
> > > *Console() version that internalAddMessage() uses? Simple and consistent,
> and
> > > easier to live with should the *Console() behavior evolve, for instance.
> > 
> > Because in internalAddMessage in case of tracing message we want collect
more
> > than one frame - 
> > ScriptCallStack::maxCallStackSizeToCapture. In this call we always collect
one
> > frame and I think that we can not do check is console agent enabled or
> disabled.
> > Continue with question :)
> > Why do extra checking if it can be skipped?
> 
> I don't understand why you want to optimize this.

We discussed this with @vsevik in patch:
https://chromiumcodereview.appspot.com/458253002/diff/1/Source/core/inspector...

I have only one consideration - getting a little faster and easier code.

[sof, 2014-09-16 12:24:46]

On 2014/09/15 14:18:21, kozyatinskiy wrote:
> lgtm, but please wait for aandrey or vsevik to review.
> 

thanks, would be good to get this one ready sooner :)

[aandrey, 2014-09-16 12:38:57]

aandrey@chromium.org changed reviewers:
+ pfeldman@chromium.org

[aandrey, 2014-09-16 12:38:57]

lgtm (not owner)

[jochen (gone - plz use gerrit), 2014-09-16 14:26:27]

lgtm

[sof, 2014-09-16 14:51:54]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-16 14:52:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562683003/1

[commit-bot: I haz the power, 2014-09-16 15:26:45]

Committed patchset #1 (id:1) as 182056