Bug 238032 – WKBundlePageUIClient console message support should include source URL, column number, and console messages with arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Jeff Miller]

WKBundlePageUIClient console message support should include source URL, column number, and console messages with arguments

[Jeff Miller]

rdar://90439983

[Jeff Miller]

Created attachment 455016 [details]
Patch

[Jeff Miller]

It appears this change inserts an extra space in console messages, which breaks test expectations. For example:

-CONSOLE MESSAGE: This test passes if it does not crash.
+CONSOLE MESSAGE:  This test passes if it does not crash.

I'll fix this before landing.

[Jeff Miller]

Created attachment 455056 [details]
Patch

[Jeff Miller]

Created attachment 455063 [details]
Patch

[Jeff Miller]

Not surprisingly, now that we include console message arguments, some existing test expectations are now incorrect. For example:

-CONSOLE MESSAGE: [loadCrossOriginImage]
+CONSOLE MESSAGE: [loadCrossOriginImage]   trying http://localhost:8000/webgl/resources/webgl_test_files/resources/opengl_logo.jpg

Also, I think the fact that we always call addMessageToConsole() now, even if the message is empty, is causing issues. For example, an expected result is:

Test for the Console.messagesCleared event.

and the actual result is:

CONSOLE MESSAGE:
CONSOLE MESSAGE:
Test for the Console.messagesCleared event.

The latter issue is a bug in this patch, but the former issue is trickier. Rather than changing the existing behavior of the willAddMessageToConsole callback, a more conservative change is to only include the message arguments when using the new willAddMessageWithDetailsToConsole callback.

[Jeff Miller]

Created attachment 455115 [details]
Patch

[Jeff Miller]

The win bot passes now, but the console-log-proxy test on the gtk-wk2 bot is failing:

https://ews-build.s3-us-west-2.amazonaws.com/GTK-WK2-Tests-EWS/455115-11928/inspector/console/console-log-proxy-diff.txt

--- /home/ews/worker/GTK-WK2-Tests-EWS/build/layout-test-results/inspector/console/console-log-proxy-expected.txt
+++ /home/ews/worker/GTK-WK2-Tests-EWS/build/layout-test-results/inspector/console/console-log-proxy-actual.txt
@@ -1,14 +1,21 @@
 CONSOLE MESSAGE: [object Proxy]
 CONSOLE MESSAGE: 0
 CONSOLE MESSAGE: [object Proxy]
+CONSOLE MESSAGE: 1
+CONSOLE MESSAGE: 1
+CONSOLE MESSAGE: 1
 CONSOLE MESSAGE: 1
 Tests for the console.log with Proxy objects.

I'll leave the patch up, I want to see what happens with the mac-wk2 bot, but I definitely seem to have regressed the console-log-proxy, although I have no idea how.

[Jeff Miller]

Tim Hatcher pointed out that the console-log-proxy is likely failing because I'm walking through all the arguments, fetching them, and converting them to strings. This causes the Proxy object to be fetched. I need to be implementing this more like ScriptArguments::getFirstArgumentAsString() and do special Proxy object handling.

[Jeff Miller]

Created attachment 456068 [details]
Patch

[Alex Christensen]

Comment on attachment 456068 [details]
Patch

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

No test?  BundlePageConsoleMessage uses willAddMessageToConsole, so a similar technique could be used to test this.

> Source/WebCore/ChangeLog:9
> +        Add a new ChromeClient addMessageWithArgumentsToConsole() member function for adding a console
> +        message that includes formatted arguments. I left the existing behavior of addMessageToConsole()

I can't say I think this is a great API.  I would expect a WKArrayRef rather than a space separated list inside the string with the message, for which you have to assume that none of the arguments have spaces in them if you want to do anything with them.

> Source/WebCore/page/PageConsoleClient.cpp:222
> +        for (size_t i = 1; i < arguments->argumentCount(); ++i) {

This is basically duplicate code.

[Jeff Miller]

Comment on attachment 456068 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        message that includes formatted arguments. I left the existing behavior of addMessageToConsole()
> 
> I can't say I think this is a great API.  I would expect a WKArrayRef rather than a space separated list inside the string with the message, for which you have to assume that none of the arguments have spaces in them if you want to do anything with them.

Since you mention WKArrayRef, I assume you're talking about the API exposed to the injected bundle:

typedef void (*WKBundlePageWillAddMessageWithDetailsToConsoleCallback)(WKBundlePageRef page, WKStringRef message, uint32_t lineNumber, uint32_t columnNumber, WKStringRef sourceID, const void *clientInfo);

The intent of this patch was to better support writing client-side tests that typically look for complete messages from JavaScript, but we can still do this while providing more information about the individual parameters as you suggested. I'll change this.

>> Source/WebCore/page/PageConsoleClient.cpp:222
>> +        for (size_t i = 1; i < arguments->argumentCount(); ++i) {
> 
> This is basically duplicate code.

Good point, I'll refactor this logic into a separate function.

[Jeff Miller]

I'll add a test as well.

[Jeff Miller]

Created attachment 456304 [details]
Patch

[Alex Christensen]

Comment on attachment 456304 [details]
Patch

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

> Source/JavaScriptCore/inspector/ScriptArguments.cpp:88
> +bool ScriptArguments::getArgumentsAsStrings(Vector<String>& result) const

Vector<String> ScriptArguments::getArgumentsAsStrings() const
Can return an empty vector.
Or it could return a std::optional<Vector<String>>

> Source/JavaScriptCore/inspector/ScriptArguments.cpp:108
> +        if (JSC::jsDynamicCast<JSC::ProxyObject*>(globalObject->vm(), value)) {
> +            result.append("[object Proxy]"_s);
> +            continue;
> +        }
> +
> +        auto scope = DECLARE_CATCH_SCOPE(globalObject->vm());
> +        result.append(value.toWTFString(globalObject));
> +        scope.clearException();

This is all duplicate code.

> Source/WebCore/page/PageConsoleClient.cpp:130
> +                messageArguments.remove(0);

This is the least efficient place to remove in a Vector.  Let's not do this.
Could we instead make a createStringArray that takes a Span<String> and pass a Span<String> through addMessageWithArgumentsToConsole containing indices 1 through the end?  We'd have to do a bounds check here.

> Source/WebCore/page/PageConsoleClient.cpp:188
> +        messageText = messageArguments[0];

ditto.

[Jeff Miller]

Created attachment 456419 [details]
Patch

[Alex Christensen]

Comment on attachment 456419 [details]
Patch

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

> Source/JavaScriptCore/inspector/ScriptArguments.cpp:111
> +    auto* globalObject = this->globalObject();
> +    if (!globalObject) {
> +        ASSERT_NOT_REACHED();
> +        return std::nullopt;
> +    }

This should be removed.  It's redundant and unused.

> Source/WebCore/page/PageConsoleClient.cpp:126
> +    ASSERT(!argumentsVector.isEmpty());
> +    if (size_t numberOfAdditionalArguments = argumentsVector.size() - 1)
> +        return argumentsVector.span().subspan(1, numberOfAdditionalArguments);
> +
> +    return { };

I think it would be safer if you checked instead of asserted that the vector is empty.
You also don't need to check if numberOfAdditionalArguments is nonzero because if it is zero it will just return an empty span.
I also don't think subspan needs a second argument in this case.

> Source/WebKit/Shared/API/APIArray.cpp:53
> +    return createStringArray(Vector<WTF::String>(strings));

This is an unneeded allocation

[Jeff Miller]

Comment on attachment 456419 [details]
Patch

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

>> Source/JavaScriptCore/inspector/ScriptArguments.cpp:111
>> +    }
> 
> This should be removed.  It's redundant and unused.

Yup, just missed this during the refactoring.

[Alex Christensen]

Created attachment 456428 [details]
Patch

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[Alex Christensen]

r292254

[Alex Christensen]

http://trac.webkit.org/r292259