Debugger's VM should never be null
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Aug 2015 23:57:07 +0000 (23:57 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Aug 2015 23:57:07 +0000 (23:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=148341

Reviewed by Joseph Pecoraro.

It doesn't make sense for a Debugger's VM to be null, and code related
to maintaining that illusion just caused the Web Inspector to crash on
launch (https://bugs.webkit.org/show_bug.cgi?id=148312). So, let's stop
doing that.

Now, Debugger requires its subclass to provide a never-null VM&.

Source/JavaScriptCore:

Also took the opportunity, based on review feedback, to remove some
confusion in the virtual recompileAllJSFunctions hierarchy, by eliminating
the pure virtual in ScriptDebugServer and the unnecessary override in
JSGlobalObjectScriptDebugServer.

* debugger/Debugger.cpp:
(JSC::Debugger::Debugger):
(JSC::Debugger::attach):
(JSC::Debugger::detach):
(JSC::Debugger::isAttached):
(JSC::Debugger::setSteppingMode):
(JSC::Debugger::registerCodeBlock):
(JSC::Debugger::toggleBreakpoint):
(JSC::Debugger::recompileAllJSFunctions):
(JSC::Debugger::setBreakpoint):
(JSC::Debugger::clearBreakpoints):
(JSC::Debugger::clearDebuggerRequests):
(JSC::Debugger::setBreakpointsActivated):
(JSC::Debugger::breakProgram):
(JSC::Debugger::stepOutOfFunction):
(JSC::Debugger::returnEvent):
(JSC::Debugger::didExecuteProgram):
* debugger/Debugger.h:
* inspector/JSGlobalObjectScriptDebugServer.cpp:
(Inspector::JSGlobalObjectScriptDebugServer::JSGlobalObjectScriptDebugServer):
(Inspector::JSGlobalObjectScriptDebugServer::recompileAllJSFunctions):
(Inspector::JSGlobalObjectScriptDebugServer::runEventLoopWhilePaused):
* inspector/ScriptDebugServer.cpp:
(Inspector::ScriptDebugServer::ScriptDebugServer):
* inspector/ScriptDebugServer.h:

Source/WebCore:

* bindings/js/WorkerScriptDebugServer.cpp:
(WebCore::WorkerScriptDebugServer::WorkerScriptDebugServer):
(WebCore::WorkerScriptDebugServer::recompileAllJSFunctions):
(WebCore::WorkerScriptDebugServer::runEventLoopWhilePaused):
* inspector/PageScriptDebugServer.cpp:
(WebCore::PageScriptDebugServer::recompileAllJSFunctions):
(WebCore::PageScriptDebugServer::didPause):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@188803 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/debugger/Debugger.cpp
Source/JavaScriptCore/debugger/Debugger.h
Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.cpp
Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.h
Source/JavaScriptCore/inspector/ScriptDebugServer.cpp
Source/JavaScriptCore/inspector/ScriptDebugServer.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp
Source/WebCore/inspector/PageScriptDebugServer.cpp
Source/WebKit/mac/WebView/WebScriptDebugger.mm

index 37a0537..356a5f4 100644 (file)
@@ -1,3 +1,48 @@
+2015-08-21  Geoffrey Garen  <ggaren@apple.com>
+
+        Debugger's VM should never be null
+        https://bugs.webkit.org/show_bug.cgi?id=148341
+
+        Reviewed by Joseph Pecoraro.
+
+        It doesn't make sense for a Debugger's VM to be null, and code related
+        to maintaining that illusion just caused the Web Inspector to crash on
+        launch (https://bugs.webkit.org/show_bug.cgi?id=148312). So, let's stop
+        doing that.
+
+        Now, Debugger requires its subclass to provide a never-null VM&.
+
+        Also took the opportunity, based on review feedback, to remove some
+        confusion in the virtual recompileAllJSFunctions hierarchy, by eliminating
+        the pure virtual in ScriptDebugServer and the unnecessary override in
+        JSGlobalObjectScriptDebugServer.
+
+        * debugger/Debugger.cpp:
+        (JSC::Debugger::Debugger):
+        (JSC::Debugger::attach):
+        (JSC::Debugger::detach):
+        (JSC::Debugger::isAttached):
+        (JSC::Debugger::setSteppingMode):
+        (JSC::Debugger::registerCodeBlock):
+        (JSC::Debugger::toggleBreakpoint):
+        (JSC::Debugger::recompileAllJSFunctions):
+        (JSC::Debugger::setBreakpoint):
+        (JSC::Debugger::clearBreakpoints):
+        (JSC::Debugger::clearDebuggerRequests):
+        (JSC::Debugger::setBreakpointsActivated):
+        (JSC::Debugger::breakProgram):
+        (JSC::Debugger::stepOutOfFunction):
+        (JSC::Debugger::returnEvent):
+        (JSC::Debugger::didExecuteProgram):
+        * debugger/Debugger.h:
+        * inspector/JSGlobalObjectScriptDebugServer.cpp:
+        (Inspector::JSGlobalObjectScriptDebugServer::JSGlobalObjectScriptDebugServer):
+        (Inspector::JSGlobalObjectScriptDebugServer::recompileAllJSFunctions):
+        (Inspector::JSGlobalObjectScriptDebugServer::runEventLoopWhilePaused):
+        * inspector/ScriptDebugServer.cpp:
+        (Inspector::ScriptDebugServer::ScriptDebugServer):
+        * inspector/ScriptDebugServer.h:
+
 2015-08-21  Basile Clement  <basile_clement@apple.com>
 
         Remove unused code relative to allocation sinking
index b6aab87..9460552 100644 (file)
@@ -79,8 +79,8 @@ private:
     Debugger& m_debugger;
 };
 
-Debugger::Debugger(bool isInWorkerThread)
-    : m_vm(nullptr)
+Debugger::Debugger(VM& vm, bool isInWorkerThread)
+    : m_vm(vm)
     , m_pauseOnExceptionsState(DontPauseOnExceptions)
     , m_pauseOnNextStatement(false)
     , m_isPaused(false)
@@ -108,16 +108,12 @@ Debugger::~Debugger()
 void Debugger::attach(JSGlobalObject* globalObject)
 {
     ASSERT(!globalObject->debugger());
-    if (!m_vm)
-        m_vm = &globalObject->vm();
-    else
-        ASSERT(m_vm == &globalObject->vm());
     globalObject->setDebugger(this);
     m_globalObjects.add(globalObject);
 
     // Call sourceParsed() because it will execute JavaScript in the inspector.
-    for (size_t i = 0; i < m_vm->heap.compiledCode().size(); ++i) {
-        ExecutableBase* base = m_vm->heap.compiledCode()[i];
+    for (size_t i = 0; i < m_vm.heap.compiledCode().size(); ++i) {
+        ExecutableBase* base = m_vm.heap.compiledCode()[i];
         if (!base->isFunctionExecutable())
             continue;
         FunctionExecutable* executable = static_cast<FunctionExecutable*>(base);
@@ -146,8 +142,6 @@ void Debugger::detach(JSGlobalObject* globalObject, ReasonForDetach reason)
         clearDebuggerRequests(globalObject);
 
     globalObject->setDebugger(0);
-    if (!m_globalObjects.size())
-        m_vm = nullptr;
 }
 
 bool Debugger::isAttached(JSGlobalObject* globalObject)
@@ -181,12 +175,12 @@ private:
 
 void Debugger::setSteppingMode(SteppingMode mode)
 {
-    if (mode == m_steppingMode || !m_vm)
+    if (mode == m_steppingMode)
         return;
 
     m_steppingMode = mode;
     SetSteppingModeFunctor functor(this, mode);
-    m_vm->heap.forEachCodeBlock(functor);
+    m_vm.heap.forEachCodeBlock(functor);
 }
 
 void Debugger::registerCodeBlock(CodeBlock* codeBlock)
@@ -271,15 +265,13 @@ private:
 
 void Debugger::toggleBreakpoint(Breakpoint& breakpoint, Debugger::BreakpointState enabledOrNot)
 {
-    if (!m_vm)
-        return;
     ToggleBreakpointFunctor functor(this, breakpoint, enabledOrNot);
-    m_vm->heap.forEachCodeBlock(functor);
+    m_vm.heap.forEachCodeBlock(functor);
 }
 
-void Debugger::recompileAllJSFunctions(VM* vm)
+void Debugger::recompileAllJSFunctions()
 {
-    vm->deleteAllCode();
+    m_vm.deleteAllCode();
 }
 
 BreakpointID Debugger::setBreakpoint(Breakpoint breakpoint, unsigned& actualLine, unsigned& actualColumn)
@@ -444,10 +436,8 @@ void Debugger::clearBreakpoints()
     m_breakpointIDToBreakpoint.clear();
     m_sourceIDToBreakpoints.clear();
 
-    if (!m_vm)
-        return;
     ClearCodeBlockDebuggerRequestsFunctor functor(this);
-    m_vm->heap.forEachCodeBlock(functor);
+    m_vm.heap.forEachCodeBlock(functor);
 }
 
 class Debugger::ClearDebuggerRequestsFunctor {
@@ -470,9 +460,8 @@ private:
 
 void Debugger::clearDebuggerRequests(JSGlobalObject* globalObject)
 {
-    ASSERT(m_vm);
     ClearDebuggerRequestsFunctor functor(globalObject);
-    m_vm->heap.forEachCodeBlock(functor);
+    m_vm.heap.forEachCodeBlock(functor);
 }
 
 void Debugger::setBreakpointsActivated(bool activated)
@@ -499,7 +488,7 @@ void Debugger::breakProgram()
 
     m_pauseOnNextStatement = true;
     setSteppingMode(SteppingModeEnabled);
-    m_currentCallFrame = m_vm->topCallFrame;
+    m_currentCallFrame = m_vm.topCallFrame;
     ASSERT(m_currentCallFrame);
     pauseIfNeeded(m_currentCallFrame);
 }
@@ -537,7 +526,7 @@ void Debugger::stepOutOfFunction()
     if (!m_isPaused)
         return;
 
-    VMEntryFrame* topVMEntryFrame = m_vm->topVMEntryFrame;
+    VMEntryFrame* topVMEntryFrame = m_vm.topVMEntryFrame;
     m_pauseOnCallFrame = m_currentCallFrame ? m_currentCallFrame->callerFrame(topVMEntryFrame) : 0;
     notifyDoneProcessingDebuggerEvents();
 }
@@ -661,11 +650,11 @@ void Debugger::returnEvent(CallFrame* callFrame)
 
     // Treat stepping over a return statement like stepping out.
     if (m_currentCallFrame == m_pauseOnCallFrame) {
-        VMEntryFrame* topVMEntryFrame = m_vm->topVMEntryFrame;
+        VMEntryFrame* topVMEntryFrame = m_vm.topVMEntryFrame;
         m_pauseOnCallFrame = m_currentCallFrame->callerFrame(topVMEntryFrame);
     }
 
-    VMEntryFrame* topVMEntryFrame = m_vm->topVMEntryFrame;
+    VMEntryFrame* topVMEntryFrame = m_vm.topVMEntryFrame;
     m_currentCallFrame = m_currentCallFrame->callerFrame(topVMEntryFrame);
 }
 
@@ -696,12 +685,12 @@ void Debugger::didExecuteProgram(CallFrame* callFrame)
     if (!m_currentCallFrame)
         return;
     if (m_currentCallFrame == m_pauseOnCallFrame) {
-        VMEntryFrame* topVMEntryFrame = m_vm->topVMEntryFrame;
+        VMEntryFrame* topVMEntryFrame = m_vm.topVMEntryFrame;
         m_pauseOnCallFrame = m_currentCallFrame->callerFrame(topVMEntryFrame);
         if (!m_currentCallFrame)
             return;
     }
-    VMEntryFrame* topVMEntryFrame = m_vm->topVMEntryFrame;
+    VMEntryFrame* topVMEntryFrame = m_vm.topVMEntryFrame;
     m_currentCallFrame = m_currentCallFrame->callerFrame(topVMEntryFrame);
 }
 
index e8ce8f1..b753d23 100644 (file)
@@ -44,9 +44,11 @@ typedef ExecState CallFrame;
 
 class JS_EXPORT_PRIVATE Debugger {
 public:
-    Debugger(bool isInWorkerThread = false);
+    Debugger(VM&, bool isInWorkerThread = false);
     virtual ~Debugger();
 
+    VM& vm() { return m_vm; }
+
     JSC::DebuggerCallFrame* currentDebuggerCallFrame() const;
     bool hasHandlerForExceptionCallback() const
     {
@@ -118,7 +120,7 @@ public:
     void didExecuteProgram(CallFrame*);
     void didReachBreakpoint(CallFrame*);
 
-    void recompileAllJSFunctions(VM*);
+    virtual void recompileAllJSFunctions();
 
     void registerCodeBlock(CodeBlock*);
 
@@ -185,7 +187,7 @@ private:
 
     void clearDebuggerRequests(JSGlobalObject*);
 
-    VM* m_vm;
+    VM& m_vm;
     HashSet<JSGlobalObject*> m_globalObjects;
 
     PauseOnExceptionsState m_pauseOnExceptionsState;
index 0e968f7..9b255bb 100644 (file)
@@ -35,7 +35,7 @@ using namespace JSC;
 namespace Inspector {
 
 JSGlobalObjectScriptDebugServer::JSGlobalObjectScriptDebugServer(JSGlobalObject& globalObject)
-    : ScriptDebugServer(false)
+    : ScriptDebugServer(globalObject.vm(), false)
     , m_globalObject(globalObject)
 {
 }
@@ -70,11 +70,6 @@ void JSGlobalObjectScriptDebugServer::removeListener(ScriptDebugListener* listen
     }
 }
 
-void JSGlobalObjectScriptDebugServer::recompileAllJSFunctions()
-{
-    JSC::Debugger::recompileAllJSFunctions(&m_globalObject.vm());
-}
-
 void JSGlobalObjectScriptDebugServer::runEventLoopWhilePaused()
 {
     // Drop all locks so another thread can work in the VM while we are nested.
index 901b673..3cf6c72 100644 (file)
@@ -42,8 +42,6 @@ public:
 
     JSC::JSGlobalObject& globalObject() const { return m_globalObject; }
 
-    virtual void recompileAllJSFunctions() override;
-
 private:
     virtual ListenerSet& getListeners() override { return m_listeners; }
     virtual void didPause(JSC::JSGlobalObject*) override { }
index 356de1c..cb34d61 100644 (file)
@@ -47,8 +47,8 @@ using namespace JSC;
 
 namespace Inspector {
 
-ScriptDebugServer::ScriptDebugServer(bool isInWorkerThread)
-    : Debugger(isInWorkerThread)
+ScriptDebugServer::ScriptDebugServer(VM& vm, bool isInWorkerThread)
+    : Debugger(vm, isInWorkerThread)
 {
 }
 
index 99eb723..b72af77 100644 (file)
@@ -44,6 +44,7 @@
 namespace JSC {
 class ExecState;
 class JSGlobalObject;
+class VM;
 }
 
 namespace Inspector {
@@ -56,8 +57,6 @@ public:
     void removeBreakpoint(JSC::BreakpointID);
     void clearBreakpoints();
 
-    virtual void recompileAllJSFunctions() = 0;
-
     const BreakpointActions& getActionsForBreakpoint(JSC::BreakpointID);
 
     class Task {
@@ -71,7 +70,7 @@ protected:
     typedef HashSet<ScriptDebugListener*> ListenerSet;
     typedef void (ScriptDebugServer::*JavaScriptExecutionCallback)(ScriptDebugListener*);
 
-    ScriptDebugServer(bool isInWorkerThread = false);
+    ScriptDebugServer(JSC::VM&, bool isInWorkerThread = false);
     ~ScriptDebugServer();
 
     virtual ListenerSet& getListeners() = 0;
index 9796eeb..6d31779 100644 (file)
@@ -1,3 +1,25 @@
+2015-08-21  Geoffrey Garen  <ggaren@apple.com>
+
+        Debugger's VM should never be null
+        https://bugs.webkit.org/show_bug.cgi?id=148341
+
+        Reviewed by Joseph Pecoraro.
+
+        It doesn't make sense for a Debugger's VM to be null, and code related
+        to maintaining that illusion just caused the Web Inspector to crash on
+        launch (https://bugs.webkit.org/show_bug.cgi?id=148312). So, let's stop
+        doing that.
+
+        Now, Debugger requires its subclass to provide a never-null VM&.
+
+        * bindings/js/WorkerScriptDebugServer.cpp:
+        (WebCore::WorkerScriptDebugServer::WorkerScriptDebugServer):
+        (WebCore::WorkerScriptDebugServer::recompileAllJSFunctions):
+        (WebCore::WorkerScriptDebugServer::runEventLoopWhilePaused):
+        * inspector/PageScriptDebugServer.cpp:
+        (WebCore::PageScriptDebugServer::recompileAllJSFunctions):
+        (WebCore::PageScriptDebugServer::didPause):
+
 2015-08-21  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [OS X] Remove dead code from FontCache::systemFallbackForCharacters()
index dd759b0..dce2a31 100644 (file)
@@ -45,7 +45,7 @@ using namespace Inspector;
 namespace WebCore {
 
 WorkerScriptDebugServer::WorkerScriptDebugServer(WorkerGlobalScope* context, const String& mode)
-    : ScriptDebugServer(true)
+    : ScriptDebugServer(context->script()->vm(), true)
     , m_workerGlobalScope(context)
     , m_debuggerTaskMode(mode)
 {
@@ -82,10 +82,8 @@ void WorkerScriptDebugServer::removeListener(ScriptDebugListener* listener, bool
 
 void WorkerScriptDebugServer::recompileAllJSFunctions()
 {
-    JSC::VM& vm = m_workerGlobalScope->script()->vm();
-
-    JSC::JSLockHolder lock(vm);
-    JSC::Debugger::recompileAllJSFunctions(&vm);
+    JSC::JSLockHolder lock(vm());
+    JSC::Debugger::recompileAllJSFunctions();
 }
 
 void WorkerScriptDebugServer::runEventLoopWhilePaused()
index dbd11cd..df820a9 100644 (file)
@@ -52,7 +52,7 @@ using namespace Inspector;
 namespace WebCore {
 
 PageScriptDebugServer::PageScriptDebugServer(Page& page)
-    : ScriptDebugServer(false)
+    : ScriptDebugServer(WebCore::JSDOMWindowBase::commonVM(), false)
     , m_page(page)
 {
 }
@@ -87,8 +87,8 @@ void PageScriptDebugServer::removeListener(ScriptDebugListener* listener, bool i
 
 void PageScriptDebugServer::recompileAllJSFunctions()
 {
-    JSLockHolder lock(JSDOMWindow::commonVM());
-    Debugger::recompileAllJSFunctions(&JSDOMWindow::commonVM());
+    JSLockHolder lock(vm());
+    Debugger::recompileAllJSFunctions();
 }
 
 void PageScriptDebugServer::didPause(JSGlobalObject*)
@@ -109,7 +109,7 @@ void PageScriptDebugServer::runEventLoopWhilePaused()
     // we need to gracefully handle releasing and reacquiring the lock.
     if (WebThreadIsEnabled()) {
         ASSERT(WebThreadIsLockedOrDisabled());
-        JSC::JSLock::DropAllLocks dropAllLocks(WebCore::JSDOMWindowBase::commonVM());
+        JSC::JSLock::DropAllLocks dropAllLocks(vm());
         WebRunLoopEnableNested();
 
         runEventLoopWhilePausedInternal();
index 6d52651..fefc3ab 100644 (file)
@@ -72,7 +72,8 @@ static WebFrame *toWebFrame(JSGlobalObject* globalObject)
 }
 
 WebScriptDebugger::WebScriptDebugger(JSGlobalObject* globalObject)
-    : m_callingDelegate(false)
+    : Debugger(globalObject->vm())
+    , m_callingDelegate(false)
     , m_globalObject(globalObject->vm(), globalObject)
 {
     setPauseOnExceptionsState(PauseOnAllExceptions);