Bug 107674 – [V8] Reduce usage of deprecatedV8String() and deprecatedV8Integer()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

A bit more work is needed to kill them completely.

[Kentaro Hara]

Created attachment 184206 [details]
Patch

[WebKit Review Bot]

Comment on attachment 184206 [details]
Patch

Clearing flags on attachment: 184206

Committed r140611: <http://trac.webkit.org/changeset/140611>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eugene Klyuchnikov]

(In reply to comment #3)
> All reviewed patches have been landed.  Closing bug.

This patch causes REGRESSION: CPU profile can't be recorded from WebInspector.

More precisely: in file ScriptProfiler.cpp it is explicitly declared that "start" and "stop" could be invoked with state == 0 (see "startForPage", "stopForPage", etc.)
Accessing "isolate" of a null pointer causes crashes.

[Kentaro Hara]

(In reply to comment #4)
> This patch causes REGRESSION: CPU profile can't be recorded from WebInspector.

oops, sorry. Let me take a look tomorrow morning.

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 185469 [details]
Patch

[Kentaro Hara]

(In reply to comment #4)
> (In reply to comment #3)
> > All reviewed patches have been landed.  Closing bug.
> 
> This patch causes REGRESSION: CPU profile can't be recorded from WebInspector.
> 
> More precisely: in file ScriptProfiler.cpp it is explicitly declared that "start" and "stop" could be invoked with state == 0 (see "startForPage", "stopForPage", etc.)
> Accessing "isolate" of a null pointer causes crashes.

Thanks for fixing the crash by passing Isolate::GetCurrent(). I uploaded a follow-up patch that uses state->isolate() when state is not 0. (In the future, I think InspectorProfilerAgent should keep ScriptState* and pass it to bindings.)

abarth: review?

[Adam Barth]

Comment on attachment 185469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185469&action=review

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:62
> -    v8::CpuProfiler::StartProfiling(v8String(title, v8::Isolate::GetCurrent()));
> +    v8::CpuProfiler::StartProfiling(v8String(title, state ? state->isolate() : v8::Isolate::GetCurrent()));

So state can be zero here but not in stop() ?

[Kentaro Hara]

Comment on attachment 185469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185469&action=review

>> Source/WebCore/bindings/v8/ScriptProfiler.cpp:62
>> +    v8::CpuProfiler::StartProfiling(v8String(title, state ? state->isolate() : v8::Isolate::GetCurrent()));
> 
> So state can be zero here but not in stop() ?

state can be zero in stop(), but it's OK.

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:81
>      const v8::CpuProfile* profile = state ?
> -        v8::CpuProfiler::StopProfiling(v8String(title, v8::Isolate::GetCurrent()), state->context()->GetSecurityToken()) :
> +        v8::CpuProfiler::StopProfiling(v8String(title, state->isolate()), state->context()->GetSecurityToken()) :

because we're checking state here.

[WebKit Review Bot]

Comment on attachment 185469 [details]
Patch

Clearing flags on attachment: 185469

Committed r141368: <http://trac.webkit.org/changeset/141368>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.