brendan-duncan / webgpu_inspector

Inspection debugger for WebGPU
MIT License
142 stars 3 forks source link

Auto-inject into workers #10

Closed csnewman closed 1 month ago

csnewman commented 1 month ago

https://github.com/brendan-duncan/webgpu_inspector/issues/8

Instead of requring the user to insert code into their worker, this MR tries to insert itself into the worker by hooking the worker creation, and injecting the inspector before the users code runs.

The main change required to enable this is storing the injector source code into a __webgpu_src variable, which can then be used to re-inject the inspector.

A benefit of this approach, is that it works on any website, and also means the extension version and website inspector version no longer need to be kept in sync.

The diff looks worse than it is, due to the rename of wgpu_inspector to wgpu_inspector_core.

This is untested on firefox and some features don't fully work, like capture. Additionally, I've noticed that the sessionstorage approach used to determine whether to inject the inspector does not work with iframes.

Creating this PR now to get early feedback.

csnewman commented 1 month ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1736575#c10

Until this Firefox feature lands, the world: "MAIN" feature this relies upon to be able to be able to reliably incept the Worker creation won't work in Firefox.

So I think the <script> injection technique will need to stick around for just Firefox.

brendan-duncan commented 1 month ago

Thanks! That would be awesome to be able to automatically inject into workers.

I'll need to sit with this for a while as I get some more time, figure out what it's doing. Big external changes like this are hard because I'll need to maintain it, and it's kinda hard to sort it out from the diff. Do you know if WebKit supports "MAIN"?

brendan-duncan commented 1 month ago

I'm learning a few things from your PR. Proxy is new to me, that's a nice one to know about :-). And world: MAIN is new. All I know about Chrome extensions, I've learned from trying to make this project. I'll need to learn more about these.

I was thinking about Blob URLs for injecting the script, but couldn't think of how I could reconcile that with the original worker script. Expanding the inspector code as a string and importing the original worker script is a nice approach.

Eval is always worrisome, I've had troubles with using it and security policies. Is that what the MAIN world is for?

I'll have to download your fork and play around with it. I'm curious how it will behave with non-graphics workers.

This is looking like a promising solution, thank you for putting it together!

brendan-duncan commented 1 month ago

The first thing I noticed trying it, besides it working with the worker which is awesome, is that putting webgpu_inspector_core into a string kills the debuggability of it.

We need to find a way that at the very least the webgpu_inspector.js can import the source version of webgpu_inspector_core.js rather than evaluating the string. It's critical to be able to debug it.

brendan-duncan commented 1 month ago

My attempt to fix the debugability issue kinda works. The source file shows up in the debugger, but setting breakpoints in the source mapped file doesn't. I haven't figured out why yet, maybe a rollup thing?

In webgpu_inspector_core.js, I:

  1. Removed the local scoping (() => { and })(); lines.
  2. Added an export to WebGPUInspector class.
  3. I made the creation of the implicit WebGPUInspector() conditional to only happen with Workers with if (!_window) { webgpuInspector = new WebGPUInspector(); }.

In webgpu_inspector.js, I:

  1. imported "./webgpu_inspector_core.js" instead of the string version.
  2. Removed the eval and replaced it with new WebGPUInspector();

That way, the main thread gets the source version of webgpu_inspector_core, and the worker thread gets the string version. Now I can get to the webgpu_inspector_core.js code from the debugger, at least for non-worker pages.

brendan-duncan commented 1 month ago

Debugging seems to work better now. The rollup terser seems to throw off the source maps in some areas, so I comment out the terser plugin in rollup.config.js when I want to debug. That's not too big of a deal, I was already doing that before.

To avoid creating forks of forks to share ideas on this, here is a patch of my changes

keep_inspector_core_debuggable.patch

csnewman commented 1 month ago

I've been thinking that it would be nice if instead of the iife, if we output the core as:

var inspectorLoader = function() {
   // all the source
};

as then we'd be able to use inspectorLoader.toString() to get a copy of the source at runtime. However, I'm not aware of any pre-existing plugins to help with this. An alternative would be to use chrome.runtime.getURL in some way, however synchroising "main" world and the other content script seems difficult.

csnewman commented 1 month ago

Is that what the MAIN world is for?

The MAIN world means that the script will run inside the pages content. This means that it will always be executed before any of the pages scripts have been executed.

csnewman commented 1 month ago

imported "./webgpu_inspector_core.js" instead of the string version.

One issue with this approach is that it's causing the Worker injection to run on all websites, regardless of whether the inspector is active. (this therefore is breaking other sites, which may be trying to detect browser modifications like Captchas).

Another issue is that the worker injection breaks cross origin workers, so we'll need to check if the domain doesn't match and prevent injection in those cases.

brendan-duncan commented 1 month ago

imported "./webgpu_inspector_core.js" instead of the string version.

One issue with this approach is that it's causing the Worker injection to run on all websites, regardless of whether the inspector is active. (this therefore is breaking other sites, which may be trying to detect browser modifications like Captchas).

If the Worker proxy setup was done in the WebGPUInspector constructor then it should only run when the inspector is turned on...

Another issue is that the worker injection breaks cross origin workers, so we'll need to check if the domain doesn't match and prevent injection in those cases.

That would be good to do.

csnewman commented 1 month ago

I've managed to rework the loading to wrap the "core inspector" library as a function, which can then be loaded inline and also passed into the worker. Now that the original source is preserved, I've configured sourcemaps, which appear to work in chrome. (I can set breakpoints on src/webgpu_inspector_core.js and they appear to work).

Could you test if the debugger works as expected for you?

brendan-duncan commented 1 month ago

The latest commit is looking good. Getting a bit convoluted with the rollup string expansions, but I'll probably get used to it. How does source-map SourceNode work, where you specify the column 36. What is that hard-coded column based on?

csnewman commented 1 month ago

How does source-map SourceNode work, where you specify the column 36.

Mostly a lot of trial and error was involved in the process, but https://github.com/mozilla/source-map?tab=readme-ov-file#sourcenode describes the overall API. In this case, 36 represents that the }; token appears at column 36 in the source file. It's all a bit nasty how there's a virtual file with the source export default function() { ${code} };, but parts are duplicated to define the source mapping.

The SourceNode API is in charge of generating the final concated string, but in the process, it maintains the source mappings.

brendan-duncan commented 1 month ago

The source-map thing is new to me, and I'm wanting to understand it in case something goes wrong and I have to fix something.

Doing this seemed to also work, without the 36, but I still am not fully understanding SourceNode and the following setSourcContent. I'm guessing SourceNode will automatically convert strings to SourceNode, and the column positions weren't really even necessary, but I could be missing something.

const newSrc = new SourceNode(1, 1, "webgpu_inspector_core_func", [
     "export default function() { ",
     originalSrc,
     " };"
]);

newSrc.setSourceContent("webgpu_inspector_core_func", "export default function() { ${code} };");

Not calling setSourceContent also seems to work, so is that necessary?

const newSrc = new SourceNode(1, 1, "webgpu_inspector_core_func", [
     "export default function() { ",
     originalSrc,
     " };"
]);
const generated = newSrc.toStringWithSourceMap({ file: "webgpu_inspector_core_func" });
csnewman commented 1 month ago

Doing this seemed to also work, without the 36, but I still am not fully understanding SourceNode and the following setSourcContent. I'm guessing SourceNode will automatically convert strings to SourceNode, and the column positions weren't really even necessary, but I could be missing something.

var node = new SourceNode(1, 2, "a.js", [
  new SourceNode(3, 4, "b.js", "one"),
  "two",
  ["three", new SourceNode(5, 6, "c.js", "four")],
  "other",
]);

node.walk(function (code, loc) {
  console.log("WALK:", code, loc);
});

Outputs:

WALK: one { source: 'b.js', line: 3, column: 4, name: null }
WALK: two { source: 'a.js', line: 1, column: 2, name: null }
WALK: three { source: 'a.js', line: 1, column: 2, name: null }
WALK: four { source: 'c.js', line: 5, column: 6, name: null }
WALK: other { source: 'a.js', line: 1, column: 2, name: null }

So it looks like it will just inherit the parent's source position, so " };" would be from (1,1) in the virtual file, which it isn't (its from (1,38)). In reality, I don't think it matters, but I'm not familiar enough with sourcemaps to say what different processors will do if its wrong.

Not calling setSourceContent also seems to work, so is that necessary?

It does appear to work without it, however if you look at the sources tab in your browser, webgpu_inspector_core_func won't have any source available. Once again, I don't think this matters in practice, but is nice for consistency.

brendan-duncan commented 1 month ago

Ok, sold. If you think this won't mess up random sites that use workers for non-webgpu purposes, we can move forward with this. I'll get used to this whole stringify everything and SourceNode thing.

It is exciting to get auto injection of render workers. I have some render bundle work I was going to start on, and don't want to start pushing more to the main branch until this refactor is in, it would be a pain to do merges for any conflicts.

I'll have a bit less time starting tomorrow, when I have to get back to my day job work. Which happens to also be WebGPU, and why I wrote this thing to begin with. Debugging Unity Wasm WebGPU code was a real pain and I was jealous the other API developers got to use RenderDoc. Now I kinda prefer this tool over when I have to debug Vulkan with RenderDoc :-). When I have something wrong with my WebGPU driver, I'll make a Vulkan build of a game, and a WebGPU build, capture a frame of the vulkan build with RenderDoc and a frame of the WebGPU build with this tool. Then I can go over the render command by command, looking at buffer data, and find where I got things wrong. It's let me figure out a number of hard bugs.

csnewman commented 1 month ago

If you think this won't mess up random sites that use workers for non-webgpu purposes

As far as I'm aware, this approach shouldn't cause issues as the injection logic isn't loaded unless the inspector is "started" for the current page. It's possible it will cause issues on random pages, but that's to be expected when running.

Debugging Unity Wasm WebGPU code was a real pain Sounds interesting! Are you using web workers here? I needed worker support as in the .NET wasm multi-threading support, you can't easily do synchronous JS api calls on the UI thrad, so you have to use a worker thread.

csnewman commented 1 month ago

I've fixed loading on Firefox by falling back to the <script> tag approach for now. FF doesn't support WebGPU on workers atm.

I can't seem to get safari to work, but don't know enough about it to debug it further.

brendan-duncan commented 1 month ago

FF still has a ways to go with WebGPU. It won't run Unity webgpu builds at all because they haven't implemented the wasm heap variants of writeBuffer and functions like that. Hopefully they'll get there sometime in the near future, but I'm not too worried about supporting them yet.

Safari is only working on Safari Tech Preview, and they're a lot farther along than FF but still have a bit to go before passing the spec conformance suite. I'm not worried about whether the extension works there or not either, particularly for workers. Chrome is the biggest audience with the most complete implementation so far, so that is the primary focus.

Whenever you're ready, go ahead and flip the draft switch and I'll look into doing the merge.

Thank you again for taking on the worker project, it is very helpful!

csnewman commented 1 month ago

I think for an initial version, this is probably a good stopping point. Recorder support is still missing and there's likely to be some bugs, but we may as well see what feedback there is from users.

brendan-duncan commented 1 month ago

Fantastic, thank you! I'll get to this when I get a chance in a little bit.

brendan-duncan commented 1 month ago

Merged. Anything broken can be fixed. I'll just do a bunch of testing before the next chrome strore update.

csnewman commented 1 month ago

Thanks for merging!

brendan-duncan commented 1 month ago

I found a page where the worker inspection breaks the page, https://2024.utsubo.com, a threejs project. I'll open a ticket for it and see if I can sort it out.

brendan-duncan commented 1 month ago

I pushed auto-injecting WebGPURecorder into workers using the same approach you came up with. Seems to work from my testing. It's been merged into main. Will require an npm update to pull the latest webgpu_recorder.