Bug 195847 – Web Inspector: provide a way to inject "bootstrap" JavaScript into the page as the first script executed

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This would be really when trying to do various debugging tasks (e.g. swizzling prototype functions for versions that call `debugger` to see where things get called), as well as a place to save/persist useful snippets of code that get pasted/run often in the console (e.g. a `removeAllEventListeners` function that uses the `getEventListeners` console API).

[Radar WebKit Bug Importer]

<rdar://problem/48950551>

[Devin Rousso]

Created attachment 372249 [details]
[Patch] WIP

MVP/proof-of-concept.  The "real" approach should be to subclass `Script` to make a `LocalScript` or modify `LocalResource` to allow for less metadata (e.g. no request/response info).

[Devin Rousso]

Comment on attachment 372249 [details]
[Patch] WIP

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

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:47
> +        this._bootstrapScriptSetting = new WI.Setting("bootstrap-script", null);

This should probably default to `""` instead.

[Joseph Pecoraro]

Comment on attachment 372249 [details]
[Patch] WIP

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

I can't wait to see this land! Next patch lets see some screenshots.

> Source/JavaScriptCore/inspector/protocol/Page.json:254
> +            "name": "setBootstrapScript",

I still don't particularly like the name `Bootstrap`. I still think "Injected Script" tends closer to reality. And this could support multiple "Inspector Injected Scripts" though then order becomes relevant. Bootstrap doesn't sound debuggable, because technically it sounds like it would be before something is completely initialized / setup.

Some other naming thoughts:

    incipient - "in an initial stage; beginning to happen or develop"
    inceptive - "relating to or marking the beginning of something;"

That said, I love how simple this is on the backend and "bootstrap" is a convenient moniker for the feature, so gives a nice name.

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:904
> +    frame.script().evaluate(ScriptSourceCode(m_bootstrapScript.value(), URL(), TextPosition(), JSC::SourceProviderSourceType::Module));

If this does not have a URL / name it doesn't seem like the user can set breakpoints in the script.

Users could use `debugger` statements, but I think we should allow breakpoints. For example if you put functions inside your bootstrap script and want to pause in them.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:665
> +    set bootstrapScript(source)
> +    {
> +        console.assert(NetworkManager.supportsBootstrapScript());
> +        console.assert(!source || typeof source === "string");
> +
> +        this._bootstrapScriptSetting.value = source;
> +
> +        for (let target of WI.targets) {
> +            if (target.PageAgent && target.PageAgent.setBootstrapScript)
> +                target.PageAgent.setBootstrapScript(this._bootstrapScriptSetting.value || undefined);
> +        }
> +    }

It seems weird to see this on NetworkManager, but that seems fine. It raises a few interesting points:

    • Should this be injected into the Main Frame or All Frames?
        - I think I'd vote All Frames, like Injected Scripts. It looks like this does that right now.
    • Should this be injected into Workers / Service Worker contexts?
        - They will have a Network Agent, and a JS environment, but they won't have frames so won't work right now with the backend.
        - I think I'd vote No right now because it seems likely someone will put DOM code in their bootstrap script and not think about Workers.
        - We should probably rely on "Local Resource Substitution" enhancement ideas if we ever wanted to inject code into Workers.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1677
> +            contextMenu.appendItem(WI.UIString("Inspector Style Sheet"), () => {

It seems we are inconsistent on wording:

    WI.UIString("Author Stylesheet");
    WI.UIString("User Stylesheet");
    WI.UIString("User Agent Stylesheet");
    WI.UIString("User Agent Stylesheet");
    WI.UIString("Inspector Style Sheet")

I think we should standardize on "Stylesheet".

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:218
> +        if (!this._textEditor.string && this._textEditor.readOnly)

Hah! That must have been tricky to discover.

[Devin Rousso]

Comment on attachment 372249 [details]
[Patch] WIP

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

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1677
>> +            contextMenu.appendItem(WI.UIString("Inspector Style Sheet"), () => {
> 
> It seems we are inconsistent on wording:
> 
>     WI.UIString("Author Stylesheet");
>     WI.UIString("User Stylesheet");
>     WI.UIString("User Agent Stylesheet");
>     WI.UIString("User Agent Stylesheet");
>     WI.UIString("Inspector Style Sheet")
> 
> I think we should standardize on "Stylesheet".

I think we should actually convert all the others to use "Style Sheet" instead, as that seems to be what the spec uses: <https://www.w3.org/TR/html401/present/styles.html>

[Devin Rousso]

Created attachment 381401 [details]
Patch

[Devin Rousso]

Created attachment 381402 [details]
[Image] After Patch is applied

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Comment on attachment 381402 [details]
[Image] After Patch is applied

I decided not to put the "Inspector Bootstrap Script" tree item in the "Local Overrides" section, as "Local Resources" (which is what I would've renamed it) doesn't have the same meaning as "Local Overrides".  The current patch has it so that the "Inspector Bootstrap Script" is always the first item in the resource tree, which I think also has a nice implication in that it's the "first" thing that's part of the page, even before the main resource, which neatly aligns with what it actually does (e.g. first script evaluated).

[Devin Rousso]

Created attachment 381415 [details]
Patch

[Devin Rousso]

Created attachment 381442 [details]
Patch

Oops.  Missed a file.

[Devin Rousso]

Created attachment 381451 [details]
Patch

Rebase

[Timothy Hatcher]

Comment on attachment 381451 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:15
> +        This change adds support for that concept, which has been named Inspector Bootstrap Script.

Maybe "Inspector Injected Script" or just "Injected Script"? Maybe rename Inspector Style Sheet to "Injected Style Sheet" to match. Using Bootstrap internally is fine. But if Chrome uses Bootstrap in their UI, we might need to match…

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:588
> +    if (!optionalSource || optionalSource->isEmpty()) {
> +        m_bootstrapScript = WTF::nullopt;

String can be a null or empty string. Optional isn't really needed. You can do m_bootstrapScript = nullString() and use m_bootstrapScript.isNull() elsewhere.

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:779
> +    frame.script().evaluate(ScriptSourceCode(m_bootstrapScript.value(), URL(URL(), "web-inspector://Bootstrap.js"_s)));

injected.js if you rename the UI. Or at least lowercase bootstrap.js.

> Source/WebCore/inspector/agents/page/PageRuntimeAgent.h:63
> -    void didCreateMainWorldContext(Frame&);
> +    void didClearWindowObjectInWorld(Frame&);

You don't pass in the world as a param, so this should be didClearWindowObjectInMainWorld.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1599
> +    return url === "web-inspector://Bootstrap.js";

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:351
> +            const sourceURL = "web-inspector://Bootstrap.js";

This likely should be a global somewhere, since it is in multiple places.

[Devin Rousso]

Comment on attachment 381451 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:15
>> +        This change adds support for that concept, which has been named Inspector Bootstrap Script.
> 
> Maybe "Inspector Injected Script" or just "Injected Script"? Maybe rename Inspector Style Sheet to "Injected Style Sheet" to match. Using Bootstrap internally is fine. But if Chrome uses Bootstrap in their UI, we might need to match…

As discussed offline, we definitely want to have "Inspector" in the name somewhere so it's as clear as possible where the script is coming from.  I don't think we should use "Injected" because that has other meanings and also doesn't really relate to the feature (yes it's how we evaluate the bootstrap script, but that's an implementation detail).  I think that bootstrap is a well enough known name (or is one that we can clarify via blog posts) for the idea of "initial creation phase", like bootstrapping or the boot phase.

>> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:588
>> +        m_bootstrapScript = WTF::nullopt;
> 
> String can be a null or empty string. Optional isn't really needed. You can do m_bootstrapScript = nullString() and use m_bootstrapScript.isNull() elsewhere.

🤦‍♂️ duh lol

>> Source/WebCore/inspector/agents/page/PageRuntimeAgent.h:63
>> +    void didClearWindowObjectInWorld(Frame&);
> 
> You don't pass in the world as a param, so this should be didClearWindowObjectInMainWorld.

I'd personally rather have the function name match the `InspectorInstrumentation` version, as it makes following the code _much_ easier.  This way, I can do a single search for `didClearWindowObjectInWorld` and see the full path from the `InspectorInstrumentation` call elsewhere in WebCore to the various agents that listen for it.

[Devin Rousso]

Created attachment 381627 [details]
Patch

Move the Inspector Bootstrap Script to the Local Overrides section and allow it to be disabled without being removed.  Having it in the Local Overrides section makes more sense IMO because one of the expected use cases of a bootstrap script is to be able to override default prototype functions or other functionality, so in a sense it is an override.

[Devin Rousso]

Created attachment 381628 [details]
[Image] After Patch is applied (creation context menu)

[Devin Rousso]

Created attachment 381631 [details]
Patch

[EWS Watchlist]

Comment on attachment 381631 [details]
Patch

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

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl

[Devin Rousso]

Created attachment 381660 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 381660 [details]
Patch

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

What does the Web Inspector UI look like if you pause in a bootstrap code?

---
window.doPause = function() {
    console.log("Inside bootstrap");
    debugger;
}
-----

Log line and pausing in the debugger would be interesting to see. Maybe the name is very long in the console and might be annoying (Console Evaluation is sometimes annoying).

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:774
> +    frame.script().evaluate(ScriptSourceCode(m_bootstrapScript, URL(URL(), "web-inspector://bootstrap.js"_s)));

Style: I've been seeing styles like the following more and more:

    URL { { }, "web-inspector://bootstrap.js"_s }
    URL { URL(), "web-inspector://bootstrap.js"_s }

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:81
> +            this._bootstrapScriptSourceSetting = new WI.Setting("bootstrap-script-source", "");

I wonder if this should be in an IndexedDB (like LocalResourceOverrides) instead of a LocalStorage setting. I could see someone dumping large libraries, such as Underscore.js or something, into the bootstrap so it is always available. In which case this fills the local storage quota rather quickly.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:281
> +    toggleBootstrapScript(enabled)

Hmm, I don't particularly like this. I'd rather there just be a setting `set bootstrapScriptEnabled` and the other side set it appropriately.

> Source/WebInspectorUI/UserInterface/Models/Script.js:131
> +            console.assert(WI.NetworkManager.supportsBootstrapScript());

The assertions in these display functions don't seem accurate. Someone could always make a web-inspector:// request or sourceURL name. I think you could just drop the assertions or only use the name if bootstrap script is enabled.

Actually, should we even consider _sourceURL here, or just _url?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:2182
> -        if (!this.target.hasCommand("Runtime.getRuntimeTypesForVariablesAtOffsets"))
> +        if (!this.target || !this.target.hasCommand("Runtime.getRuntimeTypesForVariablesAtOffsets"))

Hmm, makes me wonder if there are other references to a Script/Resource having a `target`.

Correct me if I'm wrong, these should be unreachable right now since you've disabled the buttons for ScriptContentView? Still okay to do.

[Devin Rousso]

Comment on attachment 381660 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:81
>> +            this._bootstrapScriptSourceSetting = new WI.Setting("bootstrap-script-source", "");
> 
> I wonder if this should be in an IndexedDB (like LocalResourceOverrides) instead of a LocalStorage setting. I could see someone dumping large libraries, such as Underscore.js or something, into the bootstrap so it is always available. In which case this fills the local storage quota rather quickly.

Good point.  Perhaps we should add a general `WI.ObjectStore` for random things, almost like a replacement for `LocalStorage`.

>> Source/WebInspectorUI/UserInterface/Models/Script.js:131
>> +            console.assert(WI.NetworkManager.supportsBootstrapScript());
> 
> The assertions in these display functions don't seem accurate. Someone could always make a web-inspector:// request or sourceURL name. I think you could just drop the assertions or only use the name if bootstrap script is enabled.
> 
> Actually, should we even consider _sourceURL here, or just _url?

That same issue is true for any of our other "special" URLs, like `isWebKitInternalScript`.
```
    document.write(`
        <script>
            //# sourceURL=__WebInspectorInternal__
            window.foo = 42;
        </script>
    `)
```

I also think it's extremely unlikely for someone to actually do as you're suggesting, which is why it's just an assert instead of a full-fledged early-return.

We also want to consider `_url` because that's what ends up being set for the eventual evaluations of the bootstrap script (e.g. the anonymous script).

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:2182
>> +        if (!this.target || !this.target.hasCommand("Runtime.getRuntimeTypesForVariablesAtOffsets"))
> 
> Hmm, makes me wonder if there are other references to a Script/Resource having a `target`.
> 
> Correct me if I'm wrong, these should be unreachable right now since you've disabled the buttons for ScriptContentView? Still okay to do.

Yeah, these should both be unreachable (at least via the buttons).

[Devin Rousso]

Created attachment 381766 [details]
Patch

[Devin Rousso]

Created attachment 381767 [details]
[Image] After Patch is applied (paused in evaluated bootstrap script)

[WebKit Commit Bot]

Comment on attachment 381766 [details]
Patch

Clearing flags on attachment: 381766

Committed r251531: <https://trac.webkit.org/changeset/251531>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.