Bug 193149 – Web Inspector: Audit: create new IDL type for exposing special functionality in test context

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

In order to provide special functionality within Audit tests, we need a way to expose references to these functions without having them be visible when not running an Audit.

[Devin Rousso]

<rdar://problem/46801218>

[Devin Rousso]

Created attachment 358347 [details]
Patch

[Devin Rousso]

(In reply to Devin Rousso from comment #2)
> Created attachment 358347 [details]
> Patch
One thing I was thinking about was instead of only providing `AUDIT` when running within a Page, we could always provide `AUDIT` as an empty object and instead make each of the sub-objects have a separate IDL (e.g. `InspectorAuditDOMUtitlities`, `InspectorAuditAccessibilityUtilities`, etc), adding them to the `AUDIT` object as necessary (e.g. Page vs Worker vs JSContext).  I don't feel strongly either way, other than having more IDL this way.

Another idea I'd pondered with was to skip the whole IDL idea altogether and just manually create the `AUDIT` object and sub-objects, providing the functions as `JSNativeStdFunction`.  The downside of this approach is that we lose the "free" bits of the IDL generator (namely the type checking and conversions).

[Joseph Pecoraro]

Comment on attachment 358347 [details]
Patch

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

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:76
> +    String functionString = makeString("(function(", argumentsString, ") { \"use strict\"; return eval(", test, ")(); })");

Why commas around `test` in the eval?

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:98
> +    return { };

Should JSContexts even have an AuditAgent if we aren't going to expose an AUDIT object to it?

> Source/JavaScriptCore/inspector/protocol/Audit.json:4
> +    "domain": "Audit",
> +    "description": "",
> +    "commands": [

Does this make sense to expose to a "javascript", "worker", or "service-worker" context? I think it makes sense to restrict Audits to a "page" right now.

> Source/WebCore/inspector/InspectorController.cpp:152
> +    m_agents.append(std::make_unique<PageAuditAgent>(pageContext));

This should go under `createLazyAgents` since there is no need for it to be alive before a frontend connects.

> Source/WebCore/inspector/WorkerInspectorController.cpp:81
> +    m_agents.append(std::make_unique<WorkerAuditAgent>(workerContext));

Ditto: `createLazyAgents`

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:86
> +                    inspectorAuditUtilities = toJSNewlyCreated(execState, globalObject, InspectorAuditUtilities::create());

We could avoid creating an AUDIT object each time. For example, InjectedScriptHost is created once per target. (See calls to InjectedScriptHost::create).

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:55
> +        console.assert(this.AuditAgent);

We wouldn't have an AuditAgent in every target (an older iOS target for example).

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:62
> +        WI.auditManager.addEventListener(WI.AuditManager.Event.TestScheduled, this._handleAuditManagerTestScheduled, this);
> +        WI.auditManager.addEventListener(WI.AuditManager.Event.TestCompleted, this._handleAuditManagerTestCompleted, this);

This sounds worth doing separate from this patch, right?

[EWS Watchlist]

Comment on attachment 358347 [details]
Patch

Attachment 358347 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10631644

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl.html
inspector/model/stack-trace.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 358357 [details]
Archive of layout-test-results from ews103 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 358347 [details]
Patch

Attachment 358347 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10631740

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl.html
inspector/model/stack-trace.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 358359 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Comment on attachment 358347 [details]
Patch

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

>> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:76
>> +    String functionString = makeString("(function(", argumentsString, ") { \"use strict\"; return eval(", test, ")(); })");
> 
> Why commas around `test` in the eval?

This is adding the `test` variable to the `functionString`, not the literal string "test".  One thing to note, however, is that there should be quotes around `test`.

    String functionString = makeString("(function(", argumentsString, ") { \"use strict\"; return eval(\"", test, "\")(); })");

>> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:98
>> +    return { };
> 
> Should JSContexts even have an AuditAgent if we aren't going to expose an AUDIT object to it?

In the future there might be other things we'd want to make available via an Audit (e.g. promise resolution state) that could be useful when writing tests.

I don't have a good reason to disallow Audits in non-Page contexts, especially considering the fact that there's nothing really preventing that from happening.

[Devin Rousso]

Comment on attachment 358347 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:62
>> +        WI.auditManager.addEventListener(WI.AuditManager.Event.TestCompleted, this._handleAuditManagerTestCompleted, this);
> 
> This sounds worth doing separate from this patch, right?

<https://webkit.org/b/193158> Web Inspector: Audit: disable breakpoints when running Audit

[EWS Watchlist]

Comment on attachment 358347 [details]
Patch

Attachment 358347 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10632111

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/recording-2d.html
http/wpt/css/css-animations/start-animation-001.html
inspector/canvas/recording-webgl.html
inspector/model/stack-trace.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 358375 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 358380 [details]
Patch

[EWS Watchlist]

Comment on attachment 358380 [details]
Patch

Attachment 358380 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10634054

New failing tests:
inspector/canvas/recording-webgl-snapshots.html
inspector/model/stack-trace.html
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 358394 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 358380 [details]
Patch

Attachment 358380 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10634283

New failing tests:
inspector/canvas/recording-webgl-snapshots.html
inspector/model/stack-trace.html
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 358400 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Matt Baker]

Comment on attachment 358380 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:39
> +        Create a version of `evaluate` that accepts a list of arguemnts to be passed in as arguments

arguemnts -> arguments

[EWS Watchlist]

Comment on attachment 358380 [details]
Patch

Attachment 358380 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10634880

New failing tests:
inspector/canvas/recording-webgl-snapshots.html
inspector/model/stack-trace.html
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 358409 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 358422 [details]
Patch

[Devin Rousso]

Created attachment 358462 [details]
Patch

Rework the injected `AUDIT` object to be a plain JavaScript object with special IDL subobjects.  This way, we can always provide an `AUDIT` object, and can call the `AuditAgent` functions in such an order as to preserve the `AUDIT` object between tests, allowing for a simple way to share values between runs.

[Devin Rousso]

Created attachment 358467 [details]
Patch

[Devin Rousso]

Created attachment 358843 [details]
Patch

Add a few extra functions for use by later patches

[Devin Rousso]

Comment on attachment 358843 [details]
Patch

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

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:102
> +    functionString.appendLiteral(")`)(); })");

We should also add another "AUDIT" inside the `()` so that if the test function string defines `AUDIT` as the first parameter, it isn't overwritten.

[Devin Rousso]

Comment on attachment 358843 [details]
Patch

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

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:85
> +                    m_audit##name##Utilities = InspectorAudit##name##Utilities::create(); \

Since both `InspectorAuditAccessibilityUtilites` and `InspectorAuditDOMUtilities` will passed `*this` (in order to check whether a test is currently running, and to throw an error if not), we should make that stub implementation now so the future patches don't have to worry about it.

[Devin Rousso]

Created attachment 359007 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 359007 [details]
Patch

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

r- just because I'd like to see an updated version given all of the things I've pointed out below.

• The tests look good.
• I'm still not convinced that `AUDIT.Domain` is the best approach for an API but lets proceed with it for now. I'd have preferred something like something like "WebInspectorAudit" instead of "AUDIT".

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1248
> +		91278D5821DEDA9600B57184 /* JSGlobalObjectAuditAgent.h in Headers */ = {isa = PBXBuildFile; fileRef = 91278D5621DEDA9400B57184 /* JSGlobalObjectAuditAgent.h */; settings = {ATTRIBUTES = (Private, ); }; };

This fix should not be Private, it isn't and won't be needed outside of JavaScriptCore.

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:87
> +    for (auto& argument : runArguments(executionContextId))
> +        m_injectedAUDITValue->putDirect(vm, Identifier::fromString(execState, argument.first), argument.second);

I find the name `runArguments` to be very confusing. It sounds like an action but it isn't. How about something like `populateAuditObject`. And then you could just pass in the `m_injectedAUDITValue` instead of this weird and wasteful intermediate Vector.

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:105
> +    StringBuilder functionString;
> +    functionString.appendLiteral("(function(");
> +    if (m_injectedAUDITValue)
> +        functionString.appendLiteral("AUDIT");
> +    functionString.appendLiteral(") { \"use strict\"; return eval(`(");
> +    functionString.append(test.isolatedCopy().replace('`', "\\`"));
> +    functionString.appendLiteral(")`)(");
> +    if (m_injectedAUDITValue)
> +        functionString.appendLiteral("AUDIT");
> +    functionString.appendLiteral("); })");

It might be nice to include a comment before this to show what it is doing in a less confusing way:

    // Perform the following:
    //
    //     (function(AUDIT) {
    //         "use strict";
    //         return eval(`(<escaped-test>)`(AUDIT);
    //     })

Is there any reason to not include "AUDIT" always? It would just be `undefined` if not provided below. I'd drop the `m_injectedAUDITValue` branches up here.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectAuditAgent.h:36
> +class JSGlobalObjectAuditAgent final : public InspectorAuditAgent {

I still think we should remove Audits from being available in JSContexts and Workers. Maybe it would work with a ESLint style audit, but in general I think it is not particularly useful there and would just confuse JSContext inspector users. That would eliminate quite a bit of this new code in JavaScriptCore. Note we should update the frontend to also not allow the Audits tab for JSContexts.

> Source/JavaScriptCore/inspector/protocol/Audit.json:22
> +            "name": "run",
> +            "description": "Parses and evaluates the given test string and sends back the result.",
> +            "parameters": [
> +                { "name": "test", "type": "string", "description": "Test string to parse and evaluate." },
> +                { "name": "contextId", "$ref": "Runtime.ExecutionContextId", "optional": true, "description": "Specifies in which isolated context to run the test. Each content script lives in an isolated context and this parameter may be used to specify one of those contexts. If the parameter is omitted or 0 the evaluation will be performed in the context of the inspected page." }
> +            ],
> +            "returns": [
> +                { "name": "result", "$ref": "Runtime.RemoteObject", "description": "Evaluation result." },
> +                { "name": "wasThrown", "type": "boolean", "optional": true, "description": "True if the result was thrown during the evaluation." }
> +            ]

It may be important to point out that this puts results into the "audit" object group. Which the frontend would need to release in order to free up memory from audits (or navigate the page) and thereby clear the entire injected script.

> Source/JavaScriptCore/inspector/protocol/Audit.json:26
> +            "description": ""

This could use a description. It seems like this may be necessary to balance `setup` sometimes.

> Source/WebCore/inspector/InspectorAuditAccessibilityUtilities.h:42
> +    explicit InspectorAuditAccessibilityUtilities();

Can you get away with `= default;` and drop the cpp file?

> Source/WebCore/inspector/WorkerInspectorController.cpp:181
> +    m_agents.append(std::make_unique<WorkerAuditAgent>(workerContext));

Ditto RE not including an Audit Agent in Workers unless there is a clear use case.

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:66
> +        if (executionContextId)
> +            errorString = "Internal error: main world execution context not found."_s;

This should be `if (!executionContextId)` if the error is about the main world execution context.

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:87
> +#define ADD_AUDIT_UTILITIES(name) \
> +                if (!m_audit##name##Utilities) \
> +                    m_audit##name##Utilities = InspectorAudit##name##Utilities::create(*this); \
> +                if (JSC::JSValue inspectorAudit##name##Utilities = toJS(execState, globalObject, *m_audit##name##Utilities)) \
> +                    arguments.append(std::make_pair("" #name ""_s, WTFMove(inspectorAudit##name##Utilities)));

I'd prefer an "Object" suffix to a "Utilities" suffix. Meaning the name "InspectorAudit<Foo>Object" instead of "InspectorAudit<Foo>Utilities".

We already have this pattern already in JavaScriptCore for the C++ backend to some JavaScript "Object":

    ./JavaScriptCore/runtime/SymbolObject.h
    ./JavaScriptCore/runtime/IntlObject.h
    ./JavaScriptCore/runtime/InspectorInstrumentationObject.h
    ./JavaScriptCore/runtime/ReflectObject.h
    ./JavaScriptCore/runtime/BigIntObject.h
    ./JavaScriptCore/runtime/ConsoleObject.h
    ./JavaScriptCore/runtime/StringObject.h
    ./JavaScriptCore/runtime/JSONObject.h
    ./JavaScriptCore/runtime/MathObject.h
    ...

In contrast to "Utilities" files which are often collections of functions that may be used to work with something:

    ./WebCore/page/Base64Utilities.h
    ./WebCore/platform/win/GDIUtilities.h
    ./WebCore/platform/network/mac/UTIUtilities.h
    ./WebCore/platform/animation/AnimationUtilities.h
    ./WebCore/platform/ios/wak/WKUtilities.h
    ./WebCore/platform/mac/StringUtilities.h
    ...

This falls more cleanly into the "Object" suffix bucket.

> Source/WebCore/inspector/agents/page/PageAuditAgent.cpp:111
> +void PageAuditAgent::muteConsole()
> +{
> +    InspectorAuditAgent::muteConsole();
> +
> +    PageConsoleClient::mute();
> +}
> +
> +void PageAuditAgent::unmuteConsole()
> +{
> +    PageConsoleClient::unmute();
> +
> +    InspectorAuditAgent::unmuteConsole();
> +}

Style: I'd remove the inner newlines and make these as compact as possible.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:239
> +            agentCommandArguments.objectGroup = "audit";

Do we ever clear the `audit` object group anywhere? Does the frontend clear/trash results anywhere?

[Devin Rousso]

Comment on attachment 359007 [details]
Patch

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

> • I'm still not convinced that `AUDIT.Domain` is the best approach for an API but lets proceed with it for now. I'd have preferred something like something like "WebInspectorAudit" instead of "AUDIT".
I like `WebInspectorAudit` over `AUDIT`.  It seems bit more "normal", in that we rarely use all-caps except for constants.  It's also a bit more "specific" than something general like "audit".

>> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:105
>> +    functionString.appendLiteral("); })");
> 
> It might be nice to include a comment before this to show what it is doing in a less confusing way:
> 
>     // Perform the following:
>     //
>     //     (function(AUDIT) {
>     //         "use strict";
>     //         return eval(`(<escaped-test>)`(AUDIT);
>     //     })
> 
> Is there any reason to not include "AUDIT" always? It would just be `undefined` if not provided below. I'd drop the `m_injectedAUDITValue` branches up here.

Good point.  There's no real reason to not always supply it.

>> Source/JavaScriptCore/inspector/protocol/Audit.json:26
>> +            "description": ""
> 
> This could use a description. It seems like this may be necessary to balance `setup` sometimes.

Ah whoops 😅

>> Source/WebCore/inspector/InspectorAuditAccessibilityUtilities.h:42
>> +    explicit InspectorAuditAccessibilityUtilities();
> 
> Can you get away with `= default;` and drop the cpp file?

The cpp file is used by all of the blocked patches, so rather than have them worry about it, I felt that it made sense to include it in the base patch (this way I wouldn't have to worry about what patches landed in what order).

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:239
>> +            agentCommandArguments.objectGroup = "audit";
> 
> Do we ever clear the `audit` object group anywhere? Does the frontend clear/trash results anywhere?

Ah, good point.  I think it's safe to clear them before each time we call `Audit.run`.

[Devin Rousso]

Created attachment 359118 [details]
Patch

[Devin Rousso]

Comment on attachment 359007 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:239
>>> +            agentCommandArguments.objectGroup = "audit";
>> 
>> Do we ever clear the `audit` object group anywhere? Does the frontend clear/trash results anywhere?
> 
> Ah, good point.  I think it's safe to clear them before each time we call `Audit.run`.

Actually, since we have the "Results" folder, we shouldn't ever clear the object group, as we could potentially show a value at any point in the future.  Clearing this will happen on page navigation (unless we add a button somewhere).

[Joseph Pecoraro]

Comment on attachment 359118 [details]
Patch

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

r=me! Very nice

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:102
> +    options.includeCommandLineAPI = true;

Should we be including the command line API?! That means they would have `getEventListeners` and we wouldn't need our (future) DOM additions. I feel like we shouldn't include the command line API.

[Devin Rousso]

Comment on attachment 359118 [details]
Patch

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

>> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:102
>> +    options.includeCommandLineAPI = true;
> 
> Should we be including the command line API?! That means they would have `getEventListeners` and we wouldn't need our (future) DOM additions. I feel like we shouldn't include the command line API.

Oh!  I was trying to allow for `console.*` functions.  My mistake.

[Devin Rousso]

Created attachment 359120 [details]
Patch

[EWS Watchlist]

Comment on attachment 359120 [details]
Patch

Attachment 359120 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10753530

New failing tests:
inspector/audit/run.html

[EWS Watchlist]

Created attachment 359124 [details]
Archive of layout-test-results from ews103 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 359120 [details]
Patch

Attachment 359120 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10753677

New failing tests:
inspector/audit/run.html

[EWS Watchlist]

Created attachment 359132 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 359139 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 359139 [details]
Patch

Clearing flags on attachment: 359139

Committed r239976: <https://trac.webkit.org/changeset/239976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It appears that the changes in https://trac.webkit.org/changeset/239976/webkit

has caused inspector/model/remote-object.html to become a constant failure. 

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object.html

Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/model/remote-object-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/model/remote-object-actual.txt
@@ -1729,7 +1729,7 @@
       {
         "_name": "stack",
         "_type": "string",
-        "_value": "splitText@[native code]\nglobal code\nevaluateWithSc…ension@[native code]\n\n_wrapCall"
+        "_value": "splitText@[native code]\nglobal code\nevaluateWithScopeExtension@[native code]\n\n_wrapCall"
       },
       {
         "_name": "code",


I confirmed this issue by reverting this revision locally and running the test. After the revert the test started passing again.

[Truitt Savell]

This looks like it could just be a rebaseline, could you confirm?

[Devin Rousso]

(In reply to Truitt Savell from comment #43)
> This looks like it could just be a rebaseline, could you confirm?
<https://trac.webkit.org/changeset/239990/webkit>