Summary: | [V8] Reduce usage of deprecatedV8String() and deprecatedV8Integer() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||
Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, eustas, japhet, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Kentaro Hara
2013-01-23 05:14:30 PST
Created attachment 184206 [details]
Patch
Comment on attachment 184206 [details] Patch Clearing flags on attachment: 184206 Committed r140611: <http://trac.webkit.org/changeset/140611> All reviewed patches have been landed. Closing bug. (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. (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. Reopening to attach new patch. Created attachment 185469 [details]
Patch
(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 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 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 on attachment 185469 [details] Patch Clearing flags on attachment: 185469 Committed r141368: <http://trac.webkit.org/changeset/141368> All reviewed patches have been landed. Closing bug. |