|
|
Description[Fetch API] Do not call v8 Extra script when the worker is terminating
ReadableStreamOperations assumes v8 Extra scripts cannot return an empty
handle but that assumption breaks when the worker is terminating. This CL
adds some early returns to avoid such issues.
This change is a workaround and should be reverted once the graceful
shutdown mechanism is introduced.
BUG=614272
Committed: https://crrev.com/bef901ae9100f30e3ee2fb185c4197a2de55e4c1
Cr-Commit-Position: refs/heads/master@{#396131}
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 27 (10 generated)
Description was changed from ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL some early returns to avoid such issues. BUG=614272 ========== to ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL some early returns to avoid such issues. This change is a workaround and should be reverted once the graceful shutdown mechanism is introduced. BUG=614272 ==========
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
yhirano@chromium.org changed reviewers: + domenic@chromium.org, tyoshino@chromium.org
lgtm Description: some early returns -> early returns at some places or adds some early returns
Description was changed from ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL some early returns to avoid such issues. This change is a workaround and should be reverted once the graceful shutdown mechanism is introduced. BUG=614272 ========== to ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL adds some early returns to avoid such issues. This change is a workaround and should be reverted once the graceful shutdown mechanism is introduced. BUG=614272 ==========
thank you!
> Description: > some early returns -> early returns at some places > or > adds some early returns Done.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006803006/110005 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006803006/110005
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL adds some early returns to avoid such issues. This change is a workaround and should be reverted once the graceful shutdown mechanism is introduced. BUG=614272 ========== to ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL adds some early returns to avoid such issues. This change is a workaround and should be reverted once the graceful shutdown mechanism is introduced. BUG=614272 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:110005)
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL adds some early returns to avoid such issues. This change is a workaround and should be reverted once the graceful shutdown mechanism is introduced. BUG=614272 ========== to ========== [Fetch API] Do not call v8 Extra script when the worker is terminating ReadableStreamOperations assumes v8 Extra scripts cannot return an empty handle but that assumption breaks when the worker is terminating. This CL adds some early returns to avoid such issues. This change is a workaround and should be reverted once the graceful shutdown mechanism is introduced. BUG=614272 Committed: https://crrev.com/bef901ae9100f30e3ee2fb185c4197a2de55e4c1 Cr-Commit-Position: refs/heads/master@{#396131} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bef901ae9100f30e3ee2fb185c4197a2de55e4c1 Cr-Commit-Position: refs/heads/master@{#396131}
Message was sent while issue was closed.
Why do you need to check isExecutionContextTerminating? If the worker is terminating, the V8 API being executed at that point will return an empty Maybe handle. So you should just check if the returned Maybe handle is empty or not. (That's the purpose of the Maybe handle -- we introduced the Maybe handle to avoid adding the isTerminating check to the code base.)
Message was sent while issue was closed.
On 2016/05/26 08:08:23, haraken wrote: > Why do you need to check isExecutionContextTerminating? If the worker is > terminating, the V8 API being executed at that point will return an empty Maybe > handle. So you should just check if the returned Maybe handle is empty or not. > (That's the purpose of the Maybe handle -- we introduced the Maybe handle to > avoid adding the isTerminating check to the code base.) I wanted to keep the assertion (v8CallOrCrash) as much as possible. Before: A function f must not return an empty MaybeLocal. After: A function f must not return an empty MaybeLocal if the execution context is not terminating.
Message was sent while issue was closed.
On 2016/05/26 08:18:18, yhirano1 wrote: > On 2016/05/26 08:08:23, haraken wrote: > > Why do you need to check isExecutionContextTerminating? If the worker is > > terminating, the V8 API being executed at that point will return an empty > Maybe > > handle. So you should just check if the returned Maybe handle is empty or not. > > (That's the purpose of the Maybe handle -- we introduced the Maybe handle to > > avoid adding the isTerminating check to the code base.) > > I wanted to keep the assertion (v8CallOrCrash) as much as possible. > Before: > A function f must not return an empty MaybeLocal. > > After: > A function f must not return an empty MaybeLocal if the execution context is > not terminating. Is it possible that the function f returns an empty MaybeLocal in a valid scenario? The assumption of the Maybe handle is: Maybe handle returns false if and only if the V8 API throws an exception (or OOM) There might be some edge cases where the assumption holds (which is a bug) but it should be almost 100% guaranteed.
Message was sent while issue was closed.
haraken@chromium.org changed reviewers: + bashi@chromium.org, haraken@chromium.org, yukishiino@chromium.org
Message was sent while issue was closed.
+bashi +yukishiino
Message was sent while issue was closed.
On 2016/05/26 08:38:25, haraken wrote: > On 2016/05/26 08:18:18, yhirano1 wrote: > > On 2016/05/26 08:08:23, haraken wrote: > > > Why do you need to check isExecutionContextTerminating? If the worker is > > > terminating, the V8 API being executed at that point will return an empty > > Maybe > > > handle. So you should just check if the returned Maybe handle is empty or > not. > > > (That's the purpose of the Maybe handle -- we introduced the Maybe handle to > > > avoid adding the isTerminating check to the code base.) > > > > I wanted to keep the assertion (v8CallOrCrash) as much as possible. > > Before: > > A function f must not return an empty MaybeLocal. > > > > After: > > A function f must not return an empty MaybeLocal if the execution context is > > not terminating. > > Is it possible that the function f returns an empty MaybeLocal in a valid > scenario? The assumption of the Maybe handle is: > > Maybe handle returns false if and only if the V8 API throws an exception (or > OOM) > > There might be some edge cases where the assumption holds (which is a bug) but > it should be almost 100% guaranteed. For example, IsReadableStreamDisturbed called from ReadableStreamOperations::isDisturbed must not return an empty MaybeLocal. But it does. There may be some error causes: 1) /modules/fetch or /core/streams bug 2) the worker is terminating 3) OOM 4) other v8 / bindings bug I don't care about 3 and 4 (i.e., it's OK to kill the renderer in these cases). I need to check 1. I don't want the renderer to crash when the worker is terminating. I could put early returns to allow all cases, but I didn't think it's the right way.
Message was sent while issue was closed.
Also, crashes in v8::Invoke are also related and I am wondering if these crashes are also caused by the worker termination (See "Invoke" cases in the crbug entry). That's why I put early returns before calling functions and I want to see if this modification reduces the crashes.
Message was sent while issue was closed.
On 2016/05/26 08:56:45, yhirano wrote: > On 2016/05/26 08:38:25, haraken wrote: > > On 2016/05/26 08:18:18, yhirano1 wrote: > > > On 2016/05/26 08:08:23, haraken wrote: > > > > Why do you need to check isExecutionContextTerminating? If the worker is > > > > terminating, the V8 API being executed at that point will return an empty > > > Maybe > > > > handle. So you should just check if the returned Maybe handle is empty or > > not. > > > > (That's the purpose of the Maybe handle -- we introduced the Maybe handle > to > > > > avoid adding the isTerminating check to the code base.) > > > > > > I wanted to keep the assertion (v8CallOrCrash) as much as possible. > > > Before: > > > A function f must not return an empty MaybeLocal. > > > > > > After: > > > A function f must not return an empty MaybeLocal if the execution context > is > > > not terminating. > > > > Is it possible that the function f returns an empty MaybeLocal in a valid > > scenario? The assumption of the Maybe handle is: > > > > Maybe handle returns false if and only if the V8 API throws an exception (or > > OOM) > > > > There might be some edge cases where the assumption holds (which is a bug) but > > it should be almost 100% guaranteed. > > For example, IsReadableStreamDisturbed called from > ReadableStreamOperations::isDisturbed must not return an empty MaybeLocal. But > it does. There may be some error causes: > > 1) /modules/fetch or /core/streams bug > 2) the worker is terminating > 3) OOM > 4) other v8 / bindings bug > > I don't care about 3 and 4 (i.e., it's OK to kill the renderer in these cases). > I need to check 1. Then you should fix the bug instead :) > I don't want the renderer to crash when the worker is terminating. Yes, and that's exactly why we need the Maybe handle. If the Maybe handle returns an empty handle, it means that something you cannot handle has happened (including worker termination, OOM etc), so you should early-return. Note that we introduced the Maybe handle to remove the isTermination checks from the code base. Given that we're working on the graceful shutdown which will anyway remove the isTermination checks, I don't think we need to stick to this issue too deeply (i.e., I'm okay with this CL if there is no other easy way to fix the crash). But please don't spread out the pattern in other places in the code base.
Message was sent while issue was closed.
To me, it seems better to have a following API in the binding instead of v8CallOrCrash(). v8CallOrDefaultTo(call, defaultValue) { Maybe maybe = v8Call(call); if (maybe.ToLocal(&value)) return value; CHECK(worker is terminating); // [1] return defaultValue; } We may not agree to have the check at [1]. Supposing that everything works correctly, we don't need the check. Or, DCHECK() may be acceptable. Or, we may really want it. I think we can support these varieties if necessary. Say, v8CallOrDefaultToWithCheckingWorkerTermination(...) v8CallOrDefaultTo(...)
Message was sent while issue was closed.
On 2016/05/26 08:56:45, yhirano wrote: > On 2016/05/26 08:38:25, haraken wrote: > > On 2016/05/26 08:18:18, yhirano1 wrote: > > > On 2016/05/26 08:08:23, haraken wrote: > > > > Why do you need to check isExecutionContextTerminating? If the worker is > > > > terminating, the V8 API being executed at that point will return an empty > > > Maybe > > > > handle. So you should just check if the returned Maybe handle is empty or > > not. > > > > (That's the purpose of the Maybe handle -- we introduced the Maybe handle > to > > > > avoid adding the isTerminating check to the code base.) > > > > > > I wanted to keep the assertion (v8CallOrCrash) as much as possible. > > > Before: > > > A function f must not return an empty MaybeLocal. > > > > > > After: > > > A function f must not return an empty MaybeLocal if the execution context > is > > > not terminating. > > > > Is it possible that the function f returns an empty MaybeLocal in a valid > > scenario? The assumption of the Maybe handle is: > > > > Maybe handle returns false if and only if the V8 API throws an exception (or > > OOM) > > > > There might be some edge cases where the assumption holds (which is a bug) but > > it should be almost 100% guaranteed. > > For example, IsReadableStreamDisturbed called from > ReadableStreamOperations::isDisturbed must not return an empty MaybeLocal. But > it does. There may be some error causes: > > 1) /modules/fetch or /core/streams bug > 2) the worker is terminating > 3) OOM > 4) other v8 / bindings bug > > I don't care about 3 and 4 (i.e., it's OK to kill the renderer in these cases). > I need to check 1. > I don't want the renderer to crash when the worker is terminating. > > I could put early returns to allow all cases, but I didn't think it's the right > way. Help me understand: What /modules/fetch or /core/streams bugs do you have in your mind? I might misunderstand something but I thought that in case 1) v8 would throw an exception, returning an empty MaybeLocal.
Message was sent while issue was closed.
> yukishiino@ nhiroki@ and I are working to solve this problem, so I think we don't need to invent a new mechanism. > haraken@ >> I need to check 1. > Then you should fix the bug instead :) Yeah, I generally put assertions to detect bugs. > Given that we're working on the graceful shutdown which will anyway remove the > isTermination checks, I don't think we need to stick to this issue too deeply > (i.e., I'm okay with this CL if there is no other easy way to fix the crash). > But please don't spread out the pattern in other places in the code base. Thanks, I see. I am not a fan of this kind of code at all. > bashi@ > Help me understand: What /modules/fetch or /core/streams bugs do you have in > your mind? If we mis-code ReadableStream.js or give wrong arguments to the script from C++, it could throw an exception. |