ServiceWorker: OnSimpleEventFinished could be called after timed out

ServiceWorker: OnSimpleEventFinished could be called after timed out

The mojo responses can be reached to the callback even after the PendingRequest
is removed. That didn't happen on the legacy IPC because the receiver object was
owned by PendingRequest. This patch is to allow the situation that the
PendingRequest is null when the response has received.

BUG=676984
Review-Url: https://codereview.chromium.org/2628553002
Cr-Commit-Position: refs/heads/master@{#443173}
Committed: https://chromium.googlesource.com/chromium/src/+/64efa56e5bb67106cea0eb4033b866d9217d8324

[shimazu, 2017-01-10 10:18:06]

Adding a test... just a moment!

[shimazu, 2017-01-11 01:55:29]

shimazu@chromium.org changed reviewers:
+ falken@chromium.org

[shimazu, 2017-01-11 01:55:29]

ptal:)

[shimazu, 2017-01-11 02:26:28]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 02:27:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 05:08:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 05:08:05]

Dry run: This issue passed the CQ dry run.

[falken, 2017-01-11 06:31:55]

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:290:
ASSERT_TRUE(worker != nullptr);
nit: Can drop the != nullptr

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:310: const
mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {
Can you comment why this does nothing but call the callback?

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:443:
ASSERT_TRUE(worker != NULL);
ASSERT_TRUE(worker) or else change to nullptr

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:162: // -
OnStopWorker calls StopWorkerCallback
At first glance this seems weird. Why isn't something like SimulateWorkerStopped
needed anymore?

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:163: // -
OnSendMessageToWorker calls the message's respective On*Event handler
Seems like this would read better as individual comments above each function,
rather than as a list above all the functions.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:939:
base::Time::Now());
I don't quite understand this test (although I think I originally wrote it). Why
aren't the event callbacks called when the timeout_timer_ runs and sees the
requests are expired and stops the worker? And why do we need to call
FinishRequest manually?

[shimazu, 2017-01-12 01:43:06]

Patchset #3 (id:40001) has been deleted

[shimazu, 2017-01-12 01:44:16]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 01:44:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-01-12 01:45:24]

Thanks for your comments. Updated the patch.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:290:
ASSERT_TRUE(worker != nullptr);
On 2017/01/11 06:31:55, falken wrote:
> nit: Can drop the != nullptr

Done.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:310: const
mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {
On 2017/01/11 06:31:55, falken wrote:
> Can you comment why this does nothing but call the callback?

Calling the callback means sending the response back.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:443:
ASSERT_TRUE(worker != NULL);
On 2017/01/11 06:31:55, falken wrote:
> ASSERT_TRUE(worker) or else change to nullptr

Oops, that's not my intended modification. Restored.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:162: // -
OnStopWorker calls StopWorkerCallback
On 2017/01/11 06:31:55, falken wrote:
> At first glance this seems weird. Why isn't something like
SimulateWorkerStopped
> needed anymore?

It's because real IPCs can be sent through mojo. Currently OnStarted IPC message
hasn't been mojofied yet, but after mojofication it'll be also removed. Updated
the comment.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:163: // -
OnSendMessageToWorker calls the message's respective On*Event handler
On 2017/01/11 06:31:55, falken wrote:
> Seems like this would read better as individual comments above each function,
> rather than as a list above all the functions.

Done.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:939:
base::Time::Now());
On 2017/01/11 06:31:55, falken wrote:
> I don't quite understand this test (although I think I originally wrote it).
Why
> aren't the event callbacks called when the timeout_timer_ runs and sees the
> requests are expired and stops the worker? And why do we need to call
> FinishRequest manually?

The event callbacks which are registered at StartRequest are invoked with
ERROR_TIMEOUT without any IPC responses when timeout_timer_ expires. That means
the remote-side callback (mojo's response callback,
extendable_message_event_callback() in this case) still exists on the renderer
at the point, and calling the callback with ABORT should not cause any crash.
That was the cause of recent crashes on OnSimpleEventFinished.

For FinishRequest, EXPECT_FALSE() is checking if FinishRequest fails correctly
after timeout.

[commit-bot: I haz the power, 2017-01-12 01:55:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 01:55:34]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[falken, 2017-01-12 02:29:05]

Thanks this makes more sense to me now. Sorry for the newbie questions, I'm
trying to minimize review latency by asking something rather than researching
individually.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:310: const
mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {
On 2017/01/12 01:45:24, shimazu wrote:
> On 2017/01/11 06:31:55, falken wrote:
> > Can you comment why this does nothing but call the callback?
> 
> Calling the callback means sending the response back.

OK I see now the old SimulateWorkerStopped just sent the response back. I guess
we should have a similar comment.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:310: const
mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {
Would it make sense to keep the comment that we're just notifying the sender the
worker is stopped?

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:167: // StopWorker
IPC handler. This calls StopWorkerCallback by default.
The Mojo and IPC mixing is a bit confusing. Can the comments clarify it more? If
I understand correctly, OnStartWorker is an IPC handler (since StartWorker is
not mojofied), and OnStopWorker is a Mojo thing. So should this say "IPC
handler"?

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:171: // respective
On*Event handler by default.
Why is this called "legacy IPC" and the ones above are just "IPC"? Is there a
difference?

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:932:
EXPECT_EQ(SERVICE_WORKER_ERROR_TIMEOUT, error_status);
Add comment: The request should have timed out.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:934:
base::Time::Now()));
Add comment: Calling FinishRequest should be a no-op, since the request timed
out.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:935:
EXPECT_TRUE(stop_worker_callback());
Ah, what confused me is it looked like the FinishRequest call() is changing
stop_worker_callback() from FALSE to TRUE.

Add comment: The renderer should have received a StopWorker request/message (not
sure what to call these Mojo things).

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:937: // Abort
all pending events.
Suggest changing comment to something like: Simulate the renderer aborting the
requests. This should not crash (link to bug).

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:942: // Finish
StopWorker correctly.
Suggest changing comment to: Simulate the renderer stopping the worker.

[shimazu, 2017-01-12 03:48:27]

No problem, thanks for asking to me! :)
Updated. ptal again.

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2628553002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:310: const
mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {
On 2017/01/12 02:29:05, falken wrote:
> On 2017/01/12 01:45:24, shimazu wrote:
> > On 2017/01/11 06:31:55, falken wrote:
> > > Can you comment why this does nothing but call the callback?
> > 
> > Calling the callback means sending the response back.
> 
> OK I see now the old SimulateWorkerStopped just sent the response back. I
guess
> we should have a similar comment.

Done.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:310: const
mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {
On 2017/01/12 02:29:05, falken wrote:
> Would it make sense to keep the comment that we're just notifying the sender
the
> worker is stopped?

That's great! Put it.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:167: // StopWorker
IPC handler. This calls StopWorkerCallback by default.
On 2017/01/12 02:29:05, falken wrote:
> The Mojo and IPC mixing is a bit confusing. Can the comments clarify it more?
If
> I understand correctly, OnStartWorker is an IPC handler (since StartWorker is
> not mojofied), and OnStopWorker is a Mojo thing. So should this say "IPC
> handler"?

I think it might be a bit confusing, too...

Currently StartWorker/StopWorker are dispatched through mojo IPC, but the
responses of StartWorker are sent back via the chromium IPC.
In this patch, messages on both IPC systems finally reach to
EmbeddedWorkerTestHelper::OnXXX methods. That's why I thought OnStopWorker was
an IPC handler. 

// I'm using a word "IPC" to mention about "mojo IPC" and ("chromium IPC" or
"legacy IPC"), but is it confusing?

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:171: // respective
On*Event handler by default.
On 2017/01/12 02:29:05, falken wrote:
> Why is this called "legacy IPC" and the ones above are just "IPC"? Is there a
> difference?

OnMessageToWorker handles only the legacy IPC's messages while the
Start/StopWorker handles both IPCs' messages (though now chromium IPC is turned
off for the messages).

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:932:
EXPECT_EQ(SERVICE_WORKER_ERROR_TIMEOUT, error_status);
On 2017/01/12 02:29:05, falken wrote:
> Add comment: The request should have timed out.

Done.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:934:
base::Time::Now()));
On 2017/01/12 02:29:05, falken wrote:
> Add comment: Calling FinishRequest should be a no-op, since the request timed
> out.

Done.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:935:
EXPECT_TRUE(stop_worker_callback());
On 2017/01/12 02:29:05, falken wrote:
> Ah, what confused me is it looked like the FinishRequest call() is changing
> stop_worker_callback() from FALSE to TRUE.
> 
> Add comment: The renderer should have received a StopWorker request/message
(not
> sure what to call these Mojo things).

Got it. Added a comment and moved the check right after the RunUntilIdle().

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:937: // Abort
all pending events.
On 2017/01/12 02:29:05, falken wrote:
> Suggest changing comment to something like: Simulate the renderer aborting the
> requests. This should not crash (link to bug).

Thanks, updated.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:942: // Finish
StopWorker correctly.
On 2017/01/12 02:29:05, falken wrote:
> Suggest changing comment to: Simulate the renderer stopping the worker.

Done.

[falken, 2017-01-12 04:14:48]

lgtm, thanks

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:167: // StopWorker
IPC handler. This calls StopWorkerCallback by default.
On 2017/01/12 03:48:27, shimazu wrote:
> On 2017/01/12 02:29:05, falken wrote:
> > The Mojo and IPC mixing is a bit confusing. Can the comments clarify it
more?
> If
> > I understand correctly, OnStartWorker is an IPC handler (since StartWorker
is
> > not mojofied), and OnStopWorker is a Mojo thing. So should this say "IPC
> > handler"?
> 
> I think it might be a bit confusing, too...
> 
> Currently StartWorker/StopWorker are dispatched through mojo IPC, but the
> responses of StartWorker are sent back via the chromium IPC.
> In this patch, messages on both IPC systems finally reach to
> EmbeddedWorkerTestHelper::OnXXX methods. That's why I thought OnStopWorker was
> an IPC handler. 
> 
> // I'm using a word "IPC" to mention about "mojo IPC" and ("chromium IPC" or
> "legacy IPC"), but is it confusing?

I find it confusing because I think people often talk about the "Mojo" vs "IPC"
implementation instead of "Mojo IPC" vs "legacy IPC". If we always said "Mojo
IPC" or "Chromium/legacy IPC" it'd be fine. I guess if there's no clearer
terminology to use here, and that's what other people in the codebase are doing,
I guess it's OK to use IPC to refer to both. I haven't really heard "Mojo IPC"
before.

I'll leave it up to your judgement.

Also let's choose one of "legacy IPC" or "Chromium IPC" and use it consistently.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:171: // respective
On*Event handler by default.
On 2017/01/12 03:48:27, shimazu wrote:
> On 2017/01/12 02:29:05, falken wrote:
> > Why is this called "legacy IPC" and the ones above are just "IPC"? Is there
a
> > difference?
> 
> OnMessageToWorker handles only the legacy IPC's messages while the
> Start/StopWorker handles both IPCs' messages (though now chromium IPC is
turned
> off for the messages).

OK... Let's fix s/mesasge/message.

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:158: // This
simulates each chromium IPC sent from the renderer and binds |request|
Since we use "legacy" twice below, let's change this to "legacy IPC" or else the
others also to "chromium".

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:934:
EXPECT_TRUE(stop_worker_callback());
I'd put the empty line at 933 instead of 935, all of 933-940 fit logically
together (test post-conditions after "Simulate timeout.")

[shimazu, 2017-01-12 05:25:21]

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:167: // StopWorker
IPC handler. This calls StopWorkerCallback by default.
On 2017/01/12 04:14:47, falken wrote:
> On 2017/01/12 03:48:27, shimazu wrote:
> > On 2017/01/12 02:29:05, falken wrote:
> > > The Mojo and IPC mixing is a bit confusing. Can the comments clarify it
> more?
> > If
> > > I understand correctly, OnStartWorker is an IPC handler (since StartWorker
> is
> > > not mojofied), and OnStopWorker is a Mojo thing. So should this say "IPC
> > > handler"?
> > 
> > I think it might be a bit confusing, too...
> > 
> > Currently StartWorker/StopWorker are dispatched through mojo IPC, but the
> > responses of StartWorker are sent back via the chromium IPC.
> > In this patch, messages on both IPC systems finally reach to
> > EmbeddedWorkerTestHelper::OnXXX methods. That's why I thought OnStopWorker
was
> > an IPC handler. 
> > 
> > // I'm using a word "IPC" to mention about "mojo IPC" and ("chromium IPC" or
> > "legacy IPC"), but is it confusing?
> 
> I find it confusing because I think people often talk about the "Mojo" vs
"IPC"
> implementation instead of "Mojo IPC" vs "legacy IPC". If we always said "Mojo
> IPC" or "Chromium/legacy IPC" it'd be fine. I guess if there's no clearer
> terminology to use here, and that's what other people in the codebase are
doing,
> I guess it's OK to use IPC to refer to both. I haven't really heard "Mojo IPC"
> before.
> 
> I'll leave it up to your judgement.
> 
> Also let's choose one of "legacy IPC" or "Chromium IPC" and use it
consistently.

Thanks for clear explanation. 
There are several docs comparing "chromium IPC" vs "mojo IPC" actually:
https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-...
and I prefer "chromium IPC" vs "mojo IPC" because mojo is also an IPC system. 

I'll use "legacy IPC" and "mojo IPC" here.

https://codereview.chromium.org/2628553002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:171: // respective
On*Event handler by default.
On 2017/01/12 04:14:47, falken wrote:
> On 2017/01/12 03:48:27, shimazu wrote:
> > On 2017/01/12 02:29:05, falken wrote:
> > > Why is this called "legacy IPC" and the ones above are just "IPC"? Is
there
> a
> > > difference?
> > 
> > OnMessageToWorker handles only the legacy IPC's messages while the
> > Start/StopWorker handles both IPCs' messages (though now chromium IPC is
> turned
> > off for the messages).
> 
> OK... Let's fix s/mesasge/message.

Done.

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:158: // This
simulates each chromium IPC sent from the renderer and binds |request|
On 2017/01/12 04:14:47, falken wrote:
> Since we use "legacy" twice below, let's change this to "legacy IPC" or else
the
> others also to "chromium".

Sure, I'll use legacy instead of chromium.

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2628553002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:934:
EXPECT_TRUE(stop_worker_callback());
On 2017/01/12 04:14:47, falken wrote:
> I'd put the empty line at 933 instead of 935, all of 933-940 fit logically
> together (test post-conditions after "Simulate timeout.")

Done.

[shimazu, 2017-01-12 05:25:29]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-01-12 05:25:29]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2628553002/#ps100001
(title: "Fix comments")

[commit-bot: I haz the power, 2017-01-12 05:25:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 07:06:20]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484198729014330,
"parent_rev": "0d91b4b25cdb0f9164ead9ad40654e08289bcc3b", "commit_rev":
"64efa56e5bb67106cea0eb4033b866d9217d8324"}

[commit-bot: I haz the power, 2017-01-12 07:06:57]

Description was changed from

==========
ServiceWorker: OnSimpleEventFinished could be called after timed out

The mojo responses can be reached to the callback even after the PendingRequest
is removed. That didn't happen on the legacy IPC because the receiver object was
owned by PendingRequest. This patch is to allow the situation that the
PendingRequest is null when the response has received.

BUG=676984
==========

to

==========
ServiceWorker: OnSimpleEventFinished could be called after timed out

The mojo responses can be reached to the callback even after the PendingRequest
is removed. That didn't happen on the legacy IPC because the receiver object was
owned by PendingRequest. This patch is to allow the situation that the
PendingRequest is null when the response has received.

BUG=676984

Review-Url: https://codereview.chromium.org/2628553002
Cr-Commit-Position: refs/heads/master@{#443173}
Committed:
https://chromium.googlesource.com/chromium/src/+/64efa56e5bb67106cea0eb4033b8...
==========

[commit-bot: I haz the power, 2017-01-12 07:06:58]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/64efa56e5bb67106cea0eb4033b8...