[Fetch API] Do not call v8 Extra script when the worker is terminating (issue 2006803006 by yhirano@chromium.org)

[yhi...@chromium.org]

Reviewers: tyoshino, domenic
CL: https://codereview.chromium.org/2006803006/

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
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

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+187, -27 lines):
M third_party/WebKit/Source/core/streams/ReadableStreamController.h
M third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp
M third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
M third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp

[tyos...@chromium.org]

lgtm

Description:
some early returns -> early returns at some places
or
adds some early returns

https://codereview.chromium.org/2006803006/

[yhi...@chromium.org]

thank you!

https://codereview.chromium.org/2006803006/

[yhi...@chromium.org]

> Description:
> some early returns -> early returns at some places
> or
> adds some early returns

Done.

https://codereview.chromium.org/2006803006/

[commit-bot@chromium.org via codereview.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


https://codereview.chromium.org/2006803006/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #4 (id:110005)

https://codereview.chromium.org/2006803006/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 4 (id:??) landed as
https://crrev.com/bef901ae9100f30e3ee2fb185c4197a2de55e4c1
Cr-Commit-Position: refs/heads/master@{#396131}

https://codereview.chromium.org/2006803006/

[har...@chromium.org]

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.)


https://codereview.chromium.org/2006803006/

[yhi...@google.com]

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.


https://codereview.chromium.org/2006803006/

[har...@chromium.org]

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.


https://codereview.chromium.org/2006803006/

[har...@chromium.org]

+bashi +yukishiino


https://codereview.chromium.org/2006803006/

[yhi...@chromium.org]

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.


https://codereview.chromium.org/2006803006/

[yhi...@chromium.org]

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.


https://codereview.chromium.org/2006803006/

[har...@chromium.org]

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.



https://codereview.chromium.org/2006803006/

[yukis...@chromium.org]

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(...)


https://codereview.chromium.org/2006803006/

[ba...@chromium.org]

> 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.

https://codereview.chromium.org/2006803006/

[yhi...@google.com]

> 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.


https://codereview.chromium.org/2006803006/