[wasm] Enter V8 before running foreground work [v8/v8 : main]
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Toon, I don't know exactly what I'm doing here, so please check if this is a reasonable fix.
+Jakob for a second pair of eyes.
Related details
- Toon Verwaest
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest added 1 comment![]( "Open in Gerrit")
EnterV8InternalScope<HandleScope, false> v8_scope{This is basically `EnterV8NoScriptScope` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api-inl.h;l=322;drc=9d6f972e353f541f4f3261fa971d20fa61d839bd;bpv=1;bpt=1). You probably don't want this given that you're explicitly mentioning that user code could run?
What about a regular `EnterV8Scope`?
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
EnterV8InternalScope<HandleScope, false> v8_scope{This is basically `EnterV8NoScriptScope` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api-inl.h;l=322;drc=9d6f972e353f541f4f3261fa971d20fa61d839bd;bpv=1;bpt=1). You probably don't want this given that you're explicitly mentioning that user code could run?
What about a regular `EnterV8Scope`?
Yes, we need to run user code.
`EnterV8Scope` is what I used in PS1, and it fails one debugging test. I could fix it by having a `MicrotasksScope` plus an `EnterV8Scope`. Would that be better than the `EnterV8InternalScope`?
Related details
- Toon Verwaest
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
EnterV8InternalScope<HandleScope, false> v8_scope{Clemens BackesThis is basically `EnterV8NoScriptScope` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api-inl.h;l=322;drc=9d6f972e353f541f4f3261fa971d20fa61d839bd;bpv=1;bpt=1). You probably don't want this given that you're explicitly mentioning that user code could run?
What about a regular `EnterV8Scope`?
Yes, we need to run user code.
`EnterV8Scope` is what I used in PS1, and it fails one debugging test. I could fix it by having a `MicrotasksScope` plus an `EnterV8Scope`. Would that be better than the `EnterV8InternalScope`?
I did that in PS3 now.
Related details
- Toon Verwaest
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest voted Code-Review+1![]( "Open in Gerrit")
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest added 1 comment![]( "Open in Gerrit")
Not executing things in a microtask scope is a bit weird. It would make sense to figure out the expected semantics. Not having a microtaskscope (or not executing them) means that async code won't make progress.
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
// Enter V8 before running synchronous work (which might involve callbacks
// to user code).
MicrotasksScope microtasks_scope{
reinterpret_cast<v8::Isolate*>(job->isolate_),
job->native_context_->microtask_queue(),
v8::MicrotasksScope::kDoNotRunMicrotasks};
EnterV8Scope<> v8_scope{job->isolate_,
Utils::ToLocal(job->native_context_),
RuntimeCallCounterId::kWasmAsyncCompilation};do all foreground jobs need this? if not, could it be moved into the RunForeground method of each job that does need it?
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
Andreas and I spoke about this, it looks like our implementation is using custom tasks and pseudo promise-like resolution because of a misunderstanding when reading the Wasm spec, which specifies real (separate) promises for the module compilation and instantiation. If we used real promises here, and instantiation was just a reaction to the module compilation promise resolving, then we wouldn't need a custom task here, it would just execute in a normal (micro)task.
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Andreas and I spoke about this, it looks like our implementation is using custom tasks and pseudo promise-like resolution because of a misunderstanding when reading the Wasm spec, which specifies real (separate) promises for the module compilation and instantiation. If we used real promises here, and instantiation was just a reaction to the module compilation promise resolving, then we wouldn't need a custom task here, it would just execute in a normal (micro)task.
Let's talk about this next week then when I am back in the office. I don't understand the microtasks and promises business enough to understand the difference in observable behaviour between the different options.
Related details
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 3 comments![]( "Open in Gerrit")
Clemens BackesAndreas and I spoke about this, it looks like our implementation is using custom tasks and pseudo promise-like resolution because of a misunderstanding when reading the Wasm spec, which specifies real (separate) promises for the module compilation and instantiation. If we used real promises here, and instantiation was just a reaction to the module compilation promise resolving, then we wouldn't need a custom task here, it would just execute in a normal (micro)task.
Let's talk about this next week then when I am back in the office. I don't understand the microtasks and promises business enough to understand the difference in observable behaviour between the different options.
I discussed this with Andreas. While implementing this properly via two promises is probably the better fix, it's also a lot more involved, and potentially introduces security risks by storing intermediate data on the JS heap.
For now I opened a tracking bug and put it on our backlog: https://crbug.com/454827127
In the meantime I'll to ahead and land this CL.
EnterV8InternalScope<HandleScope, false> v8_scope{Clemens BackesThis is basically `EnterV8NoScriptScope` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api-inl.h;l=322;drc=9d6f972e353f541f4f3261fa971d20fa61d839bd;bpv=1;bpt=1). You probably don't want this given that you're explicitly mentioning that user code could run?
What about a regular `EnterV8Scope`?
Clemens BackesYes, we need to run user code.
`EnterV8Scope` is what I used in PS1, and it fails one debugging test. I could fix it by having a `MicrotasksScope` plus an `EnterV8Scope`. Would that be better than the `EnterV8InternalScope`?
I did that in PS3 now.
Done
// Enter V8 before running synchronous work (which might involve callbacks
// to user code).
MicrotasksScope microtasks_scope{
reinterpret_cast<v8::Isolate*>(job->isolate_),
job->native_context_->microtask_queue(),
v8::MicrotasksScope::kDoNotRunMicrotasks};
EnterV8Scope<> v8_scope{job->isolate_,
Utils::ToLocal(job->native_context_),
RuntimeCallCounterId::kWasmAsyncCompilation};do all foreground jobs need this? if not, could it be moved into the RunForeground method of each job that does need it?
We only have two foreground tasks (after removing the third one in https://crrev.com/c/6973469): `Fail` and `FinishCompilation`. Both need this, so I think we should leave it here.
Related details
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
Clemens BackesAndreas and I spoke about this, it looks like our implementation is using custom tasks and pseudo promise-like resolution because of a misunderstanding when reading the Wasm spec, which specifies real (separate) promises for the module compilation and instantiation. If we used real promises here, and instantiation was just a reaction to the module compilation promise resolving, then we wouldn't need a custom task here, it would just execute in a normal (micro)task.
Clemens BackesLet's talk about this next week then when I am back in the office. I don't understand the microtasks and promises business enough to understand the difference in observable behaviour between the different options.
I discussed this with Andreas. While implementing this properly via two promises is probably the better fix, it's also a lot more involved, and potentially introduces security risks by storing intermediate data on the JS heap.
For now I opened a tracking bug and put it on our backlog: https://crbug.com/454827127
In the meantime I'll to ahead and land this CL.
Entering V8 scopes in tasks that didn't previously expect it is also a potential security risk though...
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Clemens BackesAndreas and I spoke about this, it looks like our implementation is using custom tasks and pseudo promise-like resolution because of a misunderstanding when reading the Wasm spec, which specifies real (separate) promises for the module compilation and instantiation. If we used real promises here, and instantiation was just a reaction to the module compilation promise resolving, then we wouldn't need a custom task here, it would just execute in a normal (micro)task.
Clemens BackesLet's talk about this next week then when I am back in the office. I don't understand the microtasks and promises business enough to understand the difference in observable behaviour between the different options.
Leszek SwirskiI discussed this with Andreas. While implementing this properly via two promises is probably the better fix, it's also a lot more involved, and potentially introduces security risks by storing intermediate data on the JS heap.
For now I opened a tracking bug and put it on our backlog: https://crbug.com/454827127
In the meantime I'll to ahead and land this CL.
Entering V8 scopes in tasks that didn't previously expect it is also a potential security risk though...
Hm. So you are voting for *not* landing this and instead plan to work on the backlog issue ASAP?
Related details
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
Clemens BackesAndreas and I spoke about this, it looks like our implementation is using custom tasks and pseudo promise-like resolution because of a misunderstanding when reading the Wasm spec, which specifies real (separate) promises for the module compilation and instantiation. If we used real promises here, and instantiation was just a reaction to the module compilation promise resolving, then we wouldn't need a custom task here, it would just execute in a normal (micro)task.
Clemens BackesLet's talk about this next week then when I am back in the office. I don't understand the microtasks and promises business enough to understand the difference in observable behaviour between the different options.
Leszek SwirskiI discussed this with Andreas. While implementing this properly via two promises is probably the better fix, it's also a lot more involved, and potentially introduces security risks by storing intermediate data on the JS heap.
For now I opened a tracking bug and put it on our backlog: https://crbug.com/454827127
In the meantime I'll to ahead and land this CL.
Clemens BackesEntering V8 scopes in tasks that didn't previously expect it is also a potential security risk though...
Hm. So you are voting for *not* landing this and instead plan to work on the backlog issue ASAP?
I'm tempted to suggest this yes, because this is kind of a weird task and things like kDoNotRunMicrotasks make it weirder
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Clemens BackesAndreas and I spoke about this, it looks like our implementation is using custom tasks and pseudo promise-like resolution because of a misunderstanding when reading the Wasm spec, which specifies real (separate) promises for the module compilation and instantiation. If we used real promises here, and instantiation was just a reaction to the module compilation promise resolving, then we wouldn't need a custom task here, it would just execute in a normal (micro)task.
Clemens BackesLet's talk about this next week then when I am back in the office. I don't understand the microtasks and promises business enough to understand the difference in observable behaviour between the different options.
Leszek SwirskiI discussed this with Andreas. While implementing this properly via two promises is probably the better fix, it's also a lot more involved, and potentially introduces security risks by storing intermediate data on the JS heap.
For now I opened a tracking bug and put it on our backlog: https://crbug.com/454827127
In the meantime I'll to ahead and land this CL.
Clemens BackesEntering V8 scopes in tasks that didn't previously expect it is also a potential security risk though...
Leszek SwirskiHm. So you are voting for *not* landing this and instead plan to work on the backlog issue ASAP?
I'm tempted to suggest this yes, because this is kind of a weird task and things like kDoNotRunMicrotasks make it weirder
Ack, abandoning this then and blocking the DCHECK failure on the refactoring task.
Related details
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |