Messaging: Fix crash when MessagePort is closed while messages are queued

Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context.
Since https://crrev.com/9c675cfdcf006e5ca978b0dfa04f187ed36f86cc, getExecutionContext()
would then return null and crash. Since close() is called when the execution
context dies and possibly in other cases, check |m_closed| before each dispatch.

BUG=649616
Committed: https://crrev.com/6ec0c90214ba34ce39e4224561235b7142d07eaa
Cr-Commit-Position: refs/heads/master@{#435190}

[falken, 2016-11-30 02:13:40]

Description was changed from

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can cause close() to be triggered, which
means getExecutionContext() would return null and crash. So we must check
m_closed before each dispatch.

BUG=649616
==========

to

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context,
so getExecutionContext() would return null and crash. Since close() is called
when the execution context dies, check |m_closed| before each dispatch.

BUG=649616
==========

[falken, 2016-11-30 04:39:27]

Description was changed from

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context,
so getExecutionContext() would return null and crash. Since close() is called
when the execution context dies, check |m_closed| before each dispatch.

BUG=649616
==========

to

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context.
Since https://crrev.com/9c675cfdcf006e5ca978b0dfa04f187ed36f86cc,
getExecutionContext()
would then return null and crash. Since close() is called when the execution
context dies and possibly in other cases, check |m_closed| before each dispatch.

BUG=649616
==========

[falken, 2016-11-30 04:39:50]

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

[falken, 2016-11-30 04:39:50]

ptal

[falken, 2016-11-30 04:39:57]

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

[commit-bot: I haz the power, 2016-11-30 04:40:16]

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

[nhiroki, 2016-11-30 07:04:27]

Nice fix! lgtm

https://codereview.chromium.org/2533323003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/workers/close-context-messageport-crash.html
(right):

https://codereview.chromium.org/2533323003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/workers/close-context-messageport-crash.html:20:
// https://crbug.com/649616.
This is not for confirming web-exposed behavior, so how about moving this to
chromium/ directory?

[falken, 2016-11-30 07:30:52]

Thanks.

https://codereview.chromium.org/2533323003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/workers/close-context-messageport-crash.html
(right):

https://codereview.chromium.org/2533323003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/workers/close-context-messageport-crash.html:20:
// https://crbug.com/649616.
On 2016/11/30 07:04:27, nhiroki (OOO until Dec 2) wrote:
> This is not for confirming web-exposed behavior, so how about moving this to
> chromium/ directory?

fast/workers/ was mostly written with just webkit/blink in mind, it has a lot of
gc and crash tests in it. So I'd rather keep it here for consistency.

[falken, 2016-11-30 07:31:32]

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

[falken, 2016-11-30 07:31:33]

+haraken for OWNER review

[haraken, 2016-11-30 07:32:37]

LGTM

[falken, 2016-11-30 07:34:31]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2016-11-30 07:35:19]

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

[nhiroki, 2016-11-30 07:52:52]

https://codereview.chromium.org/2533323003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/workers/close-context-messageport-crash.html
(right):

https://codereview.chromium.org/2533323003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/workers/close-context-messageport-crash.html:20:
// https://crbug.com/649616.
On 2016/11/30 07:30:52, falken wrote:
> On 2016/11/30 07:04:27, nhiroki (OOO until Dec 2) wrote:
> > This is not for confirming web-exposed behavior, so how about moving this to
> > chromium/ directory?
> 
> fast/workers/ was mostly written with just webkit/blink in mind, it has a lot
of
> gc and crash tests in it. So I'd rather keep it here for consistency.

Acknowledged.

[commit-bot: I haz the power, 2016-11-30 09:05:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480491271156370,
"parent_rev": "7002b98a20cc06328d1933c3f0662c5bb146215c", "commit_rev":
"d47e1fbda552648bfa9dc7589eb98dc1bd37a00b"}

[commit-bot: I haz the power, 2016-11-30 09:05:51]

Description was changed from

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context.
Since https://crrev.com/9c675cfdcf006e5ca978b0dfa04f187ed36f86cc,
getExecutionContext()
would then return null and crash. Since close() is called when the execution
context dies and possibly in other cases, check |m_closed| before each dispatch.

BUG=649616
==========

to

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context.
Since https://crrev.com/9c675cfdcf006e5ca978b0dfa04f187ed36f86cc,
getExecutionContext()
would then return null and crash. Since close() is called when the execution
context dies and possibly in other cases, check |m_closed| before each dispatch.

BUG=649616
==========

[commit-bot: I haz the power, 2016-11-30 09:05:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-30 09:07:53]

Description was changed from

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context.
Since https://crrev.com/9c675cfdcf006e5ca978b0dfa04f187ed36f86cc,
getExecutionContext()
would then return null and crash. Since close() is called when the execution
context dies and possibly in other cases, check |m_closed| before each dispatch.

BUG=649616
==========

to

==========
Messaging: Fix crash when MessagePort is closed while messages are queued

dispatchMessages() did a check at the start of function for m_closed, but it
then looped over queued messages and dispatched each one. Each dispatch causes
the onmessage handler to run, which can trigger closing the execution context.
Since https://crrev.com/9c675cfdcf006e5ca978b0dfa04f187ed36f86cc,
getExecutionContext()
would then return null and crash. Since close() is called when the execution
context dies and possibly in other cases, check |m_closed| before each dispatch.

BUG=649616

Committed: https://crrev.com/6ec0c90214ba34ce39e4224561235b7142d07eaa
Cr-Commit-Position: refs/heads/master@{#435190}
==========

[commit-bot: I haz the power, 2016-11-30 09:07:54]

Patchset 3 (id:??) landed as
https://crrev.com/6ec0c90214ba34ce39e4224561235b7142d07eaa
Cr-Commit-Position: refs/heads/master@{#435190}