Bug 38531 – V8 ScriptCallStack does not build on Android due to missing feature guards

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

V8's ScriptCallStack.cpp does not compile on Android because ScriptDebugServer has not been declared.

Changes http://trac.webkit.org/changeset/57231 and http://trac.webkit.org/changeset/55277 modified ScriptCallStack::create() to require ScriptDebugServer::topStackFrame(). ScriptDebugServer.cpp is guarded with ENABLE(JAVASCRIPT_DEBUGGER), which is not defined on Android.

It looks like the correct fix is to guard all of ScriptCallStack.cpp with ENABLE(JAVASCRIPT_DEBUGGER).

[Steve Block]

Created attachment 55067 [details]
Patch

[Steve Block]

The patch I've uploaded guards all of ScriptCallStack with ENABLE(JAVASCRIPT_DEBUGGER) and adds similar guards to the Console methods that use the class.

This fixes the Android build but is probably overly drastic. However, I'm not familiar with the intended interaction between ENABLE(JAVASCRIPT_DEBUGGER), ENABLE(INSPECTOR) and Console. Pavel, do you know what the correct approach is, or know who might?

[Pavel Feldman]

Comment on attachment 55067 [details]
Patch

This is Ok short term so that you could have the build compiled. However, longer term, I think we should move topStackFrame to the ScriptCallStack so that you did not lose console API functions.

[Steve Block]

Created attachment 55099 [details]
Patch

[Steve Block]

> This is Ok short term so that you could have the build compiled. However,
> longer term, I think we should move topStackFrame to the ScriptCallStack so
> that you did not lose console API functions.
Thanks for the comments Pavel. I've updated my patch to reflect this. There's no huge rush to fix the Android build, as we don't sync too regularly.

[Pavel Feldman]

(In reply to comment #5)
> > This is Ok short term so that you could have the build compiled. However,
> > longer term, I think we should move topStackFrame to the ScriptCallStack so
> > that you did not lose console API functions.
> Thanks for the comments Pavel. I've updated my patch to reflect this. There's
> no huge rush to fix the Android build, as we don't sync too regularly.

One thing that concerns me

[Pavel Feldman]

(In reply to comment #6)
> (In reply to comment #5)
> > > This is Ok short term so that you could have the build compiled. However,
> > > longer term, I think we should move topStackFrame to the ScriptCallStack so
> > > that you did not lose console API functions.
> > Thanks for the comments Pavel. I've updated my patch to reflect this. There's
> > no huge rush to fix the Android build, as we don't sync too regularly.
> 
> One thing that concerns me ... 

oops - was adding Jaime to the CC list.

So the thing that concerns me now is that it pulled everything together with utility context into the stack class. Initially, utility context lived in the V8Proxy and was used for the debugging needs mostly. At that time I suggested Jaime moving it into the newly create ScriptDebugServer class, but I did not realize ports might want to use console traces and not have javascript debugging enabled back then. It looks like this is the case though, so we might need to move utility context back to the v8 proxy, call it debugger context and use from ScriptDebugServer and this call stack class. I realize that it is too much to ask from you, so I am happy to fix it myself!

[Pavel Feldman]

Actually, Jaime is almost done with his v8 refactoring that allows capturing javascript stack without the need in utility context. Why don't we land first version of your patch now and get rid of the context as a whole in few days (once new stack-related code lands into v8 and rolls into the build)?

Jaime, could you confirm that you are about to land and that the utility context will go away (at least from the ScriptCallStack)?

(In reply to comment #8)
> Actually, Jaime is almost done with his v8 refactoring that allows capturing
> javascript stack without the need in utility context. Why don't we land first
> version of your patch now and get rid of the context as a whole in few days
> (once new stack-related code lands into v8 and rolls into the build)?
> 
> Jaime, could you confirm that you are about to land and that the utility
> context will go away (at least from the ScriptCallStack)?

Yes. I am about to land a new StackTrace API to V8 bleeding edge. Should go in today or tomorrow depending on when I can get a committer.  See http://codereview.chromium.org/1694011/show

Yes, this means that I can remove the utility context entirely from ScriptDebugServer. I plan on updating https://bugs.webkit.org/show_bug.cgi?id=37502 to use that API and in the process resolve this bug.

I think we should wait until bleeding edge V8 gets merged to trunk and that gets deps rolled into Chromium before updating webkit. This should happen I think by Friday. But I will try and submit the updated WebKit patch for review sometime today.

(In reply to comment #9)
> (In reply to comment #8)
> > Actually, Jaime is almost done with his v8 refactoring that allows capturing
> > javascript stack without the need in utility context. Why don't we land first
> > version of your patch now and get rid of the context as a whole in few days
> > (once new stack-related code lands into v8 and rolls into the build)?
> > 
> > Jaime, could you confirm that you are about to land and that the utility
> > context will go away (at least from the ScriptCallStack)?
> 
> Yes. I am about to land a new StackTrace API to V8 bleeding edge. Should go in
> today or tomorrow depending on when I can get a committer.  See
> http://codereview.chromium.org/1694011/show
> 
> Yes, this means that I can remove the utility context entirely from
> ScriptDebugServer. I plan on updating
> https://bugs.webkit.org/show_bug.cgi?id=37502 to use that API and in the
> process resolve this bug.
> 
> I think we should wait until bleeding edge V8 gets merged to trunk and that
> gets deps rolled into Chromium before updating webkit. This should happen I
> think by Friday. But I will try and submit the updated WebKit patch for review
> sometime today.

To clarify, after catching up on this bug, I agree with Pavel. Land this now to remove the utility context from ScriptDebugServer.

I will follow up later to use the new stack trace API.

[WebKit Commit Bot]

Comment on attachment 55099 [details]
Patch

Clearing flags on attachment: 55099

Committed r58818: <http://trac.webkit.org/changeset/58818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.