webgl: Replace query task observer with a posted task

webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/fcfd66a5660a6ec631facdc050e8f8fc3ded7858
Cr-Commit-Position: refs/heads/master@{#420975}

[Sami, 2016-09-15 11:24:39]

Description was changed from

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
==========

to

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Sami, 2016-09-15 11:27:39]

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

[commit-bot: I haz the power, 2016-09-15 11:27:51]

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

[Sami, 2016-09-15 12:06:57]

Description was changed from

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Sami, 2016-09-15 12:06:57]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org

[Sami, 2016-09-15 12:07:17]

skyostil@chromium.org changed reviewers:
- alexclarke@chromium.org

[Sami, 2016-09-15 12:24:00]

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

[commit-bot: I haz the power, 2016-09-15 12:24:16]

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

[Sami, 2016-09-15 13:46:41]

skyostil@chromium.org changed reviewers:
+ kbr@chromium.org

[commit-bot: I haz the power, 2016-09-15 15:40:28]

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

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

Dry run: This issue passed the CQ dry run.

[vmiura, 2016-09-16 20:01:22]

Would we not also require the pre-finalizer to revoke m_weakFactory pointers?

[Ken Russell (switch to Gerrit), 2016-09-16 23:12:39]

> Would we not also require the pre-finalizer to revoke m_weakFactory
> pointers?

Good point Victor -- I think so. Comment inline. Appreciate feedback from
haraken@ and sigbjornf@.

https://codereview.chromium.org/2341043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLQuery.cpp (right):

https://codereview.chromium.org/2341043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLQuery.cpp:32: , m_weakFactory(this)
As vmiura@ points out, I think the use of WeakPtrs pointing to this Oilpan-GC'd
type introduces the same basic problem as before -- that there is a
non-Oilpan-aware pointer to this GC'd object. (base::WeakPtrFactory and
consequently WTF::WeakPtrFactory doesn't know about Oilpan's GC.) There was an
attempt in the earlier code to unregister the task observer in the destructor,
but as we discovered, that violated some of Oilpan's invariants, since the
destructor wasn't called before poisoning the page the object lived on.

I think the safe way to handle this would be to have a separate, C-heap
allocated object which contains a WeakMember pointing back to this one, and use
that object in the postTask call. I'm not sure how the lifetime of that object
should be managed though. What do you think?

Alternatively, as Victor points out, we could cause these objects to be eagerly
finalized, but apparently that adds overhead so perhaps we should find another
solution.

[haraken, 2016-09-16 23:50:07]

On 2016/09/16 23:12:39, Ken Russell wrote:
> > Would we not also require the pre-finalizer to revoke m_weakFactory
> > pointers?
> 
> Good point Victor -- I think so. Comment inline. Appreciate feedback from
> haraken@ and sigbjornf@.
> 
>
https://codereview.chromium.org/2341043002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGLQuery.cpp (right):
> 
>
https://codereview.chromium.org/2341043002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGLQuery.cpp:32: ,
m_weakFactory(this)
> As vmiura@ points out, I think the use of WeakPtrs pointing to this
Oilpan-GC'd
> type introduces the same basic problem as before -- that there is a
> non-Oilpan-aware pointer to this GC'd object. (base::WeakPtrFactory and
> consequently WTF::WeakPtrFactory doesn't know about Oilpan's GC.) There was an
> attempt in the earlier code to unregister the task observer in the destructor,
> but as we discovered, that violated some of Oilpan's invariants, since the
> destructor wasn't called before poisoning the page the object lived on.
> 
> I think the safe way to handle this would be to have a separate, C-heap
> allocated object which contains a WeakMember pointing back to this one, and
use
> that object in the postTask call. I'm not sure how the lifetime of that object
> should be managed though. What do you think?
> 
> Alternatively, as Victor points out, we could cause these objects to be
eagerly
> finalized, but apparently that adds overhead so perhaps we should find another
> solution.

You're right. We're not allowed to use a WeakPtr to a GCed object (unless you're
understanding what you're doing). Also you're correct that a WeakMember is more
lightweight than a pre-finalizer.

I'd suggest using CancellableTaskFactory (instead of directly using
WeakPtrFactory), which will take care of the lifetime complexity of the GCed
object.

[Sami, 2016-09-22 10:40:22]

On 2016/09/16 23:50:07, haraken wrote:
> I'd suggest using CancellableTaskFactory (instead of directly using
> WeakPtrFactory), which will take care of the lifetime complexity of the GCed
> object.

I thought the plan was to remove CancellableTaskFactory in favor of just using
weak pointers for cancellation just like in the rest of the codebase? Is there a
way we could make that work with GC'd objects too?

If not, I think we should try to add a presubmit check for things like this
since its very easy to miss in a review.

[Sami, 2016-09-23 09:59:45]

On 2016/09/22 10:40:22, Sami wrote:
> On 2016/09/16 23:50:07, haraken wrote:
> > I'd suggest using CancellableTaskFactory (instead of directly using
> > WeakPtrFactory), which will take care of the lifetime complexity of the GCed
> > object.
> 
> I thought the plan was to remove CancellableTaskFactory in favor of just using
> weak pointers for cancellation just like in the rest of the codebase? Is there
a
> way we could make that work with GC'd objects too?
> 
> If not, I think we should try to add a presubmit check for things like this
> since its very easy to miss in a review.

Or, is the bug that we poison the memory too soon before running finalizers so
ASAN complains but actually everything is fine?

[Sami, 2016-09-23 10:00:01]

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

[Sami, 2016-09-23 10:00:12]

Oops, +haraken@ for real.

[Ken Russell (switch to Gerrit), 2016-09-23 18:48:37]

On 2016/09/23 09:59:45, Sami wrote:
> On 2016/09/22 10:40:22, Sami wrote:
> > On 2016/09/16 23:50:07, haraken wrote:
> > > I'd suggest using CancellableTaskFactory (instead of directly using
> > > WeakPtrFactory), which will take care of the lifetime complexity of the
GCed
> > > object.
> > 
> > I thought the plan was to remove CancellableTaskFactory in favor of just
using
> > weak pointers for cancellation just like in the rest of the codebase? Is
there
> a
> > way we could make that work with GC'd objects too?
> > 
> > If not, I think we should try to add a presubmit check for things like this
> > since its very easy to miss in a review.
> 
> Or, is the bug that we poison the memory too soon before running finalizers so
> ASAN complains but actually everything is fine?

vmiura@ educated me about this after a recent conversation with haraken@.
Apparently when an Oilpan object with a finalizer becomes unreferenced, its
memory is immediately poisoned via ASAN. The finalizers are actually run lazily.
Just before calling the destructor, the object's memory is unpoisoned, the
destructor is called, and the memory is poisoned again.

The "pre-finalizer" is a workaround for this, but apparently pre-finalizers are
expensive.

Basically, the invariant is that if an Oilpan object has a finalizer, and any
raw pointers to that object are ever obtained (which would be cleaned up in the
finalizer), it's actually necessary to use a pre-finalizer -- or something else.
(This should be added to the Oilpan documentation. Filed http://crbug.com/649773
about doing so.)

CancellableTaskFactory uses the existing bookkeeping mechanisms (weak persistent
Oilpan handles) to ensure that if the task is GC'd while it's posted, it doesn't
cause a crash.

I'd like to take haraken's advice and use CancellableTaskFactory for this
purpose, at least for the moment. If there is a plan to remove it then this
additional usage can be cleaned up when it is taken out.

[vmiura, 2016-09-23 20:42:48]

> Or, is the bug that we poison the memory too soon before running finalizers so
> ASAN complains but actually everything is fine?

Technically it could be fine in this case, but I think that ASAN poisoning all
objects
that are going to be finalized helps catch certain errors, such as touching
other Member
pointers in finalizers, and other use-after-mark-for-deletion bugs.

[haraken, 2016-09-25 23:17:59]

On 2016/09/23 09:59:45, Sami wrote:
> On 2016/09/22 10:40:22, Sami wrote:
> > On 2016/09/16 23:50:07, haraken wrote:
> > > I'd suggest using CancellableTaskFactory (instead of directly using
> > > WeakPtrFactory), which will take care of the lifetime complexity of the
GCed
> > > object.
> > 
> > I thought the plan was to remove CancellableTaskFactory in favor of just
using
> > weak pointers for cancellation just like in the rest of the codebase? Is
there
> a
> > way we could make that work with GC'd objects too?
> > 
> > If not, I think we should try to add a presubmit check for things like this
> > since its very easy to miss in a review.
> 
> Or, is the bug that we poison the memory too soon before running finalizers so
> ASAN complains but actually everything is fine?

As vmiura@ pointed out, ASAN is doing a right verification.

It's hard to detect all of these errors statically in a presubmit check, so
we're doing the verification with ASAN at runtime.

> > I thought the plan was to remove CancellableTaskFactory in favor of just
using
> > weak pointers for cancellation just like in the rest of the codebase? Is
there
> a
> > way we could make that work with GC'd objects too?

As I've been repeatedly saying in the other threads, we cannot use weak pointers
directly. We need some wrapper to encapsulate the lifetime logic for GCed
object.

CancellableTaskFactory is not nice, so we need to improve it or replace it with
something better. That's why I've been trying to push the discussion forward,
but we haven't reached consensus on it. Currently CancellableTaskFactory is the
only way to do that.

[Sami, 2016-09-26 17:25:22]

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

[Sami, 2016-09-26 17:26:05]

On 2016/09/25 23:17:59, haraken wrote:
> On 2016/09/23 09:59:45, Sami wrote:
> > On 2016/09/22 10:40:22, Sami wrote:
> > > On 2016/09/16 23:50:07, haraken wrote:
> > > > I'd suggest using CancellableTaskFactory (instead of directly using
> > > > WeakPtrFactory), which will take care of the lifetime complexity of the
> GCed
> > > > object.
> > > 
> > > I thought the plan was to remove CancellableTaskFactory in favor of just
> using
> > > weak pointers for cancellation just like in the rest of the codebase? Is
> there
> > a
> > > way we could make that work with GC'd objects too?
> > > 
> > > If not, I think we should try to add a presubmit check for things like
this
> > > since its very easy to miss in a review.
> > 
> > Or, is the bug that we poison the memory too soon before running finalizers
so
> > ASAN complains but actually everything is fine?
> 
> As vmiura@ pointed out, ASAN is doing a right verification.
> 
> It's hard to detect all of these errors statically in a presubmit check, so
> we're doing the verification with ASAN at runtime.
> 
> > > I thought the plan was to remove CancellableTaskFactory in favor of just
> using
> > > weak pointers for cancellation just like in the rest of the codebase? Is
> there
> > a
> > > way we could make that work with GC'd objects too?
> 
> As I've been repeatedly saying in the other threads, we cannot use weak
pointers
> directly. We need some wrapper to encapsulate the lifetime logic for GCed
> object.
> 
> CancellableTaskFactory is not nice, so we need to improve it or replace it
with
> something better. That's why I've been trying to push the discussion forward,
> but we haven't reached consensus on it. Currently CancellableTaskFactory is
the
> only way to do that.

Okay, thanks for the clarification -- CancellableTaskFactory it is! PTAL.

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

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-26 17:47:38]

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

[commit-bot: I haz the power, 2016-09-26 17:47:39]

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

[Ken Russell (switch to Gerrit), 2016-09-26 17:48:23]

Thanks Sami for fixing this. LGTM

[Sami, 2016-09-26 17:52:42]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-09-26 17:53:21]

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-26 20:17:41]

Description was changed from

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-09-26 20:17:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-26 20:21:20]

Description was changed from

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
webgl: Replace query task observer with a posted task

This patch replaces the task observers used by WebGL queries (timer and
regular) with an explicitly posted task. In general task observers
should be used sparingly because they add overhead to every task and
their lifetime must be explicitly managed.

We now post a regular task, which runs only after the current task has
finished, meaning it is safe to update the cached query availability
flag again.

BUG=644575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-09-26 20:21:23]

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