Bug 127944 – Web Inspector: Expose the console object in JSContexts to interact with Web Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

console.log(...) is a critical development tool for those working on web pages. Inspecting a JSContext there is currently no way to interact with a debugger. We should look into providing something like this.

Perhaps we could port the console object, or a subset into JavaScriptCore and make it a runtime option in a global object.

[Radar WebKit Bug Importer]

<rdar://problem/15950123>

[Joseph Pecoraro]

Created attachment 225722 [details]
[PATCH] Proposed Fix

This is basically the patch. I have a couple FIXMEs still in the code that are questions I need to have answered before landing. But I want to get this up early so that I can sort out build issues in other bots and get review comments.

[WebKit Commit Bot]

Attachment 225722 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 225722 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/JSGlobalObjectConsole.cpp:42
> +JSGlobalObjectConsole::JSGlobalObjectConsole(InspectorConsoleAgent* consoleAgent)
> +    : ConsoleClient()
> +    , m_consoleAgent(consoleAgent)

So, a console points to a console client, and a client points to a console agent? Do we really need all this indirection?

> Source/JavaScriptCore/inspector/JSGlobalObjectConsole.cpp:48
> +    internalAddMessage(MessageType::Log, level, exec, arguments);

This is duplicated a lot with PageConsole. Can we make logWithLevel and related functions non-virtual, and make internalAddMessage virtual instead?

> Source/JavaScriptCore/inspector/JSGlobalObjectConsole.h:35
> +class JSGlobalObjectConsole final : public JSC::ConsoleClient {

I would call this JSConsoleClient.

> Source/JavaScriptCore/runtime/ConsolePrototype.cpp:142
> +    ConsoleClient* client = exec->lexicalGlobalObject()->consoleClient();
> +    if (!client)
> +        return JSValue::encode(jsUndefined());

castedThis->globalObject() is probably what you want, if you want to be able to "window.top.console.log".

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:466
> +    m_consolePrototype.set(vm, this, ConsolePrototype::create(vm, this, ConsolePrototype::createStructure(vm, this, m_objectPrototype.get())));

Let's not make this a data member. We only need it in this function.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:679
> +    visitor.append(&thisObject->m_consoleStructure); // FIXME: Needed?

Yes, needed.

> Source/WebCore/bindings/js/ScriptCachedFrameData.cpp:62
> +        // FIXME: Why does clearing this early prevent sub-window (window.top.console.log) from working?
> +        // e.g. LayoutTests/fast/events/suspend-timers.html
> +        // window->setConsoleClient(nullptr);

The reason window.top.console.log fails is that you've implemented JSConsole to use exec->lexicalGlobalObject, which means that, even if you specify "window.top", the global object whose client you end up trying to call through to is the global object the caller function was declared in. It sounds like you want some other behavior -- maybe to use the global object the console function was initially attached to.

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 225722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225722&action=review
> 
> > Source/JavaScriptCore/inspector/JSGlobalObjectConsole.cpp:42
> > +JSGlobalObjectConsole::JSGlobalObjectConsole(InspectorConsoleAgent* consoleAgent)
> > +    : ConsoleClient()
> > +    , m_consoleAgent(consoleAgent)
> 
> So, a console points to a console client, and a client points to a console agent? Do we really need all this indirection?

We could eventually make the InspectorConsoleAgent the ConsoleClient. But that is something that I wanted to try to get to later.


> > Source/JavaScriptCore/inspector/JSGlobalObjectConsole.cpp:48
> > +    internalAddMessage(MessageType::Log, level, exec, arguments);
> 
> This is duplicated a lot with PageConsole. Can we make logWithLevel and related functions non-virtual, and make internalAddMessage virtual instead?

Done, good suggestion.

> > Source/JavaScriptCore/inspector/JSGlobalObjectConsole.h:35
> > +class JSGlobalObjectConsole final : public JSC::ConsoleClient {
> 
> I would call this JSConsoleClient.

Done.

> > Source/JavaScriptCore/runtime/ConsolePrototype.cpp:142
> > +    ConsoleClient* client = exec->lexicalGlobalObject()->consoleClient();
> > +    if (!client)
> > +        return JSValue::encode(jsUndefined());
> 
> castedThis->globalObject() is probably what you want, if you want to be able to "window.top.console.log".

Yep, that fixes all my issues. Thanks for finding this!


> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:466
> > +    m_consolePrototype.set(vm, this, ConsolePrototype::create(vm, this, ConsolePrototype::createStructure(vm, this, m_objectPrototype.get())));
> 
> Let's not make this a data member. We only need it in this function.

Okay.


> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:679
> > +    visitor.append(&thisObject->m_consoleStructure); // FIXME: Needed?
> 
> Yes, needed.

I was hoping for a reason =). Does this makes sure to include the console object as reachable through the global object to prevent it from getting garbage collected? Or is visitChildren used for other purposes?

I also addressed Release build issues (requires Inline includes that Debug builds do not require).

[Joseph Pecoraro]

Created attachment 225900 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 225900 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ConsoleClient.h:70:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ConsoleClient.h:70:  The parameter name "level" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 5 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

> > > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:679
> > > +    visitor.append(&thisObject->m_consoleStructure); // FIXME: Needed?
> > 
> > Yes, needed.
> 
> I was hoping for a reason =). Does this makes sure to include the console object as reachable through the global object to prevent it from getting garbage collected? Or is visitChildren used for other purposes?

Yup! Any time you have a JavaScript object as a data member, you must visit it during GC. (That's part of why I wanted to remove the other data member -- either we needed to remove it, or we needed to visit it, and removing seemed simpler.)

[Geoffrey Garen]

Comment on attachment 225900 [details]
[PATCH] Proposed Fix

r=me

[Build Bot]

Comment on attachment 225900 [details]
[PATCH] Proposed Fix

Attachment 225900 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6518076224831488

New failing tests:
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html

[Build Bot]

Created attachment 225912 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 225900 [details]
[PATCH] Proposed Fix

Attachment 225900 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5095686940917760

New failing tests:
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html

[Build Bot]

Created attachment 225920 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 225900 [details]
[PATCH] Proposed Fix

Attachment 225900 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4694286545190912

New failing tests:
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html

[Build Bot]

Created attachment 225926 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Joseph Pecoraro]

Created attachment 225928 [details]
[PATCH] For Bots 1

[WebKit Commit Bot]

Attachment 225928 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 225937 [details]
[PATCH] For Bots 2

[Joseph Pecoraro]

Created attachment 225957 [details]
[PATCH] For Bots 3

[WebKit Commit Bot]

Attachment 225957 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 225970 [details]
[PATCH] For Bots 4

This should fix windows. Hopefully the last.

[WebKit Commit Bot]

Attachment 225970 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:32:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:33:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/ConsoleTypes.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/165199>

[Joseph Pecoraro]

Follow-up windows fix, which I had mistakenly forgot to include in the landed commit:
<http://trac.webkit.org/changeset/165202>