Bug 148341

Summary:

Debugger's VM should never be null

Product:

WebKit

Reporter:

Geoffrey Garen <ggaren>

Component:

New Bugs

Assignee:

Geoffrey Garen <ggaren>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, bburg, bfulgham, commit-queue, joepeck, ossy, peavo, timothy

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

148347

Bug Blocks:

Attachments:

Description	Flags
Patch	joepeck: review+

Description Geoffrey Garen 2015-08-21 16:02:49 PDT

Debugger's VM should never be null

Comment 1 Geoffrey Garen 2015-08-21 16:05:24 PDT

Created attachment 259680 [details]
Patch

Comment 2 Saam Barati 2015-08-21 16:07:40 PDT

Comment on attachment 259680 [details]
Patch

LGTM

Comment 3 Joseph Pecoraro 2015-08-21 16:19:37 PDT

Comment on attachment 259680 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.cpp:31
> +#include "JSGlobalObject.h"

I'm not sure this include is needed. There is no new code in this file.

> Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.cpp:77
>  void JSGlobalObjectScriptDebugServer::recompileAllJSFunctions()
>  {
> -    JSC::Debugger::recompileAllJSFunctions(&m_globalObject.vm());
> +    JSC::Debugger::recompileAllJSFunctions();
>  }

Seems you can just remove this virtual method, it just calls its superclass.

> Source/JavaScriptCore/inspector/ScriptDebugServer.h:121
> -#endif // ScriptDebugServer_h
> +#endif // ScriptDebugSer*er_h

Oops

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:87
>      JSC::JSLockHolder lock(vm);

In WorkerScriptDebugServer and PageScriptDebugServer these could both be simplified if they just used (m_vm) instead of re-looking up the vm, that would require an accessor on Debugger. Up to you.

Comment 4 Geoffrey Garen 2015-08-21 16:57:14 PDT

Committed r188803: <http://trac.webkit.org/changeset/188803>

Comment 5 WebKit Commit Bot 2015-08-21 17:49:31 PDT

Re-opened since this is blocked by bug 148347

Comment 6 Geoffrey Garen 2015-08-23 14:28:48 PDT

Committed r188841: <http://trac.webkit.org/changeset/188841>

Comment 7 Csaba Osztrogonác 2015-08-23 14:47:54 PDT

(In reply to comment #6)
> Committed r188841: <http://trac.webkit.org/changeset/188841>

It broke the WinCairo build: https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/48606

Comment 8 Csaba Osztrogonác 2015-08-24 02:21:33 PDT

(In reply to comment #7)
> (In reply to comment #6)
> > Committed r188841: <http://trac.webkit.org/changeset/188841>
> 
> It broke the WinCairo build:
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/48606

build log:

c:\users\alex\documents\wincairobot\win-cairo-release\build\source\webcore\inspector\PageScriptDebugServer.cpp(57): error C2661: 'Inspector::ScriptDebugServer::ScriptDebugServer': no overloaded function takes 2 arguments
c:\users\alex\documents\wincairobot\win-cairo-release\build\source\webcore\inspector\PageScriptDebugServer.cpp(90): error C3861: 'vm': identifier not found
c:\users\alex\documents\wincairobot\win-cairo-release\build\source\webcore\inspector\PageScriptDebugServer.cpp(91): error C2660: 'JSC::Debugger::recompileAllJSFunctions': function does not take 0 arguments

c:\users\alex\documents\wincairobot\win-cairo-release\build\source\webcore\inspector\PageScriptDebugServer.cpp(57): error C2661: 'Inspector::ScriptDebugServer::ScriptDebugServer': no overloaded function takes 2 arguments
c:\users\alex\documents\wincairobot\win-cairo-release\build\source\webcore\inspector\PageScriptDebugServer.cpp(90): error C3861: 'vm': identifier not found
c:\users\alex\documents\wincairobot\win-cairo-release\build\source\webcore\inspector\PageScriptDebugServer.cpp(91): error C2660: 'JSC::Debugger::recompileAllJSFunctions': function does not take 0 arguments