Fix console.log(globalThis) crash #3175

[EngoDev]

The bug in #3175 is caused by the fact that the function getGPU in global_scope.c++ is set to only work in a DurableObject, and that is not the case in local development.

The fix is simple: Instead of setting the wgpu flag to be always on, enable it only in CI.
As long as wgpu is meant to be only accessible in DureableObjects, this fix should suffice.

I hope I didn't miss anything, and I'm happy to iterate on it otherwise.

Output of the Bazel run all tests (dbg) task:

Output of bazel test --config=cli -c dbg --cache_test_results=no //...:

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[EngoDev]

I have read the CLA Document and I hereby sign the CLA

[fhanau]

Not enabling webGPU in local development would resolve the error message, but just not building a feature is never just a simple bug fix. If we disable webGPU, it would need to be a deliberate decision and communicated properly.
CC @anonrig can we think of a more narrow workaround?

[anonrig]

I think the correct approach is basically not throw an error when webgpu attribute is accessed and just return undefined.

[EngoDev]

Not enabling webGPU in local development would resolve the error message, but just not building a feature is never just a simple bug fix. If we disable webGPU, it would need to be a deliberate decision and communicated properly. CC @anonrig can we think of a more narrow workaround?

My thought process with this approach was that because it's already implicit in the code that you can't run any wgpu code outside of a DurableObject, so disabling this feature for local development doesn't practically take any functionality away from it but I understand what you are saying, and It makes sense.

This felt like a trivial change, so I filled a PR for it per the CONTRIBUTING.md file; I didn't mean to overstep.

I think the correct approach is basically not throw an error when webgpu attribute is accessed and just return undefined.

Following your suggestion, I changed getGPU to return an Optional but I'm not sure about the logging for the function, I think it would be helpful to keep the messages as warning logs, should I use LOG_WARNING_PERIODICALLY or LOG_WARNING_ONCE?

[anonrig]

I think we should not log a warning. Anybody can console.log, in any part of the project (including dependencies). We should treat this case as "webgpu is not implemented".

[EngoDev]

I think we should not log a warning. Anybody can console.log, in any part of the project (including dependencies). We should treat this case as "webgpu is not implemented".

That's a good point, I removed the logs.

[anonrig]

Can you also add a test case where we call console.log?

[EngoDev]

Can you also add a test case where we call console.log?

Yes, of course. I added a test to inspect globalThis. Using console.log in the test, results in the error not being caught because it's not being bubbled up to the test so I call util.inspect directly on globalThis.

One thing that I noticed while testing the test function is that the error is being propagated twice (And the second error seems to have a very long stack from the same file). I lack context on the product overall, but I found this weird, so I thought I would mention it just in case it is something.

workerd/server/server.c++:4336: debug: [ TEST ] global-scope-test:validateGlobalThis
workerd/io/worker.c++:2082: info: uncaught exception; source = Uncaught; stack = AssertionError [ERR_ASSERTION]: Got unwanted exception: globalThis shouldn't fail to be inspected.
Actual message: "Error: TypeError: webgpu api is only available in Durable Objects
    at Navigator.formatJsgResourceType (node-internal:internal_inspect:2383:31)
    at formatValue (node-internal:internal_inspect:813:49)
    at formatProperty (node-internal:internal_inspect:1875:15)
    at formatRaw (node-internal:internal_inspect:1081:25)
    at formatValue (node-internal:internal_inspect:841:12)
    at inspect (node-internal:internal_inspect:397:12)
    at formatJsgResourceType (node-internal:internal_inspect:2402:23)
    at formatValue (node-internal:internal_inspect:813:49)
    at formatProperty (node-internal:internal_inspect:1875:15)
    at formatRaw (node-internal:internal_inspect:1081:25)
    at assert.fail (node-internal:internal_inspect:43:11)
    at handleMaxCallStackSize (node-internal:internal_inspect:1516:12)
    at formatRaw (node-internal:internal_inspect:1089:16)
    at formatValue (node-internal:internal_inspect:841:12)
    at inspect (node-internal:internal_inspect:397:12)
    at formatJsgResourceType (node-internal:internal_inspect:2402:23)
    at formatValue (node-internal:internal_inspect:813:49)
    at formatProperty (node-internal:internal_inspect:1875:15)
    at formatRaw (node-internal:internal_inspect:1081:25)
    at formatValue (node-internal:internal_inspect:841:12)"
    at new AssertionError (node-internal:internal_assertionerror:427:15)
    at gotUnwantedException (node-internal:internal_assert:412:15)
    at doesNotThrow (node-internal:internal_assert:153:9)
    at Object.test (worker:752:5)
kj/exception.c++:357: warning: llvm-symbolizer was not found. To symbolize stack traces, install it in your $PATH or set $LLVM_SYMBOLIZER to the location of the binary. When running tests under bazel, use `--test_env=LLVM_SYMBOLIZER=<path>`.
workerd/io/io-context.c++:355: info: uncaught exception; exception = workerd/jsg/_virtual_includes/jsg/workerd/jsg/value.h:1372: failed: jsg.Error: AssertionError: Got unwanted exception: globalThis shouldn't fail to be inspected.
Actual message: "Error: TypeError: webgpu api is only available in Durable Objects
    at Navigator.formatJsgResourceType (node-internal:internal_inspect:2383:31)
    at formatValue (node-internal:internal_inspect:813:49)
    at formatProperty (node-internal:internal_inspect:1875:15)
    at formatRaw (node-internal:internal_inspect:1081:25)
    at formatValue (node-internal:internal_inspect:841:12)
    at inspect (node-internal:internal_inspect:397:12)
    at formatJsgResourceType (node-internal:internal_inspect:2402:23)
    at formatValue (node-internal:internal_inspect:813:49)
    at formatProperty (node-internal:internal_inspect:1875:15)
    at formatRaw (node-internal:internal_inspect:1081:25)
    at assert.fail (node-internal:internal_inspect:43:11)
    at handleMaxCallStackSize (node-internal:internal_inspect:1516:12)
    at formatRaw (node-internal:internal_inspect:1089:16)
    at formatValue (node-internal:internal_inspect:841:12)
    at inspect (node-internal:internal_inspect:397:12)
    at formatJsgResourceType (node-internal:internal_inspect:2402:23)
    at formatValue (node-internal:internal_inspect:813:49)
    at formatProperty (node-internal:internal_inspect:1875:15)
    at formatRaw (node-internal:internal_inspect:1081:25)
    at formatValue (node-internal:internal_inspect:841:12)"
stack: src/workerd/server/workerd@6383ce9 src/workerd/server/workerd@4c30fec src/workerd/server/workerd@4c30a70 src/workerd/server/workerd@4c1a4c1 src/workerd/server/workerd@4c1a3db src/workerd/server/workerd@4c1a188 src/workerd/server/workerd@4c19b49 src/workerd/server/workerd@4c19493 src/workerd/server/workerd@4c19220 src/workerd/server/workerd@4c1919b src/workerd/server/workerd@4c19174 src/workerd/server/workerd@4c11431 src/workerd/server/workerd@4c11033 src/workerd/server/workerd@4c08031 src/workerd/server/workerd@4be6cdc src/workerd/server/workerd@4be7cb0 src/workerd/server/workerd@4be7aac src/workerd/server/workerd@a7ad7f0 src/workerd/server/workerd@a794bcc src/workerd/server/workerd@a794adb src/workerd/server/workerd@a79624d src/workerd/server/workerd@a796da3 src/workerd/server/workerd@a7905bb src/workerd/server/workerd@a7acf49 src/workerd/server/workerd@a792da5 src/workerd/server/workerd@a79287a src/workerd/server/workerd@3e9c58e src/workerd/server/workerd@3e9b6b2 src/workerd/server/workerd@3e9b0e1 src/workerd/server/workerd@3e9b098 src/workerd/server/workerd@3e9b06e
workerd/server/server.c++:4344: debug: [ FAIL ] global-scope-test:validateGlobalThis