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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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.