Delay client registration of MessagePort to MessagePortChannel

Delay client registration of MessagePort to MessagePortChannel

MessagePort used to be registered to MessagePortChannel on entangle(),
and be unregisted when the ExecutionContext is stopped or MessagePort
is swept by GC. Once its start() is called, its hasPendingActivity()
will be true and that extends the MessagePort lifetime to stop() or close() call.

However, there's a time gap between the MessagePort instance is marked
as unreachable, and swept by GC system. If MessagePortChannel accesses
the MessagePort in this period, that causes use-after-poison crash.

I.e. there are two pattern of the life of MessagePort.
 1. entangle() + register -> gets unreachable -(poisoned period)-> swept + unregister
 2. entangle() + register -> start() -> stop() + unregister
 3. entangle() + register -> start() -> close() + unregister

(2) and (3) cases are OK, while (1) has a dangerous period.
This CL delays the registration from entangle() to start(), so that
the case (1) doesn't register the MessagePort to MessagePortChannel at all.

BUG=627457
Committed: https://crrev.com/f7dbf39be31d8aa9214d5d84da613508d4e06491
Cr-Commit-Position: refs/heads/master@{#405450}

[tzik, 2016-07-14 03:15:45]

tzik@chromium.org changed reviewers:
+ haraken@chromium.org, yhirano@chromium.org

[tzik, 2016-07-14 03:15:46]

PTAL

[haraken, 2016-07-14 03:43:46]

LGTM

[yhirano, 2016-07-14 04:30:23]

lgtm

description: there is one another path to close the message port: close function
exposed to MessagePort.idl.

https://codereview.chromium.org/2143003003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MessagePort.cpp (right):

https://codereview.chromium.org/2143003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MessagePort.cpp:65: close();
DCHECK(!started() || !isEntangled()) is better than calling close, I think.

[tzik, 2016-07-14 04:55:16]

Description was changed from

==========
Delay client registration of MessagePort to MessagePortChannel

MessagePort used to be registered to MessagePortChannel on entangle(),
and be unregisted when the ExecutionContext is stopped or MessagePort
is swept by GC. Once its start() is called, its hasPendingActivity()
will be true and that extends the MessagePort lifetime to stop() call.

However, there's a time gap between the MessagePort instance is marked
as unreachable, and swept by GC system. If MessagePortChannel accesses
the MessagePort in this period, that causes use-after-poison crash.

I.e. there are two pattern of the life of MessagePort.
 1. entangle() + register -> gets unreachable -(poisoned period)-> swept +
unregister
 2. entangle() + register -> start() -> stop() + unregister

(2) case is OK, while (1) has a dangerous period.
This CL delays the registration from entangle() to start(), so that
the case (1) doesn't register the MessagePort to MessagePortChannel at all.

BUG=627457
==========

to

==========
Delay client registration of MessagePort to MessagePortChannel

MessagePort used to be registered to MessagePortChannel on entangle(),
and be unregisted when the ExecutionContext is stopped or MessagePort
is swept by GC. Once its start() is called, its hasPendingActivity()
will be true and that extends the MessagePort lifetime to stop() or close()
call.

However, there's a time gap between the MessagePort instance is marked
as unreachable, and swept by GC system. If MessagePortChannel accesses
the MessagePort in this period, that causes use-after-poison crash.

I.e. there are two pattern of the life of MessagePort.
 1. entangle() + register -> gets unreachable -(poisoned period)-> swept +
unregister
 2. entangle() + register -> start() -> stop() + unregister
 3. entangle() + register -> start() -> close() + unregister

(2) and (3) cases are OK, while (1) has a dangerous period.
This CL delays the registration from entangle() to start(), so that
the case (1) doesn't register the MessagePort to MessagePortChannel at all.

BUG=627457
==========

[tzik, 2016-07-14 04:59:54]

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

[tzik, 2016-07-14 04:59:57]

https://codereview.chromium.org/2143003003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/MessagePort.cpp (right):

https://codereview.chromium.org/2143003003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/MessagePort.cpp:65: close();
On 2016/07/14 04:30:22, yhirano wrote:
> DCHECK(!started() || !isEntangled()) is better than calling close, I think.

Done.

[commit-bot: I haz the power, 2016-07-14 05:00:07]

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, 2016-07-14 06:52:44]

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

[commit-bot: I haz the power, 2016-07-14 06:52:50]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tzik, 2016-07-14 07:10:26]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-07-14 07:10:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2143003003/#ps20001
(title: "s/close()/DCHECK/")

[commit-bot: I haz the power, 2016-07-14 07:10:38]

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, 2016-07-14 08:24:08]

Description was changed from

==========
Delay client registration of MessagePort to MessagePortChannel

MessagePort used to be registered to MessagePortChannel on entangle(),
and be unregisted when the ExecutionContext is stopped or MessagePort
is swept by GC. Once its start() is called, its hasPendingActivity()
will be true and that extends the MessagePort lifetime to stop() or close()
call.

However, there's a time gap between the MessagePort instance is marked
as unreachable, and swept by GC system. If MessagePortChannel accesses
the MessagePort in this period, that causes use-after-poison crash.

I.e. there are two pattern of the life of MessagePort.
 1. entangle() + register -> gets unreachable -(poisoned period)-> swept +
unregister
 2. entangle() + register -> start() -> stop() + unregister
 3. entangle() + register -> start() -> close() + unregister

(2) and (3) cases are OK, while (1) has a dangerous period.
This CL delays the registration from entangle() to start(), so that
the case (1) doesn't register the MessagePort to MessagePortChannel at all.

BUG=627457
==========

to

==========
Delay client registration of MessagePort to MessagePortChannel

MessagePort used to be registered to MessagePortChannel on entangle(),
and be unregisted when the ExecutionContext is stopped or MessagePort
is swept by GC. Once its start() is called, its hasPendingActivity()
will be true and that extends the MessagePort lifetime to stop() or close()
call.

However, there's a time gap between the MessagePort instance is marked
as unreachable, and swept by GC system. If MessagePortChannel accesses
the MessagePort in this period, that causes use-after-poison crash.

I.e. there are two pattern of the life of MessagePort.
 1. entangle() + register -> gets unreachable -(poisoned period)-> swept +
unregister
 2. entangle() + register -> start() -> stop() + unregister
 3. entangle() + register -> start() -> close() + unregister

(2) and (3) cases are OK, while (1) has a dangerous period.
This CL delays the registration from entangle() to start(), so that
the case (1) doesn't register the MessagePort to MessagePortChannel at all.

BUG=627457
==========

[commit-bot: I haz the power, 2016-07-14 08:24:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-14 08:24:15]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 08:25:34]

Description was changed from

==========
Delay client registration of MessagePort to MessagePortChannel

MessagePort used to be registered to MessagePortChannel on entangle(),
and be unregisted when the ExecutionContext is stopped or MessagePort
is swept by GC. Once its start() is called, its hasPendingActivity()
will be true and that extends the MessagePort lifetime to stop() or close()
call.

However, there's a time gap between the MessagePort instance is marked
as unreachable, and swept by GC system. If MessagePortChannel accesses
the MessagePort in this period, that causes use-after-poison crash.

I.e. there are two pattern of the life of MessagePort.
 1. entangle() + register -> gets unreachable -(poisoned period)-> swept +
unregister
 2. entangle() + register -> start() -> stop() + unregister
 3. entangle() + register -> start() -> close() + unregister

(2) and (3) cases are OK, while (1) has a dangerous period.
This CL delays the registration from entangle() to start(), so that
the case (1) doesn't register the MessagePort to MessagePortChannel at all.

BUG=627457
==========

to

==========
Delay client registration of MessagePort to MessagePortChannel

MessagePort used to be registered to MessagePortChannel on entangle(),
and be unregisted when the ExecutionContext is stopped or MessagePort
is swept by GC. Once its start() is called, its hasPendingActivity()
will be true and that extends the MessagePort lifetime to stop() or close()
call.

However, there's a time gap between the MessagePort instance is marked
as unreachable, and swept by GC system. If MessagePortChannel accesses
the MessagePort in this period, that causes use-after-poison crash.

I.e. there are two pattern of the life of MessagePort.
 1. entangle() + register -> gets unreachable -(poisoned period)-> swept +
unregister
 2. entangle() + register -> start() -> stop() + unregister
 3. entangle() + register -> start() -> close() + unregister

(2) and (3) cases are OK, while (1) has a dangerous period.
This CL delays the registration from entangle() to start(), so that
the case (1) doesn't register the MessagePort to MessagePortChannel at all.

BUG=627457

Committed: https://crrev.com/f7dbf39be31d8aa9214d5d84da613508d4e06491
Cr-Commit-Position: refs/heads/master@{#405450}
==========

[commit-bot: I haz the power, 2016-07-14 08:25:35]

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