1117712 - [Flame][Calendar]Tap an All day event in Week view,it can't enter event detail view.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Elie]

[1.Description]:
[Flame][v2.2][Calendar] If there are 2 all day events for a day, tap one All day event in Week view, it can't enter event detail view. 
Attachment:logcat.txt and  VIDEO0203[1].mp4
Happen time: 11:15

[2.Testing Steps]: 
Precondition：Create 2 all day events in a day
1.Launch Calendar
2.Switch to Week view
3.Tap the all day event, with two All day event added to this day. 

[3.Expected Result]: 
3.It should enter all day event detail view.

[4.Actual Result]: 
3.There is no response, it can‘t enter the event detail view. Then if I tap the all day event twice, it will enter New event view.

[5.Reproduction build]: 

Flame 2.2 build:
Gaia-Rev        698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/57e4e9c33bef
Build-ID        20150103010205
Version         37.0a1

[6.Reproduction Frequency]: 
Always Recurrence,5/5
TCID: Free Test

[7.Other note]: If there are 2 all day events for a day, enter Day view, tap the all day event, it can't enter event detail view.

Comment 1

[Elie]

Attachment: VIDEO0203[1].mp4

Comment 2

[Elie]

Attachment: logcat.txt

Comment 3

[Eric Chang [:ericcc] [:echang]]

Hi Gareth, We have another all day event bugs, with 2 events in a day, there is no way to view details in day view or week view, thanks..

Comment 4

[Miller Medeiros [:millermedeiros]]

this bug seems related to Panning and Zooming, if I replace the `overflow-y:scroll` with `overflow-y:hidden` on `.md__allday-events` click works correctly.

the event is just a simple `<a href="/event/show/${eventId}">`.. markup looks correct and triggering the click with `el.click()` works as expected, so it's really an issue with the "hit area".

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Can you check if it still happens with the pref "layout.event-regions.enabled" set to false? We enabled this pref a couple of days ago on B2G and it affects the hit-testing code, so it might be the cause of this bug.

Comment 6

[Miller Medeiros [:millermedeiros]]

Kartikaya, you are right. If I set "layout.event-regions.enabled" to false and restart the calendar app the problem disappears.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ok, thanks, I'll investigate. Leaving ni on me for now.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Calendar console log

When I start the calendar app on my local build (recent master) I just get black in the app area. The status bar is still visible at the top but everything else is just black. I'm attaching the console output from WebIDE. Miller, do you know what's going on here?

Comment 9

[Miller Medeiros [:millermedeiros]]

talked with :kats on IRC and was able to run the app. created a new bug for the "black screen" (Bug 1118438)

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Ok, I'm able to reproduce the problem as described in comment 0. Investigating...

Comment 11

[Kartikaya Gupta (email:[e-mail])]

The problem appears to be that when we have a dispatch-to-content region, and TabChild changes the target APZC, the input events in the queue (and any more incoming input events for the same input block) are transformed in to the coordinate space of the original APZC. This codepath only gets triggered when event regions is enabled. I'm working on a fix.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Refactor some code

Need to reuse these hunks in part 2 so moving them someplace reusable.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Retransform inputs if the confirmed target != tentative target apzc

Includes a gtest that replicates the layer structure from the calendar app.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Also: I wrote these patches on top of my local queue which includes bug 1109873 and bug 1118784. I didn't test these patches without those applied, although in theory it should still work and fix the problem since they are relatively orthogonal.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8545310 [details] [diff] [review]
Part 2 - Retransform inputs if the confirmed target != tentative target apzc

Review of attachment 8545310 [details] [diff] [review]:
-----------------------------------------------------------------

Proposal (would save a kitten):

   - Store a Matrix4x4 (rather than a Maybe<Matrix4x4>) in InputBlockState.
     Always apply it when handling the input block.

   - When creating an input block for an initial target APZC, store that
     APZC's transform in the input block rather than applying it right
     away. Since we cache the transform and apply the same transform to
     each event in the block anyways, this should be fine.

   - There are a couple of call sites of APZ::HandleInputEvent() which
     are not in InputBlockState::HandleEvents(), but it should be
     straightforward to get the matrix to those sites and apply it
     there too.

Thoughts?

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #15)
> Comment on attachment 8545310 [details] [diff] [review]
> Part 2 - Retransform inputs if the confirmed target != tentative target apzc
> 
> Review of attachment 8545310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Proposal (would save a kitten):
> 
>    - Store a Matrix4x4 (rather than a Maybe<Matrix4x4>) in InputBlockState.
>      Always apply it when handling the input block.
> 
>    - When creating an input block for an initial target APZC, store that
>      APZC's transform in the input block rather than applying it right
>      away. Since we cache the transform and apply the same transform to
>      each event in the block anyways, this should be fine.
> 
>    - There are a couple of call sites of APZ::HandleInputEvent() which
>      are not in InputBlockState::HandleEvents(), but it should be
>      straightforward to get the matrix to those sites and apply it
>      there too.
> 
> Thoughts?

I did originally want to do it this way but it seemed kinda complicated to always find a transform to pass to the input block constructor. I'll try that approach again and see if there are specific places where it's really ugly/hard to deal with.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 (alternate version)

Here's the alternate version of part 2. The things I don't like about this one are that:

(1) we end up passing around Matrix4x4() a bunch in cases where it's not going to be used (for example every ReceiveInputEvent call on a touchmove in the middle of an input block). It just seems conceptually more correct to not have to pass it around, or to use a special value (i.e. Nothing()) when we know it's not going to be used.

(2) There are places where we have to read the transform back from the input block, such as in InjectNewTouchBlock and in MainThreadTimeout. This seems a little icky to me.

So overall I still prefer the original Part 2 but if you strongly prefer this one I can live with it.

Comment 18

[Botond Ballo [:botond]]

I implemented my suggestion in a slightly different way which avoids the wrinkles you mention in comment 17 - there's no "readback", and practically no passing around.

The trick is to realize that an APZC knows its tree manager, and therefore its screen-to-local transform. We still have to cache the transform (as opposed to, say, always computing and applying it in APZC::HandleInputEvent()) because we want the same transform to be used for all events in a block, but we can avoid passing it around.

Comment 19

[Botond Ballo [:botond]]

Attachment: Part 2a - Introduce AsyncPanZoomController::GetTransformToThis()

This introduces a mechanism to query an APZC for its screen-to-local transform.

It also facilitates nice code reuse in a couple of existing APZC functions.

Comment 20

[Botond Ballo [:botond]]

Attachment: Part 2b - Make TransformToLocal() a virtual function in InputData

This isn't strictly necessary, I just did it because calling TransformToLocal() in a separate place for every derived class annoyed me.

The price I paid was making the mLocal* fields mutable, because I couldn't make a copy of the InputData itself (because it's a base class that would be sliced), and so I had to be able to call TransformToLocal() on a 'const InputData&'.

I justify this to myself as follows: the mLocal* fields have a meaning that's specific to an APZC, and are only used by that APZC during a HandleInputEvent() call, so we can think of them as "scratch space" for APZC use rather than part of the event's state.

That said, if you don't like this, there are alternatives:

  1) Change the call hierarchy of APZC::HandleInputEvent() to pass
     the InputData by non-const reference.

  2) Scrap the virtual function and go back to calling TransformToLocal()
     for each derived class separately, as you do in your patch.

Comment 21

[Botond Ballo [:botond]]

Attachment: Part 2c - Defer application of screen-to-local transform matrix until AsyncPanZoomController::HandleInputEvent()

This patch does the legwork. It's much like your patch, minus the passing around.

I took the CopyPropertiesFrom() bit from your original patch to make sure that when we're initializing a touch block based on another one, the new block uses the old block's target's transform *at the time it was cached*, rather than at the time of the copy. I'm not sure how important this is - if it's not important, we can scrap it.

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 2d - Update the screen-to-local transform matrix for an input block if the target APZC changes

This is the actual fix - a one-liner with all the legwork in place :)

Comment 23

[Botond Ballo [:botond]]

Try push with Part 1 + Part2,a-d: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=13e700b6e048

Comment 24

[Kartikaya Gupta (email:[e-mail])]

I had considered this approach as well but initially decided against it because I didn't want to be calling back into the APZCTM from the InputBlock constructor. That seemed bad to me because it's a circular call stack (APZCTM -> InputQueue -> InputBlockState -> APZC -> APZCTM) which in general I like to avoid. Now that you've implemented it, it doesn't seem so bad but I'm still concerned about the locking requirements. Specifically the InputBlockState constructor requires access to the treelock and so you can't hold the APZC lock anywhere up the stack. This makes code like [1] a little risky because you can't do it while holding the APZC lock (we don't there, but it's not enforced).

That's my main concern with this approach; I also think part b is a misuse of mutable but like you said there are other ways to deal with that so it's not a fundamental problem.

I'll think about this some more and see if I can come up with a strong argument either way.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=e92ec65eb66b#1559

Comment 25

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> Specifically the
> InputBlockState constructor requires access to the treelock and so you can't
> hold the APZC lock anywhere up the stack. This makes code like [1] a little
> risky because you can't do it while holding the APZC lock (we don't there,
> but it's not enforced).

AsyncPanZoomController::GetApzcTreeManager() does call mMonitor.AssertNotCurrentThreadIn() [1]. Granted, that's currently unimplemented [2], but there seem to be efforts underway to implement it (see bug 1027772 and bug 1027818). In the meantime, we could document the InputBlockState constructor as acquiring the tree lock.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=8f7ec061db0f#2842
[2] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/ReentrantMonitor.h?rev=ea62f34c430d#138

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8546955 [details] [diff] [review]
Part 2b - Make TransformToLocal() a virtual function in InputData

Review of attachment 8546955 [details] [diff] [review]:
-----------------------------------------------------------------

I would rather not make the fields mutable. Either of the alternatives in comment 20 is fine by me.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8546956 [details] [diff] [review]
Part 2c - Defer application of screen-to-local transform matrix until AsyncPanZoomController::HandleInputEvent()

Review of attachment 8546956 [details] [diff] [review]:
-----------------------------------------------------------------

I couldn't come up with any actual arguments against this other than my gut feeling so let's go with it for now and see how things turn out. Would like to see an updated version based on the new part b and with the MaybeHandleCurrentBlock thing (see below) fixed.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +573,5 @@
>                                                              &hitResult);
>        if (apzc) {
>          MOZ_ASSERT(hitResult == ApzcHitRegion || hitResult == ApzcContentRegion);
>  
>          transformToApzc = GetScreenToApzcTransform(apzc);

Would be nicer to get rid of the transformToApzc temp var and call GetScreenToApzcTransform directly in the transformToGecko definition (same for the other hunks in this function).

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +305,3 @@
>  {
>    while (HasEvents()) {
> +    GetTargetApzc()->HandleInputEvent(RemoveFirstEvent(), mTransformToApzc);

Inline RemoveFirstEvent() here for consistency with the WheelBlockState code. I did that in my version too since I felt it was a little cleaner and RemoveFirstEvent doesn't provide much value by being separate.

::: gfx/layers/apz/src/InputBlockState.h
@@ +120,2 @@
>     */
> +  virtual void HandleEvents() = 0;

Thanks for taking out the unnecessary arg here :)

::: gfx/layers/apz/src/InputQueue.cpp
@@ +62,5 @@
>          aTarget.get(), block->IsDefaultPrevented());
>      if (!aTarget || block->IsDefaultPrevented()) {
>        return true;
>      }
> +    aTarget->HandleInputEvent(aEvent, aTarget->GetTransformToThis());

hm.. i think the transform passed in here should be the one from the block rather than a fresh GetTransformToThis() (which may not be the same as it was at the start of the input block). Looks like I made the same mistake in my "alternate version" patch. You can add a function to CancelableStateBlock like |void DispatchImmediate(InputData&)| which asserts !HasEvents() and then calls HandleInputEvent using mTransformToApzc.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> I couldn't come up with any actual arguments against this other than my gut
> feeling so let's go with it for now and see how things turn out. Would like
> to see an updated version based on the new part b and with the
> MaybeHandleCurrentBlock thing (see below) fixed.

I just realized that any arguments I had against this approach are completely destroyed by the fact that we do exactly the same thing for building the handoff chain. i.e. InputBlockState constructor calls APZC::BuildOverscrollHandoffChain which acquires the tree lock and calls into APZCTM. So doing it for the transform doesn't make the code any worse than it was before.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8545310 [details] [diff] [review]
Part 2 - Retransform inputs if the confirmed target != tentative target apzc

Obsoleted by botond's patches

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Feel free also to fold part 1 into the new version of part 2b, or however you want to split up the patches.

Comment 31

[Botond Ballo [:botond]]

Attachment: Part 2c - Defer application of screen-to-local transform matrix until AsyncPanZoomController::HandleInputEvent()

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> Comment on attachment 8546955 [details] [diff] [review]
> Part 2b - Make TransformToLocal() a virtual function in InputData
> 
> Review of attachment 8546955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would rather not make the fields mutable. Either of the alternatives in
> comment 20 is fine by me.

I went with alternative (2), taking the relevant part of the code you wrote in https://bug1117712.bugzilla.mozilla.org/attachment.cgi?id=8546074, and making it part of this (part 2c) patch. The part 2b patch is no longer necessary.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +573,5 @@
> >                                                              &hitResult);
> >        if (apzc) {
> >          MOZ_ASSERT(hitResult == ApzcHitRegion || hitResult == ApzcContentRegion);
> >  
> >          transformToApzc = GetScreenToApzcTransform(apzc);
> 
> Would be nicer to get rid of the transformToApzc temp var and call
> GetScreenToApzcTransform directly in the transformToGecko definition (same
> for the other hunks in this function).

Done. It is indeed nicer.

> ::: gfx/layers/apz/src/InputBlockState.cpp
> @@ +305,3 @@
> >  {
> >    while (HasEvents()) {
> > +    GetTargetApzc()->HandleInputEvent(RemoveFirstEvent(), mTransformToApzc);
> 
> Inline RemoveFirstEvent() here for consistency with the WheelBlockState
> code. I did that in my version too since I felt it was a little cleaner and
> RemoveFirstEvent doesn't provide much value by being separate.

Done.

> ::: gfx/layers/apz/src/InputQueue.cpp
> @@ +62,5 @@
> >          aTarget.get(), block->IsDefaultPrevented());
> >      if (!aTarget || block->IsDefaultPrevented()) {
> >        return true;
> >      }
> > +    aTarget->HandleInputEvent(aEvent, aTarget->GetTransformToThis());
> 
> hm.. i think the transform passed in here should be the one from the block
> rather than a fresh GetTransformToThis() (which may not be the same as it
> was at the start of the input block). Looks like I made the same mistake in
> my "alternate version" patch. You can add a function to CancelableStateBlock
> like |void DispatchImmediate(InputData&)| which asserts !HasEvents() and
> then calls HandleInputEvent using mTransformToApzc.

Done. I also removed the |aTarget| parameter of MaybeHandleCurrentBlock() because it seemed redundant. There were also opportunities to reuse DispatchImmediate() elsewhere, which I did.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> > I couldn't come up with any actual arguments against this other than my gut
> > feeling so let's go with it for now and see how things turn out. Would like
> > to see an updated version based on the new part b and with the
> > MaybeHandleCurrentBlock thing (see below) fixed.
> 
> I just realized that any arguments I had against this approach are
> completely destroyed by the fact that we do exactly the same thing for
> building the handoff chain. i.e. InputBlockState constructor calls
> APZC::BuildOverscrollHandoffChain which acquires the tree lock and calls
> into APZCTM. So doing it for the transform doesn't make the code any worse
> than it was before.

I added a comment above the constructor of InputBlockState documenting that it acquires the tree lock.

Comment 32

[Botond Ballo [:botond]]

Attachment: Part 2d - Update the screen-to-local transform matrix for an input block if the target APZC changes

Updated to fix a build bustage seen in the Try push due to not qualifying gfx::Matrix4x4. Carrying r+.

Comment 33

[Botond Ballo [:botond]]

Try push that includes these patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f40f1b579f6

Comment 34

[Botond Ballo [:botond]]

Attachment: Part 2c - Defer application of screen-to-local transform matrix until AsyncPanZoomController::HandleInputEvent()

Sorry, I uploaded the wrong patch. Here is the correct one.

Comment 35

[Botond Ballo [:botond]]

Attachment: Part 2d - Update the screen-to-local transform matrix for an input block if the target APZC changes

Fixed.

Comment 36

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #35)
> Created attachment 8547807 [details] [diff] [review]
> Part 2d - Update the screen-to-local transform matrix for an input block if
> the target APZC changes
> 
> Fixed.

Er, I meant "likewise".

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8547806 [details] [diff] [review]
Part 2c - Defer application of screen-to-local transform matrix until AsyncPanZoomController::HandleInputEvent()

Review of attachment 8547806 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comment addressed

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +181,5 @@
>    while (HasEvents()) {
>      TBS_LOG("%p returning first of %lu events\n", this, mEvents.Length());
>      ScrollWheelInput event = mEvents[0];
>      mEvents.RemoveElementAt(0);
> +    DispatchImmediate(event);

If you call DispatchImmediate here then the MOZ_ASSERT(!HasEvents()) in that function may fail. Either remove that assert or (I would prefer) just call GetTargetApzc()->HandleInputEvent directly here.

@@ +314,5 @@
>    while (HasEvents()) {
> +    TBS_LOG("%p returning first of %lu events\n", this, mEvents.Length());
> +    MultiTouchInput event = mEvents[0];
> +    mEvents.RemoveElementAt(0);
> +    DispatchImmediate(event);

Ditto

@@ +392,5 @@
>  }
>  
>  } // namespace layers
>  } // namespace mozilla
> +

nit: spurious blank line

Comment 38

[Botond Ballo [:botond]]

Attachment: Part 2c - Defer application of screen-to-local transform matrix until AsyncPanZoomController::HandleInputEvent()

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> Comment on attachment 8547806 [details] [diff] [review]
> Part 2c - Defer application of screen-to-local transform matrix until
> AsyncPanZoomController::HandleInputEvent()
> 
> Review of attachment 8547806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comment addressed
> 
> ::: gfx/layers/apz/src/InputBlockState.cpp
> @@ +181,5 @@
> >    while (HasEvents()) {
> >      TBS_LOG("%p returning first of %lu events\n", this, mEvents.Length());
> >      ScrollWheelInput event = mEvents[0];
> >      mEvents.RemoveElementAt(0);
> > +    DispatchImmediate(event);
> 
> If you call DispatchImmediate here then the MOZ_ASSERT(!HasEvents()) in that
> function may fail. Either remove that assert or (I would prefer) just call
> GetTargetApzc()->HandleInputEvent directly here.
> 
> @@ +314,5 @@
> >    while (HasEvents()) {
> > +    TBS_LOG("%p returning first of %lu events\n", this, mEvents.Length());
> > +    MultiTouchInput event = mEvents[0];
> > +    mEvents.RemoveElementAt(0);
> > +    DispatchImmediate(event);
> 
> Ditto

Doh, that was silly of me. Fixed.

> @@ +392,5 @@
> >  }
> >  
> >  } // namespace layers
> >  } // namespace mozilla
> > +
> 
> nit: spurious blank line

Fixed.

Carrying r+.

Comment 39

[Botond Ballo [:botond]]

New Try push since the mistake pointed out in comment 37 might cause gtest failures: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5237502df73

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Oh! Don't forget the gtest from my original part 2 patch.

Comment 41

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #40)
> Oh! Don't forget the gtest from my original part 2 patch.

Ah, indeed! Thanks for reminding me. ni myself so I don't forget.

Comment 42

[Botond Ballo [:botond]]

Attachment: Part 2e - Gtest

This is the gtest. It's taken verbatim from kats' original patch, so I'm posting it with kats as author and r=me.

I verified that the test passes with, and fails without, the fix in the Part 2d patch.

Comment 43

[Botond Ballo [:botond]]

Attachment: Part 2e - Gtest

aaand that was the wrong patch.

Comment 44

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #39)
> New Try push since the mistake pointed out in comment 37 might cause gtest
> failures:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5237502df73

All green! This is ready to land when the trees reopen.

Comment 45

[Botond Ballo [:botond]]

Rebased patches across the bug 1119497 backout and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e61e196e7f55
https://hg.mozilla.org/integration/mozilla-inbound/rev/90de1c9ab07c
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc1cbc1d7e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/605e82b2f066
https://hg.mozilla.org/integration/mozilla-inbound/rev/19f97e6eac5c

(Kats: heads up that a relanding of bug 1119497 will probably need a corresponding rebase.)

Comment 46

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #45)
> (Kats: heads up that a relanding of bug 1119497 will probably need a
> corresponding rebase.)

Thanks. Also these patches will need uplifting to b2g37, which means they will need to rebase again because bug 920036 is still in that branch. Whoever lands the last of these should make sure that the relevant hunks are identical between b2g37 and master to avoid having to rebase every future patch that we need to uplift :)

Comment 47

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #46)
> (In reply to Botond Ballo [:botond] from comment #45)
> > (Kats: heads up that a relanding of bug 1119497 will probably need a
> > corresponding rebase.)
> 
> Thanks. Also these patches will need uplifting to b2g37

Definitely. I'll request uplift after they land on m-c.

>, which means they
> will need to rebase again because bug 920036 is still in that branch.
> Whoever lands the last of these should make sure that the relevant hunks are
> identical between b2g37 and master to avoid having to rebase every future
> patch that we need to uplift :)

Fun! :) Thanks for the heads up!

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e61e196e7f55
https://hg.mozilla.org/mozilla-central/rev/90de1c9ab07c
https://hg.mozilla.org/mozilla-central/rev/edc1cbc1d7e4
https://hg.mozilla.org/mozilla-central/rev/605e82b2f066
https://hg.mozilla.org/mozilla-central/rev/19f97e6eac5c

Comment 49

[Botond Ballo [:botond]]

Attachment: Rollup for aurora uplift

Rebased patches to aurora - being careful to make sure the resulting hunks are identical to the ones in the rebased Part 2 patch I posted in bug 1119497, as kats advised in comment 46 - and combined them. Carrying r+.

Approval Request Comment
[Feature/regressing bug #]: 
  {Event regions}-based hit testing in APZ (enabled in bug 928833).

[User impact if declined]: 
  Hit testing can give incorrect results on APZ platforms (B2G), leading to 
  touches being ignored or mistargeted.

[Describe test coverage new/current, TBPL]:
  The patch includes a gtest for the bug, and is on m-c.
  
[Risks and why]: 
  Low compared to the severity of the regression it's fixing.
  
[String/UUID change made/needed]:
  None.

Comment 50

[Lawrence Mandel [:lmandel] (use needinfo)]

From comment 49 I take it that this fix is B2G specific. If that's the case, B2G has already branched for 37. Please request approval‑mozilla‑b2g37.

Comment 51

[Botond Ballo [:botond]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #50)
> From comment 49 I take it that this fix is B2G specific. If that's the case,
> B2G has already branched for 37. Please request approval‑mozilla‑b2g37.

The fix applies to all setups where APZ and event-regions are both enabled. 

They are both enabled by default on B2G, and disabled by default elsewhere, but they can be manually enabled by toggling prefs. So, someone playing with Aurora desktop and turning on both APZ and event-regions would run into this bug.

I'm not sure if that's a compelling argument for uplifting to Aurora; if you don't think so, I'll request approval-mozilla-b2g37 as you suggest.

Comment 52

[Kartikaya Gupta (email:[e-mail])]

FWIW I think b2g37 would be more appropriate here since it should have no effect on non-b2g default configurations.

Comment 53

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8549211 [details] [diff] [review]
Rollup for aurora uplift

(In reply to Botond Ballo [:botond] from comment #51)
> They are both enabled by default on B2G, and disabled by default elsewhere,
> but they can be manually enabled by toggling prefs. So, someone playing with
> Aurora desktop and turning on both APZ and event-regions would run into this
> bug.
> 
> I'm not sure if that's a compelling argument for uplifting to Aurora; if you
> don't think so, I'll request approval-mozilla-b2g37 as you suggest.

I don't think that's a good reason to uplift this patch to Aurora. We have other prefs (like e10s) that, if enabled, will put the user at serious risk of encountering bugs as well. Given the size of this patch, I think we should land it where it's required - on the b2g branch.

I have marked this as aurora- and flagged it for b2g37 approval. Please see comment 47 for the approval request.

Comment 54

[Andrew Sutherland [:asuth] (he/him)]

Is it possible for this to cause an infinite-loop lockup?  On bug 1120447 comment 2 :kats made reference to "replacing unconfirmed target" and this bug fixing something with that.

On bug 1122276 there is a device lockup where the following is seen approximately around the time of the lockup sadness:
01-15 10:46:52.620: I/Gecko(216): 0xae9605f0 replacing unconfirmed target 0xa9e88800 with real target 0x0

The code seems to fallback to a default matrix, but it seems plausible that a default matrix could somehow result in extreme sadness.  I'll request a regression-check against this specific landing there...

Comment 55

[Kartikaya Gupta (email:[e-mail])]

FWIW it seems highly unlikely to me that this would cause an infinite loop lockup. Can't say for sure until I repro though.

Comment 57

[Botond Ballo [:botond]]

(In reply to Andrew Sutherland [:asuth] from comment #54)
> Is it possible for this to cause an infinite-loop lockup?  On bug 1120447
> comment 2 :kats made reference to "replacing unconfirmed target" and this
> bug fixing something with that.
> 
> On bug 1122276 there is a device lockup where the following is seen
> approximately around the time of the lockup sadness:
> 01-15 10:46:52.620: I/Gecko(216): 0xae9605f0 replacing unconfirmed target
> 0xa9e88800 with real target 0x0
> 
> The code seems to fallback to a default matrix, but it seems plausible that
> a default matrix could somehow result in extreme sadness.  I'll request a
> regression-check against this specific landing there...

This turned out to be a regression caused by bug 1109873, not this bug. It's being fixed by patches in bug 1122276.

Comment 58

[bhavana bajaj [:bajaj]]

(In reply to Botond Ballo [:botond] from comment #57)
> (In reply to Andrew Sutherland [:asuth] from comment #54)
> > Is it possible for this to cause an infinite-loop lockup?  On bug 1120447
> > comment 2 :kats made reference to "replacing unconfirmed target" and this
> > bug fixing something with that.
> > 
> > On bug 1122276 there is a device lockup where the following is seen
> > approximately around the time of the lockup sadness:
> > 01-15 10:46:52.620: I/Gecko(216): 0xae9605f0 replacing unconfirmed target
> > 0xa9e88800 with real target 0x0
> > 
> > The code seems to fallback to a default matrix, but it seems plausible that
> > a default matrix could somehow result in extreme sadness.  I'll request a
> > regression-check against this specific landing there...
> 
> This turned out to be a regression caused by bug 1109873, not this bug. It's
> being fixed by patches in bug 1122276.

https://bugzilla.mozilla.org/show_bug.cgi?id=1122276 is a+Ed as well to avoid the regression and this looks good to land now given we know the root cause.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/dcb79843b2b4

Comment 60

[Sue]

Attachment: video.MP4

This issue has been verified successfully on Flame 2.2/3.0
STR:
Precondition：Create 2 all day events in a day
1. Launch Calendar.
2. Switch to Week view.
3. Tap the all day event.
**It will enter all day event detail view.
See attachment:video.MP4
Rate:0/5

Flame 2.2 build:
Build ID               20150215002504
Gaia Revision          ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gaia Date              2015-02-13 05:27:43
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/62c80c92b39e
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150215.040852
Firmware Date          Sun Feb 15 04:09:03 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150215010209
Gaia Revision          f0b93e0668ef9565bd6f050b15b4f794d59feb65
Gaia Date              2015-02-13 13:13:27
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e0cb32a0b1aa
Gecko Version          38.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150215.043133
Firmware Date          Sun Feb 15 04:31:43 EST 2015
Bootloader             L1TC000118D0