790454 - (CVE-2013-0751) Touch events are shared across iframes

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Wesley Johnston (:wesj)]

Just did some quick tests to see what happens if users attempt to use touch events in two different iframes. In a page with two iframes next to one another, if you touch in one and then the touch in the other, we are able to see touches in the first iframe in the second (and vice versa). You can also look at the target of those touches. If they're both from the same origin, you can also access properties/methods on the target of the second. If they're from different domains, trying to access any properties/methods on that target will result in a "Permission denied to access property" exception being thrown.

Test page:
http://dl.dropbox.com/u/72157/iframetest.html

Chrome for Android leaks the same touches, but also doesn't throw access restriction exceptions. Filed a bug with them:
http://code.google.com/p/chromium/issues/detail?id=148567

iOS 5.1.1 it looks like touches in the second (right hand side) iframe are ignored all of the time. i.e. I never see any touches sent to the second iframe. If I start a drag in it and then move my finger into the first, I see no events until my finger is over the first document (doing that requires that I have touched the first document first so that they don't pan, but the touches I receive in the first document act as if there is only one finger on screen. i.e. its a bit of a mess).

Comment 1

[Andrew McCreight [:mccr8]]

If you can't access things across domains, it doesn't seem too terrible. What security rating would you give this?

Comment 2

[Wesley Johnston (:wesj)]

I would guess moderate? You can dump event.touches[0].target and see what the touched node is. If its an anchor it will return the link location which could potentially contain secure info. Not sure how severe that is?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Wes, could you do some more testing with Safari on iOS (and perhaps Chrome on Android).
(I don't have devices to test this all)

We need to decide where the events should be dispatched.
Should we have several touch sessions active, one for each iframe, or should we dispatch
all the events to the same document?
What if we have main document which has touch event listeners and the document has also
iframe which has listeners and you start one touch on main document and one one iframe document.
...needs testing.

I hope we could dispatch all the events to the same document - once you start touch event session on one
document, all the events go there until the end of the session.
That would be
easiest to implement and perhaps easiest to understand by developers.
But if iOS-Safari has some totally different behavior, we may need to do something else.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #3)
> I hope we could dispatch all the events to the same document - once you
> start touch event session on one
> document, all the events go there until the end of the session.

Are you calling a "session" multiple series of touchstart/touchmove/touchend, i.e. for multiple touch points?  If so, I like this behavior from the platform side because it's easier to optimize.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It also seems less likely to lead to these kinds of security issues with multiple cross-domain (or even cross-process) touch targets.

Comment 6

[Andrew McCreight [:mccr8]]

I'm going to conservatively set this as sec-high...

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Are you calling a "session" multiple series of
> touchstart/touchmove/touchend, i.e. for multiple touch points?
Yes, basically from the start of the first touch to the end of the last touch.

Comment 8

[Daniel Veditz [:dveditz]]

It's not allowing one frame to steal data from another, and leaking actions of the user don't seem quite as serious. I'm going w/Wes in comment 2.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Wesj, does target.innerHTML work in the second iframe? If so, this should definitely be
sec-high, perhaps even critical.

Comment 10

[Rick Byers]

A scenario to consider when choosing the best solution:
An app should be able to design app-wide multi-finger gestures.  Eg., Facebook should be able to implement a three-finger forward/back gesture, even if one of those fingers happens to start on a youtube video.

If we agree this is a legitimate scenario, then we should focus on solutions that remove the DOM element data from the TouchList (eg. by setting target to null when it belongs to a different frame than the receiver), rather than filtering which touches get reported in the event.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Rick Byers from comment #10)
> A scenario to consider when choosing the best solution:
> An app should be able to design app-wide multi-finger gestures.  Eg.,
> Facebook should be able to implement a three-finger forward/back gesture,
> even if one of those fingers happens to start on a youtube video.
Yes, that is why I'd like all the touches to be in the same touch session.
So, youtube wouldn't get any events, but all would go to Facebook, because touch session
started in that page. The actual touch over Youtube would show up as touch which has
Facebook's documentElement as target. Facebook wouldn't know anything about Youtube.


> If we agree this is a legitimate scenario, then we should focus on solutions
> that remove the DOM element data from the TouchList (eg. by setting target
> to null when it belongs to a different frame than the receiver), rather than
> filtering which touches get reported in the event.
No such odd special cases, please.

Comment 12

[Rick Byers]

(In reply to Olli Pettay [:smaug] from comment #11)
> (In reply to Rick Byers from comment #10)
> > A scenario to consider when choosing the best solution:
> > An app should be able to design app-wide multi-finger gestures.  Eg.,
> > Facebook should be able to implement a three-finger forward/back gesture,
> > even if one of those fingers happens to start on a youtube video.
> Yes, that is why I'd like all the touches to be in the same touch session.
> So, youtube wouldn't get any events, but all would go to Facebook, because
> touch session
> started in that page. The actual touch over Youtube would show up as touch
> which has
> Facebook's documentElement as target. Facebook wouldn't know anything about
> Youtube.

I'm a little confused on exactly how that would work without breaking iframes that do use touch events.  Eg., if I put finger 1 down first on the iframe, then (10 ms later) finger 2 down outside the iframe, which frame gets the 'touch session' now?  Do we need to try to 'transfer' the touch session somehow?

> > If we agree this is a legitimate scenario, then we should focus on solutions
> > that remove the DOM element data from the TouchList (eg. by setting target
> > to null when it belongs to a different frame than the receiver), rather than
> > filtering which touches get reported in the event.
> No such odd special cases, please.

Maybe it's less 'odd' if you get a valid node, but any property/method on it throws an AccessViolation exception?

Comment 13

[Wesley Johnston (:wesj)]

I updated the test a bit to test the innerHTML bit Smaug. Long story short, we don't leak innerHTML. I think that was just some confusion.

Basically now if you touch in the left and right frames (Touch the links so that you don't break the world by appending body.innerHTML over and over), and move the finger in the left frame you'll see something in the right frame (in Firefox) like:

touchmove
Changed Touches: 2
Touches: 1
Target Touches: 1
Target
  Target: http://people.mozilla.org/~wjohnston/touchTest2.html#
  URL: http://people.mozilla.org/~wjohnston/touchTest2.html
  InnerHTML: And touch here
Changed touch 1
  Target: Error Permission denied to access property valueOf()
  URL: Error: Permission denied to access property ownerDocument
  InnerHTML: Permission denied to access  property innerHTML
Changed touch 2
  Target: http://people.mozilla.org/~wjohnston/touchTest2.html#
  URL: http://people.mozilla.org/~wjohnston/touchTest2.html
  InnerHTML: And touch here

This actually looks better than what I reported before. I swear before we weren't getting the "Error Permission denied to access property valueOf()" bit, but the results are a bit confusing (we send touch events for tiny jitter and pressure changes after the first finger moves a bit leading to a lot of changes for the second touch even if it seems stationary on the screen), so I may have been mistaken.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Rick Byers from comment #12)
> I'm a little confused on exactly how that would work without breaking
> iframes that do use touch events.  Eg., if I put finger 1 down first on the
> iframe, then (10 ms later) finger 2 down outside the iframe, which frame
> gets the 'touch session' now? 
If the first finger is still down, the first iframe would get all the events.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Ah, ok, our security checks do what they should be doing :)
But web page shouldn't get any security exceptions.
If it can't use touch.target, it shouldn't get access to that touch.

Comment 16

[Wesley Johnston (:wesj)]

Did some iPad testing as well. It seems like there touchevents are disabled unless you have a metaviewport set on the document (that seems new in iOS6). There's some strangeness there if the parent has a metaviewport and the children don't or vice versa, but that's a separate bug.

Once that is enabled, only the first iframe in the document seems to be able to receive touches. Same or Cross-origin doesn't seem to matter, but only the first iframe will get events.

A second touches in the second iframe isn't sent to the first, but can trigger touchmove events at the same coordinates as the first finger. I think an optimization not to send events if the touch point hasn't moved far is being broken.

The parent-child relationship is closer to what I think we might want. If you touch in the child first, you will only receive events in the child. Putting a second finger in the parent triggers the same optimization bug I mentioned above (i.e. you see a touchmove event associated with the first finger in the iframe, but aren't able to see the second finger). If you touch in the parent first and then the child you receive all of the events, but the target of the second finger is just the iframe.

Hope that makes sense. Hollar if you have any other ideas for testing. These test pages are at the same dropbox location:

http://dl.dropbox.com/u/72157/iframetest.html
http://dl.dropbox.com/u/72157/iframetest2.html - Test to same origin iframes
http://dl.dropbox.com/u/72157/iframetest3.html - Test one same origin iframe + parent
http://dl.dropbox.com/u/72157/iframetest4.html - Test one x-origin iframe + parent
http://dl.dropbox.com/u/72157/iframetest5.html - Same as 1, but with iframe order flipped

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I this we should do that.

Wesj, could you still test what happens if you do both touches on iframe but move then
one finger to parent.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Could anyone test what Opera does?

Comment 19

[Rick Byers]

Re #18: I tested two fingers started on the iframe and moving one to the parent on iOS (5 and 6).  It behaves as expected - events for both fingers continue to be delivered to the iframe only.

Comment 20

[Sangwhan Moon]

From the back of my head, I was a bit concerned but after looking at the implementation a bit more I can safely conclude that Opera is not affected by this.

iOS seems to be not affected in a different way, as it seems like when two frames, if touch starts from one frame, all consequential touches will get associated to the origin frame of the first document where the touch started, and the second frame's listener will never get triggered. Opera does this differently and will associate a touch "session" to each HTMLDocument where a touch starts, so essentially if you put one finger on "Touch Here" and then another on "And Touch Here" two documents will have two completely independent touch lists containing one touch on each document. This leaves us safe from having to deal with origin checks as all touches will be of a single origin, but may have potential interoperability issues as mentioned by Rick above.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: untested wip

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 668569 [details] [diff] [review]
untested wip

The patch has few a bit unrelated changes.


>@@ -5937,41 +5958,74 @@ PresShell::HandleEvent(nsIFrame        *
>       case NS_TOUCH_END: {
>         // Remove the changed touches
>         // need to make sure we only remove touches that are ending here
>         nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent);
>         nsTArray<nsCOMPtr<nsIDOMTouch> >  &touches = touchEvent->touches;
>         for (uint32_t i = 0; i < touches.Length(); ++i) {
>           nsIDOMTouch *touch = touches[i];
>           if (!touch) {
>-            break;
>+            continue;
>           }
>   
>           int32_t id;
>           touch->GetIdentifier(&id);
>           nsCOMPtr<nsIDOMTouch> oldTouch;
>           gCaptureTouchList.Get(id, getter_AddRefs(oldTouch));
>           if (!oldTouch) {
>-            break;
>+            continue;
>           }
>   
>           nsCOMPtr<nsPIDOMEventTarget> targetPtr;
>           oldTouch->GetTarget(getter_AddRefs(targetPtr));
>           nsCOMPtr<nsIContent> content = do_QueryInterface(targetPtr);
>           if (!content) {
>-            break;
>+            continue;
>           }
> 
>           nsIFrame* contentFrame = content->GetPrimaryFrame();
>           if (!contentFrame) {
>-            break;
>+            continue;
>           }
> 
>+          frame = contentFrame;
>           shell = static_cast<PresShell*>(
>                       contentFrame->PresContext()->PresShell());
We really want try hard to get the right frame and shell



>       nsTouchEvent newEvent(NS_IS_TRUSTED_EVENT(touchEvent) ?
>                               true : false,
>                             touchEvent);
>-      newEvent.target = targetPtr;
>-
>-      // If someone is capturing, all touch events are filtered to their target
>-      nsCOMPtr<nsIContent> content = GetCapturingContent();

We don't want to retarget to random document.
Capturing content is for mouse.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Martijn, could you do some testing here. Would be good to get extra pair of eyes.
Ping me on IRC if you have questions about this.

(Wes will upload an updated patch soon).

Comment 24

[Wesley Johnston (:wesj)]

Comment on attachment 668569 [details] [diff] [review]
untested wip

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

Seems mostly fine to me (with a few variable renames and some comments about what's going on). Two issues that I have a patch for.

::: layout/base/nsPresShell.cpp
@@ +6019,5 @@
> +            }
> +            return NS_OK;
> +          }
> +          frame = newTargetFrame;
> +          shell = static_cast<PresShell*>(frame->PresContext()->PresShell());

Talked to you online, but the list we actually dispatch from is the one in the DOMEvent, so on touchmove events we'll need to also remove those touches. I have a patch for that I'll put up here to go along with this.

@@ +6559,5 @@
>                                true : false,
>                              touchEvent);
> +
> +      nsCOMPtr<nsIContent> content = do_QueryInterface(targetPtr);
> +      MOZ_ASSERT(content);

I added this to minimize the changes we needed to support nsSliderFrame's (i.e. they capture touch events, which causes events to be routed to them directly from a nsPresShellEventCB. We could modify and fix that now. As per our irc discussion, I have a patch that leaves this, but does a check to make sure capturing target is in the same document as the touch events.

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch

Marking for feedback because the first patch isn't up for review yet. I think this fixes the issue though.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch

Whoops. Not the right patch. Here we go.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 668700 [details] [diff] [review]
Patch


>+        // check if the capturecontent is in the same document as the event's target
>+        if (content->GetDocument() != targetContent->GetDocument()) {
>+            content = nullptr;
This would lead to somewhat odd behavior. If there is capturing content in different
document, we wouldn't care about it.
Would be better to just not dispatch the event

>       tmpStatus = nsEventStatus_eIgnore;
>       nsEventDispatcher::Dispatch(targetPtr, context,
>                                   &newEvent, nullptr, &tmpStatus, aEventCB);
>       if (nsEventStatus_eConsumeNoDefault == tmpStatus) {
Oh, I hadn't noticed this. We dispatch to targetPtr even if there is capturingcontent.

f+ for the first part of the patch.

I'll upload a new one.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #28)
> Created attachment 668767 [details] [diff] [review]
> patch
So if I read the code correctly this should help with the sliderframe case too, since
we end up pushing the target as event info, so nsPresShellEventCB should be able to
do what it is supposed to do.

Comment 30

[Wesley Johnston (:wesj)]

Comment on attachment 668767 [details] [diff] [review]
patch

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

::: layout/base/nsPresShell.cpp
@@ +6387,5 @@
>        }
>        case NS_TOUCH_MOVE: {
>          // Check for touches that changed. Mark them add to queue
>          nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent);
>          nsTArray<nsCOMPtr<nsIDOMTouch> > touches = touchEvent->touches;

We need to make a reference here, so that we change the original array below (and also because this should be a reference anyway I think?)

@@ -6511,5 @@
> -
> -      // if no one is capturing, set the capturing target
> -      if (!content) {
> -        content = do_QueryInterface(targetPtr);
> -      }

Remove this breaks drags that start on the thumb of a slider frame. Slider's have a thumbFrame inside them. We add an event listener to that thumb frame [1], that calls setCapturingContent with the slider frame (its parent) (i.e. we redirect the target of these touches using the mouse capture api's):

As a longer term solution, we could listen for all touch events here and forward them to the normal slider frame? There's a bit of duplicated code in here already that it would be nice to simplify at the same time.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsSliderFrame.cpp#842

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

We could fix the capturing content problem in a different bug.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 669265 [details] [diff] [review]
v3

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Reasonable easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Right now this doesn't have automatic tests. I need to write some.

Which older supported branches are affected by this flaw?
all but esr10

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Code should be similar in all the affected branches.

How likely is this patch to cause regressions; how much testing does it need?
Testing is needed. This changes touch event handling to be more aligned with iOS Safari.
Chrome (and other webkit based, not Apple, browsers) has this same bug and is about to do similar changes.

Comment 33

[Rick Byers]

[e-mail] who is working on the equivalent fix in WebKit (https://bugs.webkit.org/show_bug.cgi?id=97973).  We should compare fixes / test coverage looking for gaps and consistent behavior.

Comment 34

[Min Qin]

Comment on attachment 669265 [details] [diff] [review]
v3

I am not very familiar with the mozilla code base, but i think the logic below will be problematic in webkit.

if (touchEvent->touches.Length() == 1) {
          firstTouch = true;

When the first touch happens, there could be 2 or more touchstart touch events. The target iframe is hard to determine though. In my webkit patch, I just pick the first touch start to determine the target iframe.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Ah, good point.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 669265 [details] [diff] [review]
v3

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

I guess we need a new patch to cover comment #34

::: layout/base/nsPresShell.cpp
@@ +5634,5 @@
>  }
>  
> +static PLDHashOperator
> +FindAnyTarget(const uint32_t& aKey, nsCOMPtr<nsIDOMTouch>& aData,
> +              void* aAnyTarget)

Document this

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) 
> I guess we need a new patch to cover comment #34
Yes, Wes is about to update the patch.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 669265 [details] [diff] [review]
v3

New patch coming

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Patch v3

This is an altered approach to this that removes the evicting code which we don't need, and unifies DispatchTouchEvent to work better on events with multiple touches in touchstart. We no longer use any of the targeting code that mouse events use for touch events.

I'm working on some tests for these things but fighting DOMWindowUtils and iframes a bit. Wanted to put this up for now.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 670619 [details] [diff] [review]
Patch v3

I think we can't remove the evict code.
What if touchstart or touchmove event closes browser window? We wouldn't
dispatch touchend.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

(also, would be good to keep changes as small as possible)

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Not sure exactly about the issue with evict, but I'm fine minimizing this change. This adds it back in.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 671622 [details] [diff] [review]
Patch v4


>+FindAnyTarget(const uint32_t& aKey, nsCOMPtr<nsIDOMTouch>& aData,
>+              void* aAnyTarget)
>+{
>+  if (aData) {
>+    nsCOMPtr<nsIDOMEventTarget> target;
>+    aData->GetTarget(getter_AddRefs(target));
>+    if (target) {
>+      nsCOMPtr<nsIContent>* aContent =
>+        static_cast<nsCOMPtr<nsIContent>*>(aAnyTarget);
>+      *aContent = do_QueryInterface(target);
>+      return PL_DHASH_STOP;
>+    }
Oops, I guess my code. Variable name should be 'content', not aContent.

>+            if (target && !anyTarget) {
>+                target->GetContentForEvent(aEvent, getter_AddRefs(anyTarget));
>+                while (anyTarget && !anyTarget->IsElement()) {
>+                  anyTarget = anyTarget->GetParent();
>+                }
>+                domtouch->SetTarget(anyTarget);
>+                gCaptureTouchList.Put(id, touch);
2 space indentation, please.

>+    }
>+    nsPresContext *context = nsContentUtils::GetContextForContent(content);
>+    if (!context) {
>+      context = mPresContext;
>+    }
context should be null if you can't get right one.

(This requires sec-approval before landing. And unfortunately this is rather regression risky)

Comment 44

[Wesley Johnston (:wesj)]

Comment on attachment 671622 [details] [diff] [review]
Patch v4

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Fairly easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes. There are comments in the code and function names that make it not hard to deduce what we're doing. For a commit message, I can write something like "Find target for touch events during touchstart" 

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?
14

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No yet. I don't think this code has changed much since it was originally landed though. I don't anticipate huge differences between the patches, but I'm not sure this is a big enough security risk that we need to push it forward either. Since b2g is shipping off 18, I'd like to at least push something there. There is moderate risk here as this is a non-trivial change no matter how we do it. 

How likely is this patch to cause regressions; how much testing does it need?
Moderate. This changes event targeting for touch events in a non-trivial way. I've tested it pretty extensively and not found any problems, and it should only affect pages doing some strange things (none exist that I've ever seen). Our userbase testing touchevents is relatively small compared to desktop.

Comment 45

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 671622 [details] [diff] [review]
Patch v4

This is a sec-moderate so I'm not as worried. Since it has a potential for regressions, let's get this in. Please prepare Aurora and Beta patches as well.

Comment 46

[Wesley Johnston (:wesj)]

Comment on attachment 671622 [details] [diff] [review]
Patch v4

This requires no changes for Aurora and a very tiny one for beta. I'm not sure if I need approval or not? but I figured I'd ping for it.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 603008 - Touch Events
User impact if declined: touchevent implementation doesn't match other browsers. Potential security issues (at the very least you can detect where in an iframe the user is touching).
Testing completed (on m-c, etc.): Landed on mc 10/29
Risk to taking this patch (and alternatives if risky): Moderate risk of regression. No good alternatives really.
String or UUID changes made by this patch: None

Comment 47

[Wesley Johnston (:wesj)]

Follow up for opt clang build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b45a4dcb88c1

Comment 48

[Wesley Johnston (:wesj)]

backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d4051649e96

Comment 49

[Alex Keybl [:akeybl]]

Comment on attachment 671622 [details] [diff] [review]
Patch v4

Clearing for now while you investigate the bustage.

Comment 50

[Wesley Johnston (:wesj)]

I shouldn't have removed that code that sets shell for move/end/cancel events. Putting that back in there is still a crash when trying to use a non-existence context when trying to find the screen coordinates of a point;

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMEvent.cpp#1078

I'm also getting some strange behaviour when a target element is removed from the document during touchstart. Looking at that more...

Comment 51

[Wesley Johnston (:wesj)]

Attachment: Follow up for test failures

Ahh, so the issue is that nsContentUtils::GetContextForContent relies on the node being passed to it having a CurrentDocument. If it doesn't, it returns null. Since the target may not have a CurrentDocument in this case, we need to find some other way to get screen coordinates or make sure we have a valid presContext. I took the later approach.

Instead of using ContentUtils, I tried just dig out the presContext associated with target->OwnerDocument directly here instead.

Comment 52

[Wesley Johnston (:wesj)]

Comment on attachment 676874 [details] [diff] [review]
Follow up for test failures

The failing tests seem fine locally. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=90890c04acbb

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 676874 [details] [diff] [review]
Follow up for test failures


>+    nsPresContext *context;
>+    if (!context) {
Here you're using uninitialized variable.


>+      if (!doc) {
>+        continue;
>+      }
doc can't be null here.

Comment 54

[Wesley Johnston (:wesj)]

Attachment: Follow up patch v2

https://tbpl.mozilla.org/?tree=Try&rev=bb24b066a779

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 677064 [details] [diff] [review]
Follow up patch v2


>     PresShell* shell =
>         static_cast<PresShell*>(frame->PresContext()->PresShell());
>+    switch (aEvent->message) {
>+      case NS_TOUCH_MOVE:
>+      case NS_TOUCH_CANCEL:
>+      case NS_TOUCH_END: {
>+        // Remove the changed touches
>+        // need to make sure we only remove touches that are ending here
This comment is wrong

>+
>+          shell = static_cast<PresShell*>(
>+                      contentFrame->PresContext()->PresShell());
add 
if (shell) {
  break;
}

Comment 56

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34137da41cbb

Comment 57

[Wesley Johnston (:wesj)]

Comment on attachment 671622 [details] [diff] [review]
Patch v4

Comment on attachment 671622 [details] [diff] [review] [diff] [review]
Patch v4

Everything looks good this time. Same comments as before.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 603008 - Touch Events
User impact if declined: touchevent implementation doesn't match other browsers. Potential security issues (at the very least you can detect where in an iframe the user is touching).
Testing completed (on m-c, etc.): Landed on mc 10/29
Risk to taking this patch (and alternatives if risky): Moderate risk of regression. No good alternatives really.
String or UUID changes made by this patch: None

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34137da41cbb

Comment 59

[Arthur Barstow]

(In reply to Ryan VanderMeulen from comment #58)
> https://hg.mozilla.org/mozilla-central/rev/34137da41cbb

Olli, Matt, Rick, Sangwhan (WebEvents people) - given this resolution, is there a need to make any related normative changes to the Touch Events v1 specification (http://www.w3.org/TR/2011/CR-touch-events-20111215/)? How about any non-normative (informative) changes e.g. hints, guidelines or such?

Comment 60

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 671622 [details] [diff] [review]
Patch v4

Given that this is so late in the beta cycle and is a sec-moderate, please go ahead with uplift to Aurora but we'll hold off on Beta 17 since we're out of time for gathering data on possible regressions with enough room to backout if need be before shipping.

Comment 61

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91bc264379f9