Bug 107674 - [V8] Reduce usage of deprecatedV8String() and deprecatedV8Integer()
Summary: [V8] Reduce usage of deprecatedV8String() and deprecatedV8Integer()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 05:14 PST by Kentaro Hara
Modified: 2013-01-30 20:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (22.10 KB, patch)
2013-01-23 05:15 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2013-01-30 05:04 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-01-23 05:14:30 PST
A bit more work is needed to kill them completely.
Comment 1 Kentaro Hara 2013-01-23 05:15:48 PST
Created attachment 184206 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-23 16:31:29 PST
Comment on attachment 184206 [details]
Patch

Clearing flags on attachment: 184206

Committed r140611: <http://trac.webkit.org/changeset/140611>
Comment 3 WebKit Review Bot 2013-01-23 16:31:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Eugene Klyuchnikov 2013-01-28 03:36:36 PST
(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.
Comment 5 Kentaro Hara 2013-01-28 05:57:24 PST
(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.
Comment 6 Kentaro Hara 2013-01-30 05:04:55 PST
Reopening to attach new patch.
Comment 7 Kentaro Hara 2013-01-30 05:04:59 PST
Created attachment 185469 [details]
Patch
Comment 8 Kentaro Hara 2013-01-30 05:08:41 PST
(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?
Comment 9 Adam Barth 2013-01-30 11:57:00 PST
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() ?
Comment 10 Kentaro Hara 2013-01-30 20:17:13 PST
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.
Comment 11 WebKit Review Bot 2013-01-30 20:33:53 PST
Comment on attachment 185469 [details]
Patch

Clearing flags on attachment: 185469

Committed r141368: <http://trac.webkit.org/changeset/141368>
Comment 12 WebKit Review Bot 2013-01-30 20:33:57 PST
All reviewed patches have been landed.  Closing bug.