FetchEvent should not forcibly reject an unresolved Promise when destructed

FetchEvent should not forcibly reject an unresolved Promise when destructed

Currently an unresolved Promise on RespondWithObserver is forcibly rejected
when the RespondWithObserver is destructed. This means that an unresolved Promise
registered with fetchEvent.respondWith() is forcibly rejected when the fetchEvent
is destructed. This behavior is not good because it makes GC observable.
(For more details, see comment #2 and #4 in the CL discussion.)

Given the above, this CL stops rejecting an unresolved Promise in
RespondWithObserver's destructor. Also this CL removes a test that were
testing the behavior. The removed test should hang after this change
(because an unresolved Promise is never resolved nor rejected).

BUG=400645
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179695

[haraken, 2014-08-06 05:22:28]

+serviceworker-reviews

PTAL

[Mads Ager (chromium), 2014-08-06 06:57:50]

Does the spec say that you have to reject unresolved promises like this?
Intuitively, I would have expected that promises had to be explicitly resolved
or rejected. Therefore, I would expect this test to hang. The current behavior
makes Oilpan GCs observable: create an unresolved promise, when it is rejected a
GC has occurred. It is usually a bad idea to make GC observable.

If we have to do this according to the spec I think this change makes sense. The
promise will be eventually rejected if the page stays active.

[haraken, 2014-08-06 07:06:57]

dominicc@, falken@: Do you have any idea on Mads' comment in #2?

[dominicc (has gone to gerrit), 2014-08-06 07:29:31]

On 2014/08/06 07:06:57, haraken wrote:
> dominicc@, falken@: Do you have any idea on Mads' comment in #2?

At a high level, Mads' comment makes sense. Per the spec we should not be
rejecting there.

However in this case I think it is Blink, not a web author, that is directly
observing the GC. What the web author gets to observe about GC is pretty
abstract (after some time they get a response from the network instead of the
Service Worker.)

I think we should change FetchEvent destructor to not reject the Promise it this
case. It's more consistent (for example, when the whole Service Worker has shut
down we do an early return in RespondWithObserver::sendResponse.) It's what I
think the spec intends.

Happy to make this change tomorrow; happy to review it if you want to do it.

Sorry for making Oilpan flaky. This even exposes V8 GC I think :(

[haraken, 2014-08-06 07:57:26]

On 2014/08/06 07:29:31, dominicc wrote:
> On 2014/08/06 07:06:57, haraken wrote:
> > dominicc@, falken@: Do you have any idea on Mads' comment in #2?
> 
> At a high level, Mads' comment makes sense. Per the spec we should not be
> rejecting there.
> 
> However in this case I think it is Blink, not a web author, that is directly
> observing the GC. What the web author gets to observe about GC is pretty
> abstract (after some time they get a response from the network instead of the
> Service Worker.)
> 
> I think we should change FetchEvent destructor to not reject the Promise it
this
> case. It's more consistent (for example, when the whole Service Worker has
shut
> down we do an early return in RespondWithObserver::sendResponse.) It's what I
> think the spec intends.
> 
> Happy to make this change tomorrow; happy to review it if you want to do it.

Updated the CL. PTAL.

[haraken, 2014-08-06 08:00:19]

Also updated the CL description.

[haraken, 2014-08-07 02:28:43]

dominicc: would you take a look at this?

[dominicc (has gone to gerrit), 2014-08-07 04:40:18]

This not LGTM, won't the assert fire if you call respondWith and never resolve
the promise?

https://codereview.chromium.org/442183003/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/fetch-event.html (left):

https://codereview.chromium.org/442183003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/fetch-event.html:124: var t = async_test(
I think we want to test this, but I can re-add a new test later.

[haraken, 2014-08-07 04:47:15]

> I think we should change FetchEvent destructor to not reject the Promise it
this
> case. It's more consistent (for example, when the whole Service Worker has
shut
> down we do an early return in RespondWithObserver::sendResponse.) It's what I
> think the spec intends.

Probably I don't fully understand what you're proposing here. What's your
proposal? (What should FetchEvent's destructor do when it finds an unresolved
Promise?)

[dominicc (has gone to gerrit), 2014-08-07 05:17:05]

On 2014/08/07 04:47:15, haraken wrote:
> > I think we should change FetchEvent destructor to not reject the Promise it
> this
> > case. It's more consistent (for example, when the whole Service Worker has
> shut
> > down we do an early return in RespondWithObserver::sendResponse.) It's what
I
> > think the spec intends.
> 
> Probably I don't fully understand what you're proposing here. What's your
> proposal? (What should FetchEvent's destructor do when it finds an unresolved
> Promise?)

~RespondWithObserver should not assert that state != PENDING, you can see from
the existing test it is possible to enter ~RespondWithObserver when state is
pending.

The destructor should do nothing for now. Later we should add something to clean
up the browser side.

[dominicc (has gone to gerrit), 2014-08-07 05:23:08]

On 2014/08/07 05:17:05, dominicc wrote:
> On 2014/08/07 04:47:15, haraken wrote:
> > > I think we should change FetchEvent destructor to not reject the Promise
it
> > this
> > > case. It's more consistent (for example, when the whole Service Worker has
> > shut
> > > down we do an early return in RespondWithObserver::sendResponse.) It's
what
> I
> > > think the spec intends.
> > 
> > Probably I don't fully understand what you're proposing here. What's your
> > proposal? (What should FetchEvent's destructor do when it finds an
unresolved
> > Promise?)
> 
> ~RespondWithObserver should not assert that state != PENDING, you can see from
> the existing test it is possible to enter ~RespondWithObserver when state is
> pending.
> 
> The destructor should do nothing for now. Later we should add something to
clean
> up the browser side.

Just looked at this with falken.

Short answer is: Just make the destructor do nothing, remove that test, and
we'll work on this offline.

We may end up using GC in the Service Worker to trigger a network error in the
page. I think that is OK because it is pretty abstract; you can get a
NetworkError for other reasons, you can't really observe GC with this.

When we implement this (there's a FIXME in
RespondWithObserver::responseWasRejected about this already) we'll work on
making the test not suck/timeout/etc.

[haraken, 2014-08-07 06:00:47]

> ~RespondWithObserver should not assert that state != PENDING, you can see from
> the existing test it is possible to enter ~RespondWithObserver when state is
> pending.
> 
> The destructor should do nothing for now. Later we should add something to
clean
> up the browser side.

Done. PTAL.

I noticed that we have the same problem in ~WaitUntilObserver.
~WaitUntilObserver does nothing, and thus an unresolved Promise on the
WaitUntilObserver is left as is.

[dominicc (has gone to gerrit), 2014-08-07 07:16:50]

LGTM

[haraken, 2014-08-07 07:27:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-07 07:28:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/442183003/60001

[commit-bot: I haz the power, 2014-08-07 08:59:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-08-07 09:08:55]

Change committed as 179695