Issue 1831743002: Worker: Move an assertion to a correct place

Issue 1831743002: Worker: Move an assertion to a correct place (Closed)

Created:
4 years, 9 months ago by nhiroki

Modified:
4 years, 9 months ago

Reviewers:
haraken, yhirano

CC:
chromium-reviews, blink-reviews, kinuko+worker_chromium.org, falken, blink-worker-reviews_chromium.org, horo+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Worker: Move an assertion to a correct place This CL fixes an assertion failure happening only on non-oilpan builds. WorkerThread::performShutdownTask() assumes that the count of references to WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a reference to the context is still alive at the point and the assertion fails. This CL moves the assertion to after the line to dispose the loader. <More details> The timing to release a reference to the context owned by the loader was changed by this patch: https://codereview.chromium.org/1749633002 Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was supposed to release the reference before performShutdownTask() as follows: WorkerThread::shutdown() WorkerGlobalScope::dispose() stopActiveDOMObjects(); StopDetector::stop() FetchManager::stop() FetchManager::Loader::dispose() // Release WorkerThreadableLoader. WorkerThread::performShutdownTask() // The assertion can pass. The patch removed StopDetector and instead made FetchManager::Loader to observe lifecycle of the context to release the reference. The context destruction is notified after the assertion as follows: WorkerThread::shutdown() WorkerThread::performShutdownTask() // The assertion cannot pass. WorkerGlobalScope::notifyContextDestroyed() LifecycleNotifier::notifyContextDestroyed() FetchManager::contextDestroyed() FetchManager::Loader::dispose() // Release WorkerThreadableLoader. BUG=594230 Committed: https://crrev.com/0fdc4685d3b7e5134109b814eb08d8c265cdb84c Cr-Commit-Position: refs/heads/master@{#383048}

Patch Set 1 #

Created: 4 years, 9 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1 line, -5 lines)			Patch
	M	third_party/WebKit/LayoutTests/TestExpectations	View		1 chunk	+0 lines, -4 lines	0 comments	Download
	M	third_party/WebKit/Source/core/workers/WorkerThread.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 13 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

nhiroki

Description was changed from ========== Worker: Move an assertion for non-oilpan builds to a correct ...

4 years, 9 months ago (2016-03-24 07:39:54 UTC) #1

Description was changed from

==========
Worker: Move an assertion for non-oilpan builds to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to actually release the reference.

<Detailed description>

The timing to release references to the context owned by WorkerThreadableLoader
was changed by CL[1].

Before [1], GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the references to the context before the assertion as
follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Releases WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The CL [1] removed StopDetector and instead made FetchManager::Loader to observe
the context lifecycle. The event of the context destruction is notified after
the assertion, so the assertion started to fail.

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Releases WorkerThreadableLoader.

BUG=594230
==========

to

==========
Worker: Move an assertion for non-oilpan builds to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.


<More details>

The timing to release references to the context owned by the loader was changed
by the following patch:

  https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release references to the context before performShutdownTask():

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
the context lifecycle. The event of the context destruction is notified after
the assertion.

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

nhiroki

Description was changed from ========== Worker: Move an assertion for non-oilpan builds to a correct ...

4 years, 9 months ago (2016-03-24 08:12:34 UTC) #2

Description was changed from

==========
Worker: Move an assertion for non-oilpan builds to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.


<More details>

The timing to release references to the context owned by the loader was changed
by the following patch:

  https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release references to the context before performShutdownTask():

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
the context lifecycle. The event of the context destruction is notified after
the assertion.

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

to

==========
Worker: Move an assertion for non-oilpan builds to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.

<More details>

The timing to release a reference to the context owned by the loader was changed
by this patch: https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the reference before performShutdownTask() as follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
lifecycle of the context. The context destruction is notified after the
assertion as follows:

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

nhiroki

Description was changed from ========== Worker: Move an assertion for non-oilpan builds to a correct ...

4 years, 9 months ago (2016-03-24 08:22:26 UTC) #3

Description was changed from

==========
Worker: Move an assertion for non-oilpan builds to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.

<More details>

The timing to release a reference to the context owned by the loader was changed
by this patch: https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the reference before performShutdownTask() as follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
lifecycle of the context. The context destruction is notified after the
assertion as follows:

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

to

==========
Worker: Move an assertion to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.

<More details>

The timing to release a reference to the context owned by the loader was changed
by this patch: https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the reference before performShutdownTask() as follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
lifecycle of the context. The context destruction is notified after the
assertion as follows:

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

nhiroki

Description was changed from ========== Worker: Move an assertion to a correct place This CL ...

4 years, 9 months ago (2016-03-24 08:47:19 UTC) #8

Description was changed from

==========
Worker: Move an assertion to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.

<More details>

The timing to release a reference to the context owned by the loader was changed
by this patch: https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the reference before performShutdownTask() as follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
lifecycle of the context. The context destruction is notified after the
assertion as follows:

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

to

==========
Worker: Move an assertion to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.

<More details>

The timing to release a reference to the context owned by the loader was changed
by this patch: https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the reference before performShutdownTask() as follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
lifecycle of the context to release the reference. The context destruction is
notified after the assertion as follows:

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1831743002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831743002/1

4 years, 9 months ago (2016-03-24 08:47:55 UTC) #10

commit-bot: I haz the power

Description was changed from ========== Worker: Move an assertion to a correct place This CL ...

4 years, 9 months ago (2016-03-24 10:25:05 UTC) #12

Message was sent while issue was closed.

Description was changed from

==========
Worker: Move an assertion to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.

<More details>

The timing to release a reference to the context owned by the loader was changed
by this patch: https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the reference before performShutdownTask() as follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
lifecycle of the context to release the reference. The context destruction is
notified after the assertion as follows:

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230
==========

to

==========
Worker: Move an assertion to a correct place

This CL fixes an assertion failure happening only on non-oilpan builds.

WorkerThread::performShutdownTask() assumes that the count of references to
WorkerGlobalScope is only one. However, WorkerThreadableLoader that has a
reference to the context is still alive at the point and the assertion fails.
This CL moves the assertion to after the line to dispose the loader.

<More details>

The timing to release a reference to the context owned by the loader was changed
by this patch: https://codereview.chromium.org/1749633002

Before the patch, GlobalFetchImpl::StopDetector was an ActiveDOMObject and was
supposed to release the reference before performShutdownTask() as follows:

  WorkerThread::shutdown()
    WorkerGlobalScope::dispose()
      stopActiveDOMObjects();
        StopDetector::stop()
          FetchManager::stop()
            FetchManager::Loader::dispose()
              // Release WorkerThreadableLoader.
  WorkerThread::performShutdownTask()
    // The assertion can pass.

The patch removed StopDetector and instead made FetchManager::Loader to observe
lifecycle of the context to release the reference. The context destruction is
notified after the assertion as follows:

  WorkerThread::shutdown()
  WorkerThread::performShutdownTask()
    // The assertion cannot pass.
    WorkerGlobalScope::notifyContextDestroyed()
      LifecycleNotifier::notifyContextDestroyed()
        FetchManager::contextDestroyed()
          FetchManager::Loader::dispose()
            // Release WorkerThreadableLoader.

BUG=594230

Committed: https://crrev.com/0fdc4685d3b7e5134109b814eb08d8c265cdb84c
Cr-Commit-Position: refs/heads/master@{#383048}
==========

commit-bot: I haz the power

4 years, 9 months ago (2016-03-24 10:25:06 UTC) #13

Message was sent while issue was closed.

Patchset 1 (id:??) landed as
https://crrev.com/0fdc4685d3b7e5134109b814eb08d8c265cdb84c
Cr-Commit-Position: refs/heads/master@{#383048}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages