Remove ActiveDOMObject::stop()

Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

BUG=610176
Committed: https://crrev.com/40c818f6f15d6db05f87fcb0ee69b58aaea226b4
Cr-Commit-Position: refs/heads/master@{#423056}

[haraken, 2016-09-26 05:31:47]

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

[commit-bot: I haz the power, 2016-09-26 05:32:05]

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

[haraken, 2016-09-26 05:34:40]

Description was changed from

==========
Remove ActiveDOMObject::stop()

BUG=610176
==========

to

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed(). And remove stop().

BUG=610176
==========

[haraken, 2016-09-26 05:34:40]

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

[commit-bot: I haz the power, 2016-09-26 06:10:18]

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

[commit-bot: I haz the power, 2016-09-26 06:10:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2016-09-28 01:32:48]

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

[commit-bot: I haz the power, 2016-09-28 01:33:06]

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-09-28 02:28:06]

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

[commit-bot: I haz the power, 2016-09-28 02:28:07]

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

[haraken, 2016-09-28 02:48:14]

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

[commit-bot: I haz the power, 2016-09-28 02:48:22]

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-09-28 03:45:53]

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

[commit-bot: I haz the power, 2016-09-28 03:45:55]

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

[haraken, 2016-09-28 04:34:38]

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

[commit-bot: I haz the power, 2016-09-28 04:34:58]

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-09-28 04:37:55]

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

[commit-bot: I haz the power, 2016-09-28 04:37:56]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[haraken, 2016-09-28 04:53:15]

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

[commit-bot: I haz the power, 2016-09-28 04:53:34]

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-09-28 05:51:37]

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

[commit-bot: I haz the power, 2016-09-28 05:51:38]

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

[haraken, 2016-09-28 06:13:13]

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

[commit-bot: I haz the power, 2016-09-28 06:13:20]

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-09-28 07:02:38]

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

[commit-bot: I haz the power, 2016-09-28 07:02:39]

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

[haraken, 2016-09-28 13:44:30]

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

[commit-bot: I haz the power, 2016-09-28 13:44:44]

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-09-28 14:58:00]

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

[commit-bot: I haz the power, 2016-09-28 14:58:02]

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

[haraken, 2016-09-28 23:58:55]

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

[commit-bot: I haz the power, 2016-09-28 23:59:21]

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

[haraken, 2016-09-29 00:06:10]

Description was changed from

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed(). And remove stop().

BUG=610176
==========

to

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

This CL also makes notifyContextDestroyed() get called a bit earlier than before
to make sure that contextDestroyed() gets called before
Document/WorkerGlobalScope starts any disruptive operations.

BUG=610176
==========

[commit-bot: I haz the power, 2016-09-29 01:26:15]

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

[commit-bot: I haz the power, 2016-09-29 01:26:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2016-10-04 06:28:13]

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

[commit-bot: I haz the power, 2016-10-04 06:28:27]

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-10-04 07:54:56]

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

[commit-bot: I haz the power, 2016-10-04 07:55:00]

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

[haraken, 2016-10-04 08:29:37]

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

[commit-bot: I haz the power, 2016-10-04 08:29:48]

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-10-04 09:54:37]

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

[commit-bot: I haz the power, 2016-10-04 09:54:38]

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

[haraken, 2016-10-04 11:57:35]

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

[commit-bot: I haz the power, 2016-10-04 11:57:48]

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-10-04 13:52:45]

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

[commit-bot: I haz the power, 2016-10-04 13:52:46]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-10-04 14:59:26]

Description was changed from

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

This CL also makes notifyContextDestroyed() get called a bit earlier than before
to make sure that contextDestroyed() gets called before
Document/WorkerGlobalScope starts any disruptive operations.

BUG=610176
==========

to

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

BUG=610176
==========

[haraken, 2016-10-04 14:59:58]

Now this finally passes all tests :D

PTAL

https://codereview.chromium.org/2366253002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2366253002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:556:
globalScope()->notifyContextDestroyed();

This is a just-in-case change. I moved notifyContextDestroyed() here to call it
before we start any disruptive operations.

I want to make the same change to Document::notifyContextDestroyed() but it
needs a couple of more work. So I added a TODO.

[dcheng, 2016-10-05 00:10:27]

Awesome! LGTM with a question

https://codereview.chromium.org/2366253002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp
(right):

https://codereview.chromium.org/2366253002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:21:
if (!executionContext)
Is this because we can be asked to provide a presentationClient() for a detached
context? Just curious, but how come we didn't hit this before?

[haraken, 2016-10-05 02:38:24]

https://codereview.chromium.org/2366253002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp
(right):

https://codereview.chromium.org/2366253002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:21:
if (!executionContext)
On 2016/10/05 00:10:27, dcheng wrote:
> Is this because we can be asked to provide a presentationClient() for a
detached
> context? Just curious, but how come we didn't hit this before?

You're correct.

We didn't hit this branch before because stop() had been called a bit earlier
than contextDestroyed(), which means that State::Inactive had been set a bit
earlier before.

To mitigate these kinds of subtle timing issues, I'm planning to move
notifyContextDestroyed() to the beginning of Document::shutdown() so that
contextDestroyed() is called earlier (and before Document starts any disruptive
operations).

[haraken, 2016-10-05 02:38:45]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-10-05 02:39:01]

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-10-05 02:47:31]

Description was changed from

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

BUG=610176
==========

to

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

BUG=610176
==========

[commit-bot: I haz the power, 2016-10-05 02:47:34]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-10-05 02:51:08]

Description was changed from

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

BUG=610176
==========

to

==========
Remove ActiveDOMObject::stop()

Now that ActiveDOMObject::stop() is called almost at the same timing as
ContextLifecycleObserver::contextDestroyed().
This CL replaces stop() with contextDestroyed() and removes stop().

BUG=610176

Committed: https://crrev.com/40c818f6f15d6db05f87fcb0ee69b58aaea226b4
Cr-Commit-Position: refs/heads/master@{#423056}
==========

[commit-bot: I haz the power, 2016-10-05 02:51:10]

Patchset 12 (id:??) landed as
https://crrev.com/40c818f6f15d6db05f87fcb0ee69b58aaea226b4
Cr-Commit-Position: refs/heads/master@{#423056}