Web Inspector: InspectorFrontendAPIDispatcher should not ignore all exceptions
authorbburg@apple.com <bburg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Dec 2020 21:06:17 +0000 (21:06 +0000)
committerbburg@apple.com <bburg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Dec 2020 21:06:17 +0000 (21:06 +0000)
https://bugs.webkit.org/show_bug.cgi?id=219378

Reviewed by Devin Rousso.

Source/WebCore:

Covered by existing Web Inspector layout tests.

* bindings/js/ScriptController.h: Make evaluateInWorld public and exported.

* inspector/InspectorFrontendAPIDispatcher.h:
* inspector/InspectorFrontendAPIDispatcher.cpp:
(WebCore::InspectorFrontendAPIDispatcher::evaluateOrQueueExpression):
(WebCore::InspectorFrontendAPIDispatcher::evaluateExpression):
Evaluate and pass along the result whether it's a value or exception.

* inspector/InspectorFrontendClientLocal.h:
* inspector/InspectorFrontendClientLocal.cpp:
(WebCore::InspectorFrontendClientLocal::evaluationResultToBoolean):
(WebCore::InspectorFrontendClientLocal::isDebuggingEnabled):
(WebCore::InspectorFrontendClientLocal::isTimelineProfilingEnabled):
(WebCore::InspectorFrontendClientLocal::isProfilingJavaScript):
Refactor the common code to take an EvaluationResult and figure out if the value is true or falsy.

* platform/Logging.h: Add an Inspector logging channel, for logging errors.

Source/WebKit:

The underlying method used for frontend expression evaluations is
ScriptController::evaluateIgnoringExceptions. This method calls
evaluateInWorld and returns nullopt if an exception happens.

Switch to using evaluateInWorld directly and using the existing ValueOrException
type from in WebCore. Change our EvaluationResult type to use ValueOrException
in place of JSC::JSValue. ValueOrException is Expected<JSC::JSValue, ExceptionDetails>
so this is exposing more error information in addition to the JSC::JSValue.

* Platform/Logging.h: Add 'Inspector' log channel for WebKit.framework.

* WebProcess/Inspector/WebInspectorUIExtensionController.cpp:
(WebKit::WebInspectorUIExtensionController::WebInspectorUIExtensionController):
(WebKit::WebInspectorUIExtensionController::~WebInspectorUIExtensionController):
Remove unnecessary debugging code that was accidentally left in/commented out.

* WebProcess/Inspector/WebInspectorUIExtensionController.h:
(WebKit::WebInspectorUIExtensionController::parseInspectorExtensionErrorFromResult): Deleted.
(WebKit::WebInspectorUIExtensionController::parseInspectorExtensionErrorFromEvaluationResult): Added.
(WebKit::WebInspectorUIExtensionController::registerExtension):
(WebKit::WebInspectorUIExtensionController::unregisterExtension):
Adapt to using the new result type. Use the InspectorExtensionID type where possible.

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

Source/WebCore/ChangeLog
Source/WebCore/bindings/js/ScriptController.h
Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp
Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h
Source/WebCore/inspector/InspectorFrontendClientLocal.cpp
Source/WebCore/inspector/InspectorFrontendClientLocal.h
Source/WebCore/platform/Logging.h
Source/WebKit/ChangeLog
Source/WebKit/Platform/Logging.h
Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp
Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h

index df5bd0f..a62ce9d 100644 (file)
@@ -1,3 +1,30 @@
+2020-12-04  Brian Burg  <bburg@apple.com>
+
+        Web Inspector: InspectorFrontendAPIDispatcher should not ignore all exceptions
+        https://bugs.webkit.org/show_bug.cgi?id=219378
+
+        Reviewed by Devin Rousso.
+
+        Covered by existing Web Inspector layout tests.
+
+        * bindings/js/ScriptController.h: Make evaluateInWorld public and exported.
+
+        * inspector/InspectorFrontendAPIDispatcher.h:
+        * inspector/InspectorFrontendAPIDispatcher.cpp:
+        (WebCore::InspectorFrontendAPIDispatcher::evaluateOrQueueExpression):
+        (WebCore::InspectorFrontendAPIDispatcher::evaluateExpression):
+        Evaluate and pass along the result whether it's a value or exception.
+
+        * inspector/InspectorFrontendClientLocal.h:
+        * inspector/InspectorFrontendClientLocal.cpp:
+        (WebCore::InspectorFrontendClientLocal::evaluationResultToBoolean):
+        (WebCore::InspectorFrontendClientLocal::isDebuggingEnabled):
+        (WebCore::InspectorFrontendClientLocal::isTimelineProfilingEnabled):
+        (WebCore::InspectorFrontendClientLocal::isProfilingJavaScript):
+        Refactor the common code to take an EvaluationResult and figure out if the value is true or falsy.
+
+        * platform/Logging.h: Add an Inspector logging channel, for logging errors.
+
 2020-12-04  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][Floats] FloatAvoider does not need to keep a pointer to Layout::Box around.
index c5a6cc0..9643b92 100644 (file)
@@ -104,6 +104,7 @@ public:
     WEBCORE_EXPORT JSC::JSValue executeUserAgentScriptInWorldIgnoringException(DOMWrapperWorld&, const String& script, bool forceUserGesture);
     WEBCORE_EXPORT ValueOrException executeUserAgentScriptInWorld(DOMWrapperWorld&, const String& script, bool forceUserGesture);
     WEBCORE_EXPORT void executeAsynchronousUserAgentScriptInWorld(DOMWrapperWorld&, RunJavaScriptParameters&&, ResolveFunction&&);
+    ValueOrException evaluateInWorld(const ScriptSourceCode&, DOMWrapperWorld&);
     JSC::JSValue evaluateIgnoringException(const ScriptSourceCode&);
     JSC::JSValue evaluateInWorldIgnoringException(const ScriptSourceCode&, DOMWrapperWorld&);
 
@@ -177,7 +178,6 @@ public:
 private:
     ValueOrException executeUserAgentScriptInWorldInternal(DOMWrapperWorld&, RunJavaScriptParameters&&);
     ValueOrException executeScriptInWorld(DOMWrapperWorld&, RunJavaScriptParameters&&);
-    ValueOrException evaluateInWorld(const ScriptSourceCode&, DOMWrapperWorld&);
     ValueOrException callInWorld(RunJavaScriptParameters&&, DOMWrapperWorld&);
     
     void setupModuleScriptHandlers(LoadableModuleScript&, JSC::JSInternalPromise&, DOMWrapperWorld&);
index 02bb4ca..9b01d44 100644 (file)
@@ -165,7 +165,7 @@ void InspectorFrontendAPIDispatcher::evaluateOrQueueExpression(const String& exp
         return;
     }
 
-    JSC::JSValue result = evaluateExpression(expression);
+    ValueOrException result = evaluateExpression(expression);
     if (optionalResultHandler)
         optionalResultHandler(result);
 }
@@ -197,14 +197,14 @@ void InspectorFrontendAPIDispatcher::evaluateQueuedExpressions()
     }
 }
 
-JSC::JSValue InspectorFrontendAPIDispatcher::evaluateExpression(const String& expression)
+ValueOrException InspectorFrontendAPIDispatcher::evaluateExpression(const String& expression)
 {
     ASSERT(m_frontendPage);
     ASSERT(!m_suspended);
     ASSERT(m_queuedEvaluations.isEmpty());
 
     JSC::SuspendExceptionScope scope(&m_frontendPage->inspectorController().vm());
-    return m_frontendPage->mainFrame().script().evaluateIgnoringException(ScriptSourceCode(expression));
+    return m_frontendPage->mainFrame().script().evaluateInWorld(ScriptSourceCode(expression), mainThreadNormalWorld());
 }
 
 void InspectorFrontendAPIDispatcher::evaluateExpressionForTesting(const String& expression)
index 496adec..c599e4a 100644 (file)
@@ -25,7 +25,6 @@
 
 #pragma once
 
-#include <JavaScriptCore/JSCJSValue.h>
 #include <wtf/CompletionHandler.h>
 #include <wtf/Expected.h>
 #include <wtf/JSONValues.h>
 #include <wtf/WeakPtr.h>
 #include <wtf/text/WTFString.h>
 
+namespace JSC {
+class JSGlobalObject;
+class JSValue;
+}
+
 namespace WebCore {
 
 class Page;
+struct ExceptionDetails;
 
 class InspectorFrontendAPIDispatcher final : public RefCounted<InspectorFrontendAPIDispatcher> {
 public:
     enum class EvaluationError { ExecutionSuspended, ContextDestroyed };
-    using EvaluationResult = Expected<JSC::JSValue, EvaluationError>;
+    using ValueOrException = Expected<JSC::JSValue, ExceptionDetails>;
+    using EvaluationResult = Expected<ValueOrException, EvaluationError>;
     using EvaluationResultHandler = CompletionHandler<void(EvaluationResult)>;
 
     enum class UnsuspendSoon { Yes, No };
@@ -78,7 +84,7 @@ private:
     void evaluateOrQueueExpression(const String&, EvaluationResultHandler&& handler = { });
     void evaluateQueuedExpressions();
     void invalidateQueuedExpressions();
-    JSC::JSValue evaluateExpression(const String&);
+    ValueOrException evaluateExpression(const String&);
 
     WeakPtr<Page> m_frontendPage;
     Vector<std::pair<String, EvaluationResultHandler>> m_queuedEvaluations;
index 5071efe..c53a997 100644 (file)
@@ -35,6 +35,7 @@
 #include "Chrome.h"
 #include "DOMWrapperWorld.h"
 #include "Document.h"
+#include "ExceptionDetails.h"
 #include "FloatRect.h"
 #include "Frame.h"
 #include "FrameLoadRequest.h"
@@ -43,6 +44,7 @@
 #include "InspectorController.h"
 #include "InspectorFrontendHost.h"
 #include "InspectorPageAgent.h"
+#include "Logging.h"
 #include "Page.h"
 #include "ScriptController.h"
 #include "ScriptSourceCode.h"
@@ -323,11 +325,23 @@ void InspectorFrontendClientLocal::restoreAttachedWindowHeight()
     setAttachedWindowHeight(constrainedAttachedWindowHeight(preferredHeight, inspectedPageHeight));
 }
 
-bool InspectorFrontendClientLocal::isDebuggingEnabled()
+Optional<bool> InspectorFrontendClientLocal::evaluationResultToBoolean(InspectorFrontendAPIDispatcher::EvaluationResult result)
 {
-    auto result = m_frontendAPIDispatcher->dispatchCommandWithResultSync("isDebuggingEnabled"_s);
-    return result && result.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject());
+    if (!result)
+        return WTF::nullopt;
+
+    auto valueOrException = result.value();
+    if (!valueOrException) {
+        LOG(Inspector, "Encountered exception while evaluating upon the frontend: %s", valueOrException.error().message.utf8().data());
+        return WTF::nullopt;
+    }
 
+    return valueOrException.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject());
+}
+
+bool InspectorFrontendClientLocal::isDebuggingEnabled()
+{
+    return evaluationResultToBoolean(m_frontendAPIDispatcher->dispatchCommandWithResultSync("isDebuggingEnabled"_s)).valueOr(false);
 }
 
 void InspectorFrontendClientLocal::setDebuggingEnabled(bool enabled)
@@ -337,8 +351,7 @@ void InspectorFrontendClientLocal::setDebuggingEnabled(bool enabled)
 
 bool InspectorFrontendClientLocal::isTimelineProfilingEnabled()
 {
-    auto result = m_frontendAPIDispatcher->dispatchCommandWithResultSync("isTimelineProfilingEnabled"_s);
-    return result && result.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject());
+    return evaluationResultToBoolean(m_frontendAPIDispatcher->dispatchCommandWithResultSync("isTimelineProfilingEnabled"_s)).valueOr(false);
 }
 
 void InspectorFrontendClientLocal::setTimelineProfilingEnabled(bool enabled)
@@ -348,8 +361,7 @@ void InspectorFrontendClientLocal::setTimelineProfilingEnabled(bool enabled)
 
 bool InspectorFrontendClientLocal::isProfilingJavaScript()
 {
-    auto result = m_frontendAPIDispatcher->dispatchCommandWithResultSync("isProfilingJavaScript"_s);
-    return result && result.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject());
+    return evaluationResultToBoolean(m_frontendAPIDispatcher->dispatchCommandWithResultSync("isProfilingJavaScript"_s)).valueOr(false);
 }
 
 void InspectorFrontendClientLocal::startProfilingJavaScript()
index ad18f60..bcffd22 100644 (file)
@@ -134,6 +134,7 @@ protected:
 
 private:
     friend class FrontendMenuProvider;
+    Optional<bool> evaluationResultToBoolean(InspectorFrontendAPIDispatcher::EvaluationResult);
 
     InspectorController* m_inspectedPageController { nullptr };
     Page* m_frontendPage { nullptr };
index bf9c95e..86cc77e 100644 (file)
@@ -71,6 +71,7 @@ namespace WebCore {
     M(Images) \
     M(IndexedDB) \
     M(IndexedDBOperations) \
+    M(Inspector) \
     M(Layers) \
     M(Layout) \
     M(FormattingContextLayout) \
index 827865a..9c1626a 100644 (file)
@@ -1,3 +1,33 @@
+2020-12-04  Brian Burg  <bburg@apple.com>
+
+        Web Inspector: InspectorFrontendAPIDispatcher should not ignore all exceptions
+        https://bugs.webkit.org/show_bug.cgi?id=219378
+
+        Reviewed by Devin Rousso.
+
+        The underlying method used for frontend expression evaluations is 
+        ScriptController::evaluateIgnoringExceptions. This method calls
+        evaluateInWorld and returns nullopt if an exception happens.
+
+        Switch to using evaluateInWorld directly and using the existing ValueOrException
+        type from in WebCore. Change our EvaluationResult type to use ValueOrException 
+        in place of JSC::JSValue. ValueOrException is Expected<JSC::JSValue, ExceptionDetails>
+        so this is exposing more error information in addition to the JSC::JSValue.
+
+        * Platform/Logging.h: Add 'Inspector' log channel for WebKit.framework.
+
+        * WebProcess/Inspector/WebInspectorUIExtensionController.cpp:
+        (WebKit::WebInspectorUIExtensionController::WebInspectorUIExtensionController):
+        (WebKit::WebInspectorUIExtensionController::~WebInspectorUIExtensionController):
+        Remove unnecessary debugging code that was accidentally left in/commented out.
+
+        * WebProcess/Inspector/WebInspectorUIExtensionController.h:
+        (WebKit::WebInspectorUIExtensionController::parseInspectorExtensionErrorFromResult): Deleted.
+        (WebKit::WebInspectorUIExtensionController::parseInspectorExtensionErrorFromEvaluationResult): Added.
+        (WebKit::WebInspectorUIExtensionController::registerExtension):
+        (WebKit::WebInspectorUIExtensionController::unregisterExtension):
+        Adapt to using the new result type. Use the InspectorExtensionID type where possible.
+
 2020-12-04  Kate Cheney  <katherine_cheney@apple.com>
 
         Create API to enable/disable text interaction gestures in WKWebView
index 20dcc5e..d085790 100644 (file)
@@ -60,6 +60,7 @@ extern "C" {
     M(Images) \
     M(IncrementalPDF) \
     M(IncrementalPDFVerbose) \
+    M(Inspector) \
     M(IndexedDB) \
     M(KeyHandling) \
     M(Layers) \
index 6275cf6..0fef2f8 100644 (file)
 
 #if ENABLE(INSPECTOR_EXTENSIONS)
 
+#include "Logging.h"
 #include "WebInspectorUI.h"
 #include "WebInspectorUIExtensionControllerMessages.h"
 #include "WebInspectorUIExtensionControllerMessagesReplies.h"
 #include "WebPage.h"
 #include "WebProcess.h"
+#include <JavaScriptCore/JSCInlines.h>
+#include <WebCore/ExceptionDetails.h>
 #include <WebCore/InspectorFrontendAPIDispatcher.h>
 
 namespace WebKit {
@@ -43,27 +46,42 @@ WebInspectorUIExtensionController::WebInspectorUIExtensionController(WebCore::In
     Page* page = inspectorFrontend.frontendPage();
     ASSERT(page);
 
-//    WTFReportBacktrace();
     WebProcess::singleton().addMessageReceiver(Messages::WebInspectorUIExtensionController::messageReceiverName(), WebPage::fromCorePage(*page).identifier(), *this);
 }
 
 WebInspectorUIExtensionController::~WebInspectorUIExtensionController()
 {
-//    WTFReportBacktrace();
     WebProcess::singleton().removeMessageReceiver(*this);
 }
 
-Optional<InspectorExtensionError> WebInspectorUIExtensionController::parseInspectorExtensionErrorFromResult(JSC::JSValue result)
+Optional<InspectorExtensionError> WebInspectorUIExtensionController::parseInspectorExtensionErrorFromEvaluationResult(InspectorFrontendAPIDispatcher::EvaluationResult result)
 {
+    if (!result.has_value()) {
+        switch (result.error()) {
+        case WebCore::InspectorFrontendAPIDispatcher::EvaluationError::ContextDestroyed:
+            return InspectorExtensionError::ContextDestroyed;
+        case WebCore::InspectorFrontendAPIDispatcher::EvaluationError::ExecutionSuspended:
+            return InspectorExtensionError::InternalError;
+        }
+    }
+
     ASSERT(m_frontendClient);
     auto globalObject = m_frontendClient->frontendAPIDispatcher().frontendGlobalObject();
     if (!globalObject)
         return InspectorExtensionError::ContextDestroyed;
 
+    auto valueOrException = result.value();
+    if (!valueOrException.has_value()) {
+        LOG(Inspector, "Encountered exception while evaluating upon the frontend: %s", valueOrException.error().message.utf8().data());
+        return InspectorExtensionError::InternalError;
+    }
+    
     // If the evaluation result is a string, the frontend returned an error string.
+    // If there was a JS exception, log it and return error code InternalError.
     // Anything else (falsy values, objects, arrays, DOM, etc.) is interpreted as success.
-    if (result.isString()) {
-        auto resultString = result.toWTFString(globalObject);
+    JSC::JSValue scriptValue = valueOrException.value();
+    if (scriptValue.isString()) {
+        auto resultString = scriptValue.toWTFString(globalObject);
         if (resultString == "ContextDestroyed"_s)
             return InspectorExtensionError::ContextDestroyed;
         if (resultString == "InternalError"_s)
@@ -82,7 +100,7 @@ Optional<InspectorExtensionError> WebInspectorUIExtensionController::parseInspec
 
 // WebInspectorUIExtensionController IPC messages.
 
-void WebInspectorUIExtensionController::registerExtension(const String& extensionID, const String& displayName, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&& completionHandler)
+void WebInspectorUIExtensionController::registerExtension(const InspectorExtensionID& extensionID, const String& displayName, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&& completionHandler)
 {
     if (!m_frontendClient) {
         completionHandler(makeUnexpected(InspectorExtensionError::InvalidRequest));
@@ -99,7 +117,7 @@ void WebInspectorUIExtensionController::registerExtension(const String& extensio
             return;
         }
 
-        if (auto parsedError = weakThis->parseInspectorExtensionErrorFromResult(result.value())) {
+        if (auto parsedError = weakThis->parseInspectorExtensionErrorFromEvaluationResult(result)) {
             completionHandler(makeUnexpected(parsedError.value()));
             return;
         }
@@ -108,7 +126,7 @@ void WebInspectorUIExtensionController::registerExtension(const String& extensio
     });
 }
 
-void WebInspectorUIExtensionController::unregisterExtension(const String& extensionID, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&& completionHandler)
+void WebInspectorUIExtensionController::unregisterExtension(const InspectorExtensionID& extensionID, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&& completionHandler)
 {
     if (!m_frontendClient) {
         completionHandler(makeUnexpected(InspectorExtensionError::InvalidRequest));
@@ -122,7 +140,7 @@ void WebInspectorUIExtensionController::unregisterExtension(const String& extens
             return;
         }
 
-        if (auto parsedError = weakThis->parseInspectorExtensionErrorFromResult(result.value())) {
+        if (auto parsedError = weakThis->parseInspectorExtensionErrorFromEvaluationResult(result.value())) {
             completionHandler(makeUnexpected(parsedError.value()));
             return;
         }
index f5b40ab..65b1528 100644 (file)
@@ -30,6 +30,7 @@
 #include "Connection.h"
 #include "InspectorExtensionTypes.h"
 #include "MessageReceiver.h"
+#include <WebCore/InspectorFrontendAPIDispatcher.h>
 #include <wtf/Forward.h>
 #include <wtf/Noncopyable.h>
 #include <wtf/URL.h>
@@ -37,6 +38,7 @@
 
 namespace JSC {
 class JSValue;
+class JSObject;
 }
 
 namespace WebCore {
@@ -60,11 +62,11 @@ public:
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
 
     // WebInspectorUIExtensionController IPC messages.
-    void registerExtension(const String& extensionID, const String& displayName, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&&);
-    void unregisterExtension(const String& extensionID, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&&);
+    void registerExtension(const InspectorExtensionID&, const String& displayName, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&&);
+    void unregisterExtension(const InspectorExtensionID&, CompletionHandler<void(Expected<bool, InspectorExtensionError>)>&&);
 
 private:
-    Optional<InspectorExtensionError> parseInspectorExtensionErrorFromResult(JSC::JSValue result);
+    Optional<InspectorExtensionError> parseInspectorExtensionErrorFromEvaluationResult(WebCore::InspectorFrontendAPIDispatcher::EvaluationResult);
 
     WeakPtr<WebCore::InspectorFrontendClient> m_frontendClient;
 };