Send got/lostpointercapture immediately if possible

Send got/lostpointercapture immediately if possible

After calls to set/releasepointercapture queue
the process pending pointer capture task if the
capture target and pending capture target were the
same. This way it not only sends the capture event
immediately in the normal cases but also doesn't
generate to the inifinite got/lostpointercapture
event sequence.

BUG=627192
Committed: https://crrev.com/7d314cf3ad6a9f0fe163228f7a0efc5a72adc560
Cr-Commit-Position: refs/heads/master@{#406972}

[Navid Zolghadr, 2016-07-15 16:32:45]

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

[commit-bot: I haz the power, 2016-07-15 16:33:37]

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

[Navid Zolghadr, 2016-07-15 16:34:13]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-15 16:38:17]

nzolghadr@chromium.org changed reviewers:
+ mustaq@chromium.org, rbyers@chromium.org

[Navid Zolghadr, 2016-07-15 16:38:18]

I was not sure how I should pass the "this" parameter to the queued task. I used
WTF::unretained for now. Let me know if you think I should use something else.

[Rick Byers, 2016-07-15 17:35:32]

Just a few small things.
I love that the only test changes required are removing hacks from our
automation of existing WPT tests ;-).  Hopefully more and more PE CLs will look
like this (not needing any LayoutTests).

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:218:
pointerEventInit.setPointerId(pointerId);
so for now you're just matching edge's behavior for deferred events here, right?
 Can you add a TODO tracking the spec bug, just to make sure it's really what we
want (since spec doesn't currently say).

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:620: EventTarget*
pct = m_pointerCaptureTarget.contains(pointerId)
nit: HashMap::find is the efficient way to do contains/get at the same time.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:670: // Re-target
lostpointercapture to the document when the element is
is this behavior specified somewhere?  It seems reasonable to me, but if it's
not spec'd then please file a GitHub issue and reference it from a TODO here
(but obviously don't block on it).

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:681:
dispatchPointerEvent(pendingPointerCaptureTarget,
Don't you need to check for null here too - eg. for the
Element.releasePointerCapture cases where only a 'lost' event is fired without
any 'got'?

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:694: bool
PointerEventManager::processPendingPointerCapture(
once we make the other changes here (eg. not firing the boundary events) it
should be easier to merge these two similar "process pending pointer capture"
codepaths, right?  Please add a TODO saying they should be merged.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:808: // Only queue
the processing pending pointer capture if the pending
nit: Add a comment referencing the github issue where we've debated the
semantics (since it may still change).

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:812:
m_frame->document()->getExecutionContext()->postTask(
Looking at this more, I think you probably should be using the
document()->postTask - if for nothing more than to be consistent with the wide
variety of existing uses (they can all be migrated at once).

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:838: // Only queue
the processing pending pointer capture if the pending
please avoid copy/paste - share this code

[mustaq, 2016-07-15 19:35:43]

Question for Rick: it's not clear to me how posted tasks are prioritized against
other "pending" tasks. What happens in case of a janky JS handler where
setPointerCapture call is after the jank: what guarantees that the posted
processPendingCapture task goes ahead of the other PE handler calls potentially
triggered by user activities during the jank? The |postTask| doc doesn't explain
this to me. What am I missing here?

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:659: void
PointerEventManager::immediateProcessPendingPointerCapture(int pointerId)
Nit: s/immediate/immediately/

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:681:
dispatchPointerEvent(pendingPointerCaptureTarget,
On 2016/07/15 17:35:32, Rick Byers wrote:
> Don't you need to check for null here too - eg. for the
> Element.releasePointerCapture cases where only a 'lost' event is fired without
> any 'got'?

Yes, pendingPointerCaptureTarget would be null after a release call.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:737: // affect the
behavior of sendBoundaryEvents function. So the
Does that mean the boundary event firing is inconsistent between this and the
immediate processing above?

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:802: void
PointerEventManager::setPointerCapture(int pointerId, EventTarget* target)
It's not clear to me what happens in case of a janky JS handler where
setPointerCapture call is after the jank: what guarantees here that the posted
task goes ahead of the other PE handler calls triggered by user activities
during the jank?

[Rick Byers, 2016-07-15 20:13:02]

On 2016/07/15 19:35:43, mustaq wrote:
> Question for Rick: it's not clear to me how posted tasks are prioritized
against
> other "pending" tasks. What happens in case of a janky JS handler where
> setPointerCapture call is after the jank: what guarantees that the posted
> processPendingCapture task goes ahead of the other PE handler calls
potentially
> triggered by user activities during the jank? The |postTask| doc doesn't
explain
> this to me. What am I missing here?

It goes to the end of the normal priority task queue, but the blink scheduler
does allow high priority tasks (like input) to run first. But that shouldn't
matter - the other input task will trigger it's own
processPendingPointerCapture, and then this "immediate" task that was queued
will be a no-op when it runs later.

 
>
https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:659: void
> PointerEventManager::immediateProcessPendingPointerCapture(int pointerId)
> Nit: s/immediate/immediately/
> 
>
https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:681:
> dispatchPointerEvent(pendingPointerCaptureTarget,
> On 2016/07/15 17:35:32, Rick Byers wrote:
> > Don't you need to check for null here too - eg. for the
> > Element.releasePointerCapture cases where only a 'lost' event is fired
without
> > any 'got'?
> 
> Yes, pendingPointerCaptureTarget would be null after a release call.
> 
>
https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:737: // affect
the
> behavior of sendBoundaryEvents function. So the
> Does that mean the boundary event firing is inconsistent between this and the
> immediate processing above?
> 
>
https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:802: void
> PointerEventManager::setPointerCapture(int pointerId, EventTarget* target)
> It's not clear to me what happens in case of a janky JS handler where
> setPointerCapture call is after the jank: what guarantees here that the posted
> task goes ahead of the other PE handler calls triggered by user activities
> during the jank?

[Navid Zolghadr, 2016-07-15 20:37:54]

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

[commit-bot: I haz the power, 2016-07-15 20:38:23]

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

[Navid Zolghadr, 2016-07-15 20:38:32]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-15 20:39:07]

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

[commit-bot: I haz the power, 2016-07-15 20:39:48]

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

[Navid Zolghadr, 2016-07-15 20:41:01]

I believe I have applied all the comments.
One question regarding that WTF::unretained instance. I don't have a very good
feeling about that. At the same time there are many options to choose from some
of them need the object to be GC handled object which PointerEventManager is
not. Do you know who I can ask about different options and what they mean? Is
tkent@ a good person for this question?

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:218:
pointerEventInit.setPointerId(pointerId);
On 2016/07/15 17:35:31, Rick Byers wrote:
> so for now you're just matching edge's behavior for deferred events here,
right?
>  Can you add a TODO tracking the spec bug, just to make sure it's really what
we
> want (since spec doesn't currently say).

Not quite. Edge does set the attributes for the gotpointercapture but it is not
possible without caching the last pointerevent. So in that sense this does not
agree with Edge on gotpointercapture. I was going to put this in the same spec
PR as not delaying the got/lostpointercapture.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:620: EventTarget*
pct = m_pointerCaptureTarget.contains(pointerId)
On 2016/07/15 17:35:32, Rick Byers wrote:
> nit: HashMap::find is the efficient way to do contains/get at the same time.

Done.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:659: void
PointerEventManager::immediateProcessPendingPointerCapture(int pointerId)
On 2016/07/15 19:35:43, mustaq wrote:
> Nit: s/immediate/immediately/

Done.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:670: // Re-target
lostpointercapture to the document when the element is
On 2016/07/15 17:35:32, Rick Byers wrote:
> is this behavior specified somewhere?  It seems reasonable to me, but if it's
> not spec'd then please file a GitHub issue and reference it from a TODO here
> (but obviously don't block on it).

Yes. It is here the second paragraph.
https://w3c.github.io/pointerevents/#implicit-release-of-pointer-capture

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:681:
dispatchPointerEvent(pendingPointerCaptureTarget,
On 2016/07/15 17:35:32, Rick Byers wrote:
> Don't you need to check for null here too - eg. for the
> Element.releasePointerCapture cases where only a 'lost' event is fired without
> any 'got'?

Since there was a lot of those checks around every dispatchPointerEvent in this
file I just put them inside dispatchPointerEvent to avoid having it for every
one in one of the early changes. The only reason that I have it in the previous
case is because for the lostpointercapture case it wanted to get the parent not
and I wanted to make sure it is not null.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:694: bool
PointerEventManager::processPendingPointerCapture(
On 2016/07/15 17:35:31, Rick Byers wrote:
> once we make the other changes here (eg. not firing the boundary events) it
> should be easier to merge these two similar "process pending pointer capture"
> codepaths, right?  Please add a TODO saying they should be merged.

That is correct. I added the TODO.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:737: // affect the
behavior of sendBoundaryEvents function. So the
On 2016/07/15 19:35:43, mustaq wrote:
> Does that mean the boundary event firing is inconsistent between this and the
> immediate processing above?

I believe yes. But I saw none of the tests failing. I'll see if anything fails
on the bots as well. However, I'm going to get rid of the boundary events and
mouse hit testing early next week in another change and that won't be any
problem I believe.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:802: void
PointerEventManager::setPointerCapture(int pointerId, EventTarget* target)
On 2016/07/15 19:35:43, mustaq wrote:
> It's not clear to me what happens in case of a janky JS handler where
> setPointerCapture call is after the jank: what guarantees here that the posted
> task goes ahead of the other PE handler calls triggered by user activities
> during the jank?

Nothing I guess. Actually before we fire every pointerevent we make sure to run
that processPendingPointerCapture anyway through
processCaptureAndPositionOfPointerEvent. So we never miss sending
got/lostpointercapture before the next pointerevent.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:808: // Only queue
the processing pending pointer capture if the pending
On 2016/07/15 17:35:32, Rick Byers wrote:
> nit: Add a comment referencing the github issue where we've debated the
> semantics (since it may still change).

Done.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:812:
m_frame->document()->getExecutionContext()->postTask(
On 2016/07/15 17:35:31, Rick Byers wrote:
> Looking at this more, I think you probably should be using the
> document()->postTask - if for nothing more than to be consistent with the wide
> variety of existing uses (they can all be migrated at once).

Done.

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:838: // Only queue
the processing pending pointer capture if the pending
On 2016/07/15 17:35:32, Rick Byers wrote:
> please avoid copy/paste - share this code

Done.

[commit-bot: I haz the power, 2016-07-15 20:49:41]

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

[commit-bot: I haz the power, 2016-07-15 20:49:42]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mustaq, 2016-07-15 21:06:14]

After an offline discussion & a closer look, I am convinced that posted tasks
will work even for the events-during-jank scenario.

I have one comment re boundary events...

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:694: bool
PointerEventManager::processPendingPointerCapture(
On 2016/07/15 17:35:31, Rick Byers wrote:
> once we make the other changes here (eg. not firing the boundary events) it
> should be easier to merge these two similar "process pending pointer capture"
> codepaths, right?  Please add a TODO saying they should be merged.

"Not firing the boundary events": we still need to fire boundary
events here because of the consistency principles regarding entry vs
capturing states, right?
https://github.com/w3c/pointerevents/issues/61#issuecomment-218512170
(The github issue is about boundary events /during capturing/, not
/at the moment of capturing/.)

https://codereview.chromium.org/2147263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:737: // affect the
behavior of sendBoundaryEvents function. So the
On 2016/07/15 19:35:43, mustaq wrote:
> Does that mean the boundary event firing is inconsistent between this and the
> immediate processing above?

Here is the catch for not sending boundary events consistently with
got/lost events: the actual sequence of boundary events will be
determined by whether pending-pointer-capture was processed through
posted-tasks or through events fired during a jank. We need to fix it.

[Rick Byers, 2016-07-15 21:36:28]

On 2016/07/15 20:41:01, Navid Zolghadr wrote:
> I believe I have applied all the comments.
> One question regarding that WTF::unretained instance. I don't have a very good
> feeling about that. At the same time there are many options to choose from
some
> of them need the object to be GC handled object which PointerEventManager is
> not. Do you know who I can ask about different options and what they mean? Is
> tkent@ a good person for this question?

Oh I'm sorry, I forgot to reply to this.  So big picture, what you need to
protect yourself against here is having the PointerEventManager being deleted
before your task runs.  EventHandler is GC'd (with a reference held by the Page)
so I think, for example, if you had an iframe that got torn down between the
time you posted the task and the task actually got a chance to run, then you
could crash.  As a result I _think_ unretained isn't good enough here.

Really what you probably want is to have your task keep a ref on the
EventHandler so that it cannot be collected as long as your task is in the
queue.  The obvious ways to do that are:
1) move the callback method to EventHandler and have it call into the
PointerEventManager.  This seems ugly and is hopefully unnecessary
2) make PointerEventManager also garbage collected (and traced from
EventHandler).  Seems a little unfortunate to break the simple by-value
containment of PointerEventManager just for this though.

But perhaps there's some other simpler way I'm not aware of to take a ref on the
EventHandler when we're keeping a pointer to a member inside of it.  Oilpan
folks would know best - you can add oilpan-reviews or ask jbroman.

[Rick Byers, 2016-07-15 21:45:08]

On 2016/07/15 21:36:28, Rick Byers wrote:
> On 2016/07/15 20:41:01, Navid Zolghadr wrote:
> > I believe I have applied all the comments.
> > One question regarding that WTF::unretained instance. I don't have a very
good
> > feeling about that. At the same time there are many options to choose from
> some
> > of them need the object to be GC handled object which PointerEventManager is
> > not. Do you know who I can ask about different options and what they mean?
Is
> > tkent@ a good person for this question?
> 
> Oh I'm sorry, I forgot to reply to this.  So big picture, what you need to
> protect yourself against here is having the PointerEventManager being deleted
> before your task runs.  EventHandler is GC'd (with a reference held by the
Page)
> so I think, for example, if you had an iframe that got torn down between the
> time you posted the task and the task actually got a chance to run, then you
> could crash.  As a result I _think_ unretained isn't good enough here.
> 
> Really what you probably want is to have your task keep a ref on the
> EventHandler so that it cannot be collected as long as your task is in the
> queue.  The obvious ways to do that are:
> 1) move the callback method to EventHandler and have it call into the
> PointerEventManager.  This seems ugly and is hopefully unnecessary
> 2) make PointerEventManager also garbage collected (and traced from
> EventHandler).  Seems a little unfortunate to break the simple by-value
> containment of PointerEventManager just for this though.
> 
> But perhaps there's some other simpler way I'm not aware of to take a ref on
the
> EventHandler when we're keeping a pointer to a member inside of it.  Oilpan
> folks would know best - you can add oilpan-reviews or ask jbroman.

Oh, perhaps you can just include a Peristent<EventHandler> along with the task
some how.  Passing it as an argument to the function would work, but it would be
an unused argument so probably silly.  I have a feeling that if you ping someone
on oilpan-reviews@ they'll say there's no special handling for interior pointers
of GC'd objects like this (I certainly don't see anything at
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
or a quick scan of the code) and the work-around is to just make
PointerEventManager a GC'd type itself.  Note that'll be enough to keep
everything alive since you've got a Member to the Frame, and that has a Member
to the EventHandler.

[Rick Byers, 2016-07-15 21:47:43]

rbyers@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[Rick Byers, 2016-07-15 21:47:43]

LGTM (modulo the outstanding Oilpan question - adding oilpan-reviews for that).

[haraken, 2016-07-16 01:53:41]

If the task is guaranteed to run in a finite time period, you can just use
wrapPersistent(this). The task holds a strong reference to PointerEventManager.

If the task is not guaranteed to run in a finite time period, you can use
wrapWeakPersistent(this). If PointerEventManager is gone before the task runs,
the task doesn't run (i.e., it doesn't crash).

[Navid Zolghadr, 2016-07-16 03:08:47]

On 2016/07/16 01:53:41, haraken wrote:
> If the task is guaranteed to run in a finite time period, you can just use
> wrapPersistent(this). The task holds a strong reference to
PointerEventManager.
> 
> If the task is not guaranteed to run in a finite time period, you can use
> wrapWeakPersistent(this). If PointerEventManager is gone before the task runs,
> the task doesn't run (i.e., it doesn't crash).

Does wrapWeakPersistent(this) work even though PointerEventManager is not a GC
object and it is only a class field in EventHandler?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...

or should I change it to GC class and create a pointer to it in EventHandler
instead as Rick suggested?

[haraken, 2016-07-16 03:54:08]

On 2016/07/16 03:08:47, Navid Zolghadr wrote:
> On 2016/07/16 01:53:41, haraken wrote:
> > If the task is guaranteed to run in a finite time period, you can just use
> > wrapPersistent(this). The task holds a strong reference to
> PointerEventManager.
> > 
> > If the task is not guaranteed to run in a finite time period, you can use
> > wrapWeakPersistent(this). If PointerEventManager is gone before the task
runs,
> > the task doesn't run (i.e., it doesn't crash).
> 
> Does wrapWeakPersistent(this) work even though PointerEventManager is not a GC
> object and it is only a class field in EventHandler?
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...
> 
> or should I change it to GC class and create a pointer to it in EventHandler
> instead as Rick suggested?

Yes, it would be the best to make PointerEventManager GarbageCollected.

[Rick Byers, 2016-07-16 12:44:09]

Has there been any discussion of supporting Persistent interior pointers in
Oilpan (see brief discussion here
http://www.memorymanagement.org/glossary/i.html).  It seems like a bit of a
shame to have to convert embedded-by-value objects into their own GC'd
object just to get the tracing to work correctly.

Naively it seems to me like something like
PersistentInterior<EventHandler,PointerEventManager> might be easy.  Its
value would be to the PointerEventManager, but the GC handle would be for
the provided EventHandler instance.  It would be easy to validate that the
PointerEventManager pointer was somewhere inside the provided
EventHandlerInstance (and so indeed an interior pointer).

Of course, in this case there are so few of these objects I doubt there's
any real perf issues.  In a language like Java you wouldn't have any other
choice and wouldn't think twice about it.

On Jul 15, 2016 11:54 PM, <haraken@chromium.org> wrote:

> On 2016/07/16 03:08:47, Navid Zolghadr wrote:
> > On 2016/07/16 01:53:41, haraken wrote:
> > > If the task is guaranteed to run in a finite time period, you can just
> use
> > > wrapPersistent(this). The task holds a strong reference to
> > PointerEventManager.
> > >
> > > If the task is not guaranteed to run in a finite time period, you can
> use
> > > wrapWeakPersistent(this). If PointerEventManager is gone before the
> task
> runs,
> > > the task doesn't run (i.e., it doesn't crash).
> >
> > Does wrapWeakPersistent(this) work even though PointerEventManager is
> not a GC
> > object and it is only a class field in EventHandler?
> >
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...
> >
> > or should I change it to GC class and create a pointer to it in
> EventHandler
> > instead as Rick suggested?
>
> Yes, it would be the best to make PointerEventManager GarbageCollected.
>
>
> https://codereview.chromium.org/2147263003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Rick Byers, 2016-07-16 12:44:09]

Has there been any discussion of supporting Persistent interior pointers in
Oilpan (see brief discussion here
http://www.memorymanagement.org/glossary/i.html).  It seems like a bit of a
shame to have to convert embedded-by-value objects into their own GC'd
object just to get the tracing to work correctly.

Naively it seems to me like something like
PersistentInterior<EventHandler,PointerEventManager> might be easy.  Its
value would be to the PointerEventManager, but the GC handle would be for
the provided EventHandler instance.  It would be easy to validate that the
PointerEventManager pointer was somewhere inside the provided
EventHandlerInstance (and so indeed an interior pointer).

Of course, in this case there are so few of these objects I doubt there's
any real perf issues.  In a language like Java you wouldn't have any other
choice and wouldn't think twice about it.

On Jul 15, 2016 11:54 PM, <haraken@chromium.org> wrote:

> On 2016/07/16 03:08:47, Navid Zolghadr wrote:
> > On 2016/07/16 01:53:41, haraken wrote:
> > > If the task is guaranteed to run in a finite time period, you can just
> use
> > > wrapPersistent(this). The task holds a strong reference to
> > PointerEventManager.
> > >
> > > If the task is not guaranteed to run in a finite time period, you can
> use
> > > wrapWeakPersistent(this). If PointerEventManager is gone before the
> task
> runs,
> > > the task doesn't run (i.e., it doesn't crash).
> >
> > Does wrapWeakPersistent(this) work even though PointerEventManager is
> not a GC
> > object and it is only a class field in EventHandler?
> >
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...
> >
> > or should I change it to GC class and create a pointer to it in
> EventHandler
> > instead as Rick suggested?
>
> Yes, it would be the best to make PointerEventManager GarbageCollected.
>
>
> https://codereview.chromium.org/2147263003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2016-07-17 13:09:55]

On 2016/07/16 12:44:09, Rick Byers (out until Aug 1) wrote:
> Has there been any discussion of supporting Persistent interior pointers in
> Oilpan (see brief discussion here
> http://www.memorymanagement.org/glossary/i.html).  It seems like a bit of a
> shame to have to convert embedded-by-value objects into their own GC'd
> object just to get the tracing to work correctly.
> 
> Naively it seems to me like something like
> PersistentInterior<EventHandler,PointerEventManager> might be easy.  Its
> value would be to the PointerEventManager, but the GC handle would be for
> the provided EventHandler instance.  It would be easy to validate that the
> PointerEventManager pointer was somewhere inside the provided
> EventHandlerInstance (and so indeed an interior pointer).

Sorry, I'm confused. What do you want to do here?

If you just want to embed PointerEventManager in EventHandler and add a strong
reference from the EventHandler to the PointerEventManager, you can just do that
without making PointerEventManager GarbageCollected.

class EventHandler : public GarbageCollected<> {
  DEFINE_INLINE_TRACE() { visitor->trace(m_manager); }
  PointerEventManager m_manager;  // embedded by value
};

class PointerEventManager {  // not GarbageCollected
  DISALLOW_NEW();
  DEFINE_INLINE_TRACE() { ...; }
};


> 
> Of course, in this case there are so few of these objects I doubt there's
> any real perf issues.  In a language like Java you wouldn't have any other
> choice and wouldn't think twice about it.

[Rick Byers, 2016-07-17 17:35:45]

On 2016/07/17 13:09:55, haraken wrote:
> On 2016/07/16 12:44:09, Rick Byers (out until Aug 1) wrote:
> > Has there been any discussion of supporting Persistent interior pointers in
> > Oilpan (see brief discussion here
> > http://www.memorymanagement.org/glossary/i.html).  It seems like a bit of a
> > shame to have to convert embedded-by-value objects into their own GC'd
> > object just to get the tracing to work correctly.
> > 
> > Naively it seems to me like something like
> > PersistentInterior<EventHandler,PointerEventManager> might be easy.  Its
> > value would be to the PointerEventManager, but the GC handle would be for
> > the provided EventHandler instance.  It would be easy to validate that the
> > PointerEventManager pointer was somewhere inside the provided
> > EventHandlerInstance (and so indeed an interior pointer).
> 
> Sorry, I'm confused. What do you want to do here?
> 
> If you just want to embed PointerEventManager in EventHandler and add a strong
> reference from the EventHandler to the PointerEventManager, you can just do
that
> without making PointerEventManager GarbageCollected.
> 
> class EventHandler : public GarbageCollected<> {
>   DEFINE_INLINE_TRACE() { visitor->trace(m_manager); }
>   PointerEventManager m_manager;  // embedded by value
> };
> 
> class PointerEventManager {  // not GarbageCollected
>   DISALLOW_NEW();
>   DEFINE_INLINE_TRACE() { ...; }
> };

That's basically what we have today.  The problem is that we now want to invoke
a callback on PointerEventManager and need to ensure it's kept alive until the
callback runs.  So we need some Persistent to the EventHandler, but the callback
doesn't need/take an EventHandler instance, just the PointerEventManager
instance.  I.e. we want an interior pointer to the EventHandler object (the
PointerEventManager this pointer) to keep that object alive (as any interior
reference would in other garbage collection systems).

> 
> > 
> > Of course, in this case there are so few of these objects I doubt there's
> > any real perf issues.  In a language like Java you wouldn't have any other
> > choice and wouldn't think twice about it.

[haraken, 2016-07-18 12:32:11]

On 2016/07/17 17:35:45, Rick Byers (out until Aug 1) wrote:
> On 2016/07/17 13:09:55, haraken wrote:
> > On 2016/07/16 12:44:09, Rick Byers (out until Aug 1) wrote:
> > > Has there been any discussion of supporting Persistent interior pointers
in
> > > Oilpan (see brief discussion here
> > > http://www.memorymanagement.org/glossary/i.html).  It seems like a bit of
a
> > > shame to have to convert embedded-by-value objects into their own GC'd
> > > object just to get the tracing to work correctly.
> > > 
> > > Naively it seems to me like something like
> > > PersistentInterior<EventHandler,PointerEventManager> might be easy.  Its
> > > value would be to the PointerEventManager, but the GC handle would be for
> > > the provided EventHandler instance.  It would be easy to validate that the
> > > PointerEventManager pointer was somewhere inside the provided
> > > EventHandlerInstance (and so indeed an interior pointer).
> > 
> > Sorry, I'm confused. What do you want to do here?
> > 
> > If you just want to embed PointerEventManager in EventHandler and add a
strong
> > reference from the EventHandler to the PointerEventManager, you can just do
> that
> > without making PointerEventManager GarbageCollected.
> > 
> > class EventHandler : public GarbageCollected<> {
> >   DEFINE_INLINE_TRACE() { visitor->trace(m_manager); }
> >   PointerEventManager m_manager;  // embedded by value
> > };
> > 
> > class PointerEventManager {  // not GarbageCollected
> >   DISALLOW_NEW();
> >   DEFINE_INLINE_TRACE() { ...; }
> > };
> 
> That's basically what we have today.  The problem is that we now want to
invoke
> a callback on PointerEventManager and need to ensure it's kept alive until the
> callback runs.  So we need some Persistent to the EventHandler, but the
callback
> doesn't need/take an EventHandler instance, just the PointerEventManager
> instance.  I.e. we want an interior pointer to the EventHandler object (the
> PointerEventManager this pointer) to keep that object alive (as any interior
> reference would in other garbage collection systems).

Thanks, I now understand your point :) But I'm not really happy about supporting
the interior pointers for the following reasons:

- Oilpan already has a mechanism to trace interior pointers found during
conservative scanning on the machine stack. However, it is heavy because it
needs to build up a mapping to look up the object start address from any given
address. I don't want to add the overhead to normal Oilpan's GCs.

- The interior pointers add extra programming rule to Oilpan.

Overall I think it's better to just make PointerEventManager GarbageCollected.
It would be pretty straightforward.

[Navid Zolghadr, 2016-07-18 15:38:38]

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

[commit-bot: I haz the power, 2016-07-18 15:39:01]

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

[Navid Zolghadr, 2016-07-18 15:50:38]

I made PointerEventManager a GC object. ptal.

[commit-bot: I haz the power, 2016-07-18 16:13:15]

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

[commit-bot: I haz the power, 2016-07-18 16:13:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[haraken, 2016-07-18 16:47:03]

On 2016/07/18 15:50:38, Navid Zolghadr wrote:
> I made PointerEventManager a GC object. ptal.

Thanks, Oilpan part LGTM.

[Navid Zolghadr, 2016-07-18 17:59:06]

On 2016/07/18 16:47:03, haraken wrote:
> On 2016/07/18 15:50:38, Navid Zolghadr wrote:
> > I made PointerEventManager a GC object. ptal.
> 
> Thanks, Oilpan part LGTM.

haraken@ this last patch causes some crashes in some of the tests. Particularly
when EventHandler gets destroyed the crash happens I believe. I don't seem to be
able to see the problem. Does this stack trace ring any bell for you?

#0 0x7f907dfef007 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7f907cccf330 <unknown>
#2 0x7f907f7e1ed0 blink::EventHandler::~EventHandler()
#3 0x7f907ec30bf0 blink::NormalPage::sweep()
#4 0x7f907ec2f469 blink::BaseArena::completeSweep()
#5 0x7f907ec335ec blink::ThreadState::completeSweep()
#6 0x7f907ec307cc blink::NormalPageArena::outOfLineAllocate()
#7 0x7f907f977ab5 WTF::HashTable<>::allocateTable()
#8 0x7f907f9777e1 WTF::HashTable<>::expandBuffer()
#9 0x7f907f977665 WTF::HashTable<>::rehash()
#10 0x7f907f97760f
_ZN3WTF9HashTableINS_12AtomicStringENS_12KeyValuePairIS1_N5blink6MemberINS3_15HeapLinkedStackINS3_8RuleDataEEEEEEENS_24KeyValuePairKeyExtractorENS_16AtomicStringHashENS_18HashMapValueTraitsINS_10HashTraitsIS1_EENSD_IS8_EEEESE_NS3_13HeapAllocatorEE3addINS_17HashMapTranslatorISG_SB_EERKS1_DnEENS_18HashTableAddResultISI_S9_EEOT0_OT1_
#11 0x7f907f975e3d blink::RuleSet::findBestRuleSetAndAdd()
#12 0x7f907f976292 blink::RuleSet::addRule()
#13 0x7f907f9c9cf1 blink::ScopedStyleResolver::addTreeBoundaryCrossingRules()
#14 0x7f907f9c9b0f blink::ScopedStyleResolver::appendCSSStyleSheet()
#15 0x7f907f9d9b8b blink::StyleResolver::appendAuthorStyleSheets()
#16 0x7f907f7ac77d blink::StyleEngine::appendActiveAuthorStyleSheets()
#17 0x7f907f7ac8cb blink::StyleEngine::createResolver()
#18 0x7f907f7445b4 blink::Document::updateStyle()
#19 0x7f907f741755 blink::Document::updateStyleAndLayoutTree()
#20 0x7f907fa98970
blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal()
#21 0x7f907fa98bd8
blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal()
#22 0x7f907fa97cc2 blink::FrameView::updateStyleAndLayoutIfNeededRecursive()
#23 0x7f907fa97760 blink::FrameView::updateLifecyclePhasesInternal()
#24 0x7f907fd58719 blink::LayoutView::hitTest()
#25 0x7f907f7e3822 blink::EventHandler::updateCursor()
#26 0x7f9081b8a966 blink::TimerBase::runInternal()
#27 0x7f9081b8aa0e blink::TimerBase::CancellableTimerTask::run()
#28 0x7f907dc73b49
_ZN4base8internal7InvokerINS0_9BindStateIPFvSt10unique_ptrIN13safe_browsing16IncidentReceiverESt14default_deleteIS5_EEEJNS0_13PassedWrapperIS8_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
#29 0x7f907e051266 base::debug::TaskAnnotator::RunTask()
#30 0x7f9081fd1077 scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
#31 0x7f9081fd010e scheduler::TaskQueueManager::DoWork()
#32 0x7f907e051266 base::debug::TaskAnnotator::RunTask()
#33 0x7f907e006ba5 base::MessageLoop::RunTask()
#34 0x7f907e006e98 base::MessageLoop::DeferOrRunPendingTask()
#35 0x7f907e0071eb base::MessageLoop::DoWork()
#36 0x7f907e00830a base::MessagePumpDefault::Run()
#37 0x7f907e01ce1e base::RunLoop::Run()
#38 0x7f9080cf0ffa content::RendererMain()
#39 0x7f907dfc0b2d content::RunZygote()
#40 0x7f907dfc1bc7 content::ContentMainRunnerImpl::Run()
#41 0x7f907dfc0700 content::ContentMain()
#42 0x7f907dbd9c5b ChromeMain
#43 0x7f9076e4ef45 __libc_start_main

[Navid Zolghadr, 2016-07-18 18:40:37]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org

[mustaq, 2016-07-18 20:31:40]

LGTM but please file a bug for missing boundary events during jank. Whether we
fix that or not should be debated in the WG, but keep the bug around for records
for now.

https://codereview.chromium.org/2147263003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2147263003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:697: // when we
stop hit-testing while a pointer is captured.
Please add in the TODO comment that firing of boundary events /during/ capture
or release is delayed except for PEs-while-in-jank case.

This is important because the outcome of consecutive setPointerCapture calls
would be jank-dependent, would feel buggy for completely lost enter/leaves
during the jank.

[haraken, 2016-07-18 23:03:58]

On 2016/07/18 17:59:06, Navid Zolghadr wrote:
> On 2016/07/18 16:47:03, haraken wrote:
> > On 2016/07/18 15:50:38, Navid Zolghadr wrote:
> > > I made PointerEventManager a GC object. ptal.
> > 
> > Thanks, Oilpan part LGTM.
> 
> haraken@ this last patch causes some crashes in some of the tests.
Particularly
> when EventHandler gets destroyed the crash happens I believe. I don't seem to
be
> able to see the problem. Does this stack trace ring any bell for you?
> 
> #0 0x7f907dfef007 base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #1 0x7f907cccf330 <unknown>
> #2 0x7f907f7e1ed0 blink::EventHandler::~EventHandler()
> #3 0x7f907ec30bf0 blink::NormalPage::sweep()
> #4 0x7f907ec2f469 blink::BaseArena::completeSweep()
> #5 0x7f907ec335ec blink::ThreadState::completeSweep()
> #6 0x7f907ec307cc blink::NormalPageArena::outOfLineAllocate()
> #7 0x7f907f977ab5 WTF::HashTable<>::allocateTable()
> #8 0x7f907f9777e1 WTF::HashTable<>::expandBuffer()
> #9 0x7f907f977665 WTF::HashTable<>::rehash()
> #10 0x7f907f97760f
>
_ZN3WTF9HashTableINS_12AtomicStringENS_12KeyValuePairIS1_N5blink6MemberINS3_15HeapLinkedStackINS3_8RuleDataEEEEEEENS_24KeyValuePairKeyExtractorENS_16AtomicStringHashENS_18HashMapValueTraitsINS_10HashTraitsIS1_EENSD_IS8_EEEESE_NS3_13HeapAllocatorEE3addINS_17HashMapTranslatorISG_SB_EERKS1_DnEENS_18HashTableAddResultISI_S9_EEOT0_OT1_
> #11 0x7f907f975e3d blink::RuleSet::findBestRuleSetAndAdd()
> #12 0x7f907f976292 blink::RuleSet::addRule()
> #13 0x7f907f9c9cf1 blink::ScopedStyleResolver::addTreeBoundaryCrossingRules()
> #14 0x7f907f9c9b0f blink::ScopedStyleResolver::appendCSSStyleSheet()
> #15 0x7f907f9d9b8b blink::StyleResolver::appendAuthorStyleSheets()
> #16 0x7f907f7ac77d blink::StyleEngine::appendActiveAuthorStyleSheets()
> #17 0x7f907f7ac8cb blink::StyleEngine::createResolver()
> #18 0x7f907f7445b4 blink::Document::updateStyle()
> #19 0x7f907f741755 blink::Document::updateStyleAndLayoutTree()
> #20 0x7f907fa98970
> blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal()
> #21 0x7f907fa98bd8
> blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal()
> #22 0x7f907fa97cc2 blink::FrameView::updateStyleAndLayoutIfNeededRecursive()
> #23 0x7f907fa97760 blink::FrameView::updateLifecyclePhasesInternal()
> #24 0x7f907fd58719 blink::LayoutView::hitTest()
> #25 0x7f907f7e3822 blink::EventHandler::updateCursor()
> #26 0x7f9081b8a966 blink::TimerBase::runInternal()
> #27 0x7f9081b8aa0e blink::TimerBase::CancellableTimerTask::run()
> #28 0x7f907dc73b49
>
_ZN4base8internal7InvokerINS0_9BindStateIPFvSt10unique_ptrIN13safe_browsing16IncidentReceiverESt14default_deleteIS5_EEEJNS0_13PassedWrapperIS8_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
> #29 0x7f907e051266 base::debug::TaskAnnotator::RunTask()
> #30 0x7f9081fd1077 scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
> #31 0x7f9081fd010e scheduler::TaskQueueManager::DoWork()
> #32 0x7f907e051266 base::debug::TaskAnnotator::RunTask()
> #33 0x7f907e006ba5 base::MessageLoop::RunTask()
> #34 0x7f907e006e98 base::MessageLoop::DeferOrRunPendingTask()
> #35 0x7f907e0071eb base::MessageLoop::DoWork()
> #36 0x7f907e00830a base::MessagePumpDefault::Run()
> #37 0x7f907e01ce1e base::RunLoop::Run()
> #38 0x7f9080cf0ffa content::RendererMain()
> #39 0x7f907dfc0b2d content::RunZygote()
> #40 0x7f907dfc1bc7 content::ContentMainRunnerImpl::Run()
> #41 0x7f907dfc0700 content::ContentMain()
> #42 0x7f907dbd9c5b ChromeMain
> #43 0x7f9076e4ef45 __libc_start_main

Hmm, I don't see any problem either.

If the stack trace is of release builds, can you get a stack trace of debug
builds? It would be helpful to identify exactly where we're crashing.

[Navid Zolghadr, 2016-07-19 15:45:01]

On 2016/07/18 23:03:58, haraken wrote:
> On 2016/07/18 17:59:06, Navid Zolghadr wrote:
> > On 2016/07/18 16:47:03, haraken wrote:
> > > On 2016/07/18 15:50:38, Navid Zolghadr wrote:
> > > > I made PointerEventManager a GC object. ptal.
> > > 
> > > Thanks, Oilpan part LGTM.
> > 
> > haraken@ this last patch causes some crashes in some of the tests.
> Particularly
> > when EventHandler gets destroyed the crash happens I believe. I don't seem
to
> be
> > able to see the problem. Does this stack trace ring any bell for you?
> > 
> > #0 0x7f907dfef007 base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #1 0x7f907cccf330 <unknown>
> > #2 0x7f907f7e1ed0 blink::EventHandler::~EventHandler()
> > #3 0x7f907ec30bf0 blink::NormalPage::sweep()
> > #4 0x7f907ec2f469 blink::BaseArena::completeSweep()
> > #5 0x7f907ec335ec blink::ThreadState::completeSweep()
> > #6 0x7f907ec307cc blink::NormalPageArena::outOfLineAllocate()
> > #7 0x7f907f977ab5 WTF::HashTable<>::allocateTable()
> > #8 0x7f907f9777e1 WTF::HashTable<>::expandBuffer()
> > #9 0x7f907f977665 WTF::HashTable<>::rehash()
> > #10 0x7f907f97760f
> >
>
_ZN3WTF9HashTableINS_12AtomicStringENS_12KeyValuePairIS1_N5blink6MemberINS3_15HeapLinkedStackINS3_8RuleDataEEEEEEENS_24KeyValuePairKeyExtractorENS_16AtomicStringHashENS_18HashMapValueTraitsINS_10HashTraitsIS1_EENSD_IS8_EEEESE_NS3_13HeapAllocatorEE3addINS_17HashMapTranslatorISG_SB_EERKS1_DnEENS_18HashTableAddResultISI_S9_EEOT0_OT1_
> > #11 0x7f907f975e3d blink::RuleSet::findBestRuleSetAndAdd()
> > #12 0x7f907f976292 blink::RuleSet::addRule()
> > #13 0x7f907f9c9cf1
blink::ScopedStyleResolver::addTreeBoundaryCrossingRules()
> > #14 0x7f907f9c9b0f blink::ScopedStyleResolver::appendCSSStyleSheet()
> > #15 0x7f907f9d9b8b blink::StyleResolver::appendAuthorStyleSheets()
> > #16 0x7f907f7ac77d blink::StyleEngine::appendActiveAuthorStyleSheets()
> > #17 0x7f907f7ac8cb blink::StyleEngine::createResolver()
> > #18 0x7f907f7445b4 blink::Document::updateStyle()
> > #19 0x7f907f741755 blink::Document::updateStyleAndLayoutTree()
> > #20 0x7f907fa98970
> > blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal()
> > #21 0x7f907fa98bd8
> > blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal()
> > #22 0x7f907fa97cc2 blink::FrameView::updateStyleAndLayoutIfNeededRecursive()
> > #23 0x7f907fa97760 blink::FrameView::updateLifecyclePhasesInternal()
> > #24 0x7f907fd58719 blink::LayoutView::hitTest()
> > #25 0x7f907f7e3822 blink::EventHandler::updateCursor()
> > #26 0x7f9081b8a966 blink::TimerBase::runInternal()
> > #27 0x7f9081b8aa0e blink::TimerBase::CancellableTimerTask::run()
> > #28 0x7f907dc73b49
> >
>
_ZN4base8internal7InvokerINS0_9BindStateIPFvSt10unique_ptrIN13safe_browsing16IncidentReceiverESt14default_deleteIS5_EEEJNS0_13PassedWrapperIS8_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
> > #29 0x7f907e051266 base::debug::TaskAnnotator::RunTask()
> > #30 0x7f9081fd1077 scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
> > #31 0x7f9081fd010e scheduler::TaskQueueManager::DoWork()
> > #32 0x7f907e051266 base::debug::TaskAnnotator::RunTask()
> > #33 0x7f907e006ba5 base::MessageLoop::RunTask()
> > #34 0x7f907e006e98 base::MessageLoop::DeferOrRunPendingTask()
> > #35 0x7f907e0071eb base::MessageLoop::DoWork()
> > #36 0x7f907e00830a base::MessagePumpDefault::Run()
> > #37 0x7f907e01ce1e base::RunLoop::Run()
> > #38 0x7f9080cf0ffa content::RendererMain()
> > #39 0x7f907dfc0b2d content::RunZygote()
> > #40 0x7f907dfc1bc7 content::ContentMainRunnerImpl::Run()
> > #41 0x7f907dfc0700 content::ContentMain()
> > #42 0x7f907dbd9c5b ChromeMain
> > #43 0x7f9076e4ef45 __libc_start_main
> 
> Hmm, I don't see any problem either.
> 
> If the stack trace is of release builds, can you get a stack trace of debug
> builds? It would be helpful to identify exactly where we're crashing.

Using a debug build it gives the error in an ASSERT here

SSERTION FAILED: m_thread == currentThread()
../../third_party/WebKit/Source/platform/Timer.h(216) : bool
blink::TimerBase::isActive() const
1   0x7f58de7987d7
2   0x7f58df017f15 blink::EventHandler::~EventHandler()
3   0x7f58df025275
4   0x7f58df025255
5   0x7f58df025235
6   0x7f58ea8624c1 blink::HeapObjectHeader::finalize(unsigned char*, unsigned
long)
7   0x7f58ea8670c8
8   0x7f58ea863389 blink::BaseArena::sweepUnsweptPage()
9   0x7f58ea86356e blink::BaseArena::lazySweepWithDeadline(double)
10  0x7f58ea8713f9 blink::ThreadState::performIdleLazySweep(double)
11  0x7f58ea8785da
12  0x7f58ea878516
13  0x7f58ea8784a7
14  0x7f58ea8783fc
15  0x7f58ea406b54
16  0x7f58ea406a84
17  0x7f58ea40683b
18  0x7f58e3367a65
scheduler::WebSchedulerImpl::runIdleTask(std::unique_ptr<blink::WebThread::IdleTask,
std::default_delete<blink::WebThread::IdleTask> >, base::TimeTicks)
19  0x7f58e336914f
20  0x7f58e336909d
21  0x7f58e3369037
22  0x7f58e3368dfc
23  0x7f58e3366996
24  0x7f58e336635a
25  0x7f58e3366f78
26  0x7f58e3366e7f
27  0x7f58e3366df3
28  0x7f58e3366b6c
29  0x7f58f7f4a39e
30  0x7f58f7f6ff9e base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&)
31  0x7f58e3340a94
scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::WorkQueue*,
scheduler::internal::TaskQueueImpl::Task*)

[haraken, 2016-07-19 20:24:09]

> Using a debug build it gives the error in an ASSERT here
> 
> SSERTION FAILED: m_thread == currentThread()
> ../../third_party/WebKit/Source/platform/Timer.h(216) : bool
> blink::TimerBase::isActive() const
> 1   0x7f58de7987d7
> 2   0x7f58df017f15 blink::EventHandler::~EventHandler()
> 3   0x7f58df025275
> 4   0x7f58df025255
> 5   0x7f58df025235
> 6   0x7f58ea8624c1 blink::HeapObjectHeader::finalize(unsigned char*, unsigned
> long)
> 7   0x7f58ea8670c8
> 8   0x7f58ea863389 blink::BaseArena::sweepUnsweptPage()
> 9   0x7f58ea86356e blink::BaseArena::lazySweepWithDeadline(double)
> 10  0x7f58ea8713f9 blink::ThreadState::performIdleLazySweep(double)
> 11  0x7f58ea8785da
> 12  0x7f58ea878516
> 13  0x7f58ea8784a7
> 14  0x7f58ea8783fc
> 15  0x7f58ea406b54
> 16  0x7f58ea406a84
> 17  0x7f58ea40683b
> 18  0x7f58e3367a65
>
scheduler::WebSchedulerImpl::runIdleTask(std::unique_ptr<blink::WebThread::IdleTask,
> std::default_delete<blink::WebThread::IdleTask> >, base::TimeTicks)
> 19  0x7f58e336914f
> 20  0x7f58e336909d
> 21  0x7f58e3369037
> 22  0x7f58e3368dfc
> 23  0x7f58e3366996
> 24  0x7f58e336635a
> 25  0x7f58e3366f78
> 26  0x7f58e3366e7f
> 27  0x7f58e3366df3
> 28  0x7f58e3366b6c
> 29  0x7f58f7f4a39e
> 30  0x7f58f7f6ff9e base::debug::TaskAnnotator::RunTask(char const*,
> base::PendingTask const&)
> 31  0x7f58e3340a94
>
scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::WorkQueue*,
> scheduler::internal::TaskQueueImpl::Task*)

This looks strange... The assert means that you're destructing the EventHandler
object on a different thread than the thread that activated the timer.

Will this CL change any threading interaction? (I don't think so...)

[Navid Zolghadr, 2016-07-19 20:29:16]

On 2016/07/19 20:24:09, haraken wrote:
> > Using a debug build it gives the error in an ASSERT here
> > 
> > SSERTION FAILED: m_thread == currentThread()
> > ../../third_party/WebKit/Source/platform/Timer.h(216) : bool
> > blink::TimerBase::isActive() const
> > 1   0x7f58de7987d7
> > 2   0x7f58df017f15 blink::EventHandler::~EventHandler()
> > 3   0x7f58df025275
> > 4   0x7f58df025255
> > 5   0x7f58df025235
> > 6   0x7f58ea8624c1 blink::HeapObjectHeader::finalize(unsigned char*,
unsigned
> > long)
> > 7   0x7f58ea8670c8
> > 8   0x7f58ea863389 blink::BaseArena::sweepUnsweptPage()
> > 9   0x7f58ea86356e blink::BaseArena::lazySweepWithDeadline(double)
> > 10  0x7f58ea8713f9 blink::ThreadState::performIdleLazySweep(double)
> > 11  0x7f58ea8785da
> > 12  0x7f58ea878516
> > 13  0x7f58ea8784a7
> > 14  0x7f58ea8783fc
> > 15  0x7f58ea406b54
> > 16  0x7f58ea406a84
> > 17  0x7f58ea40683b
> > 18  0x7f58e3367a65
> >
>
scheduler::WebSchedulerImpl::runIdleTask(std::unique_ptr<blink::WebThread::IdleTask,
> > std::default_delete<blink::WebThread::IdleTask> >, base::TimeTicks)
> > 19  0x7f58e336914f
> > 20  0x7f58e336909d
> > 21  0x7f58e3369037
> > 22  0x7f58e3368dfc
> > 23  0x7f58e3366996
> > 24  0x7f58e336635a
> > 25  0x7f58e3366f78
> > 26  0x7f58e3366e7f
> > 27  0x7f58e3366df3
> > 28  0x7f58e3366b6c
> > 29  0x7f58f7f4a39e
> > 30  0x7f58f7f6ff9e base::debug::TaskAnnotator::RunTask(char const*,
> > base::PendingTask const&)
> > 31  0x7f58e3340a94
> >
>
scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::WorkQueue*,
> > scheduler::internal::TaskQueueImpl::Task*)
> 
> This looks strange... The assert means that you're destructing the
EventHandler
> object on a different thread than the thread that activated the timer.
> 
> Will this CL change any threading interaction? (I don't think so...)

Playing with this CL as soon as I move the PointerEventManager back to the class
member instead of creating it with new it stops crashing.
Is it possible that somehow EventHandler is created and moved to another thread
but the inner PointerEventManager pointer is not moved properly and as a result
at the time of destruction it fails?

[haraken, 2016-07-19 20:43:32]

On 2016/07/19 20:29:16, Navid Zolghadr wrote:
> On 2016/07/19 20:24:09, haraken wrote:
> > > Using a debug build it gives the error in an ASSERT here
> > > 
> > > SSERTION FAILED: m_thread == currentThread()
> > > ../../third_party/WebKit/Source/platform/Timer.h(216) : bool
> > > blink::TimerBase::isActive() const
> > > 1   0x7f58de7987d7
> > > 2   0x7f58df017f15 blink::EventHandler::~EventHandler()
> > > 3   0x7f58df025275
> > > 4   0x7f58df025255
> > > 5   0x7f58df025235
> > > 6   0x7f58ea8624c1 blink::HeapObjectHeader::finalize(unsigned char*,
> unsigned
> > > long)
> > > 7   0x7f58ea8670c8
> > > 8   0x7f58ea863389 blink::BaseArena::sweepUnsweptPage()
> > > 9   0x7f58ea86356e blink::BaseArena::lazySweepWithDeadline(double)
> > > 10  0x7f58ea8713f9 blink::ThreadState::performIdleLazySweep(double)
> > > 11  0x7f58ea8785da
> > > 12  0x7f58ea878516
> > > 13  0x7f58ea8784a7
> > > 14  0x7f58ea8783fc
> > > 15  0x7f58ea406b54
> > > 16  0x7f58ea406a84
> > > 17  0x7f58ea40683b
> > > 18  0x7f58e3367a65
> > >
> >
>
scheduler::WebSchedulerImpl::runIdleTask(std::unique_ptr<blink::WebThread::IdleTask,
> > > std::default_delete<blink::WebThread::IdleTask> >, base::TimeTicks)
> > > 19  0x7f58e336914f
> > > 20  0x7f58e336909d
> > > 21  0x7f58e3369037
> > > 22  0x7f58e3368dfc
> > > 23  0x7f58e3366996
> > > 24  0x7f58e336635a
> > > 25  0x7f58e3366f78
> > > 26  0x7f58e3366e7f
> > > 27  0x7f58e3366df3
> > > 28  0x7f58e3366b6c
> > > 29  0x7f58f7f4a39e
> > > 30  0x7f58f7f6ff9e base::debug::TaskAnnotator::RunTask(char const*,
> > > base::PendingTask const&)
> > > 31  0x7f58e3340a94
> > >
> >
>
scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::WorkQueue*,
> > > scheduler::internal::TaskQueueImpl::Task*)
> > 
> > This looks strange... The assert means that you're destructing the
> EventHandler
> > object on a different thread than the thread that activated the timer.
> > 
> > Will this CL change any threading interaction? (I don't think so...)
> 
> Playing with this CL as soon as I move the PointerEventManager back to the
class
> member instead of creating it with new it stops crashing.
> Is it possible that somehow EventHandler is created and moved to another
thread
> but the inner PointerEventManager pointer is not moved properly and as a
result
> at the time of destruction it fails?

I don't think so. Oilpan's object is guaranteed to get destructed on the thread
that has created the object.

Would you investigate what threads are causing the issue? i.e., what are
m_thread and currentThread() pointing to when you hit the crash?

[Navid Zolghadr, 2016-07-20 19:59:23]

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

[commit-bot: I haz the power, 2016-07-20 19:59:54]

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

[Navid Zolghadr, 2016-07-20 20:00:10]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-20 20:00:30]

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

[Navid Zolghadr, 2016-07-20 20:00:42]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-21 12:20:13]

ptal.
Since we wanted this change in sooner, for now I just put a hack by making a
public API in PointerEventManager and EventHandler and pass the EventHandler
instance to postTask and keep PointerEventManager as is. I filed a separate bug
to make PointerEventManager a GC managed object.

[haraken, 2016-07-21 13:41:18]

On 2016/07/21 12:20:13, Navid Zolghadr wrote:
> ptal.
> Since we wanted this change in sooner, for now I just put a hack by making a
> public API in PointerEventManager and EventHandler and pass the EventHandler
> instance to postTask and keep PointerEventManager as is. I filed a separate
bug
> to make PointerEventManager a GC managed object.

I don't think that is a right direction since it would mean that you're hiding
the bug.

How urgent do you need to land this change? Did you check what m_thread and
currentThread() are pointing to?

[mustaq, 2016-07-21 14:31:19]

On 2016/07/21 13:41:18, haraken wrote:
> On 2016/07/21 12:20:13, Navid Zolghadr wrote:
> > ptal.
> > Since we wanted this change in sooner, for now I just put a hack by making a
> > public API in PointerEventManager and EventHandler and pass the EventHandler
> > instance to postTask and keep PointerEventManager as is. I filed a separate
> bug
> > to make PointerEventManager a GC managed object.
> 
> I don't think that is a right direction since it would mean that you're hiding
> the bug.
> 
> How urgent do you need to land this change? Did you check what m_thread and
> currentThread() are pointing to?

Our goal is to have a Canary build ready before our hackathon with MS
next week. So, yes, we would like to have it landed by today asap. In the
current form, this CL avoids an easy-to-repro crash. That's all we need at this
point to go on.

@haraken: what if we just let it land now, and then following a deeper
investigation of the memory bug, we land a perfect/better fix for that?

[haraken, 2016-07-21 15:08:08]

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

[haraken, 2016-07-21 15:08:09]

I'm not really happy about landing hacky things to ToT without investigating the
real issue, but LGTM if you promise you fix the issue in a reasonable timeline.

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:623: EventTarget*
pct = (it != m_pointerCaptureTarget.end()) ? it->value : nullptr;

pct => target

Blink prefers fully-qualified variable names. The same comment for other parts
in this CL.

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:74: // events.

Why is it okay to not dispatch boundary events?

[Navid Zolghadr, 2016-07-21 19:12:18]

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

[commit-bot: I haz the power, 2016-07-21 19:12:51]

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

[Navid Zolghadr, 2016-07-21 19:17:22]

I applied all the comments.
haraken@ I will work on that GC issue next week after the Hackathon. One quick
question regarding those m_thread and current one for the issue I'm going to
work on next week. They are both from ThreadIdentifier type which is typedef to
int something. So you want me just to print those ints for you or do we have
some sort of names or something for the threads running and what API should I
use to get those attributes?

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:623: EventTarget*
pct = (it != m_pointerCaptureTarget.end()) ? it->value : nullptr;
On 2016/07/21 15:08:09, haraken wrote:
> 
> pct => target
> 
> Blink prefers fully-qualified variable names. The same comment for other parts
> in this CL.

Done.

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:74: // events.
On 2016/07/21 15:08:09, haraken wrote:
> 
> Why is it okay to not dispatch boundary events?

There was this discussion to change the way we send boundary eventssent while a
pointer is captured in the pointer event working group. As the result when
sending a got/lostpointercatpure we don't need to send any boundary events and
just by changing the hittested node of the events the boundary events will be
sent via some other flow.

[haraken, 2016-07-21 19:23:29]

On 2016/07/21 19:17:22, Navid Zolghadr wrote:
> I applied all the comments.
> haraken@ I will work on that GC issue next week after the Hackathon. One quick
> question regarding those m_thread and current one for the issue I'm going to
> work on next week. They are both from ThreadIdentifier type which is typedef
to
> int something. So you want me just to print those ints for you or do we have
> some sort of names or something for the threads running and what API should I
> use to get those attributes?

I want you to identify what thread the integer values are pointing to and debug
what's going on around the threads.

> 
>
https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:623: EventTarget*
> pct = (it != m_pointerCaptureTarget.end()) ? it->value : nullptr;
> On 2016/07/21 15:08:09, haraken wrote:
> > 
> > pct => target
> > 
> > Blink prefers fully-qualified variable names. The same comment for other
parts
> > in this CL.
> 
> Done.
> 
>
https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/PointerEventManager.h (right):
> 
>
https://codereview.chromium.org/2147263003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.h:74: // events.
> On 2016/07/21 15:08:09, haraken wrote:
> > 
> > Why is it okay to not dispatch boundary events?
> 
> There was this discussion to change the way we send boundary eventssent while
a
> pointer is captured in the pointer event working group. As the result when
> sending a got/lostpointercatpure we don't need to send any boundary events and
> just by changing the hittested node of the events the boundary events will be
> sent via some other flow.

[commit-bot: I haz the power, 2016-07-21 19:44:17]

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

[commit-bot: I haz the power, 2016-07-21 19:44:18]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Navid Zolghadr, 2016-07-21 19:46:25]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-21 19:46:26]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
mustaq@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2147263003/#ps100001
(title: "Apply comments")

[commit-bot: I haz the power, 2016-07-21 19:46:50]

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-21 20:01:11]

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

[commit-bot: I haz the power, 2016-07-21 20:01:16]

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

[Navid Zolghadr, 2016-07-21 20:52:48]

The CQ bit was checked by nzolghadr@chromium.org

[commit-bot: I haz the power, 2016-07-21 20:53:14]

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-21 22:42:37]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-21 22:43:56]

Description was changed from

==========
Send got/lostpointercapture immediately if possible

After calls to set/releasepointercapture queue
the process pending pointer capture task if the
capture target and pending capture target were the
same. This way it not only sends the capture event
immediately in the normal cases but also doesn't
generate to the inifinite got/lostpointercapture
event sequence.

BUG=627192
==========

to

==========
Send got/lostpointercapture immediately if possible

After calls to set/releasepointercapture queue
the process pending pointer capture task if the
capture target and pending capture target were the
same. This way it not only sends the capture event
immediately in the normal cases but also doesn't
generate to the inifinite got/lostpointercapture
event sequence.

BUG=627192

Committed: https://crrev.com/7d314cf3ad6a9f0fe163228f7a0efc5a72adc560
Cr-Commit-Position: refs/heads/master@{#406972}
==========

[commit-bot: I haz the power, 2016-07-21 22:43:58]

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