995603 - (CVE-2014-1539) Cursor can be totally invisible using flash object and div

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jordi Chancel]

Attachment: test INVISIBLE MOUSE.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

firstly i would to say that it's not a flash issue and it's works only for firefox for mac os x (don't works on WINDOWS , don't works on Google Chrome , don't works on Opera, don't works on safari).
when you go on a flash object that defined the cursor like unvisible and you go on a <div> with the cursor , the cursor is totaly unvisible (don't works on WINDOWS , don't works on Google Chrome , don't works on Opera, don't works on safari).
This can lead to clickjacking , cursorjacking , spoofing or others...

steps:
1 : go to the flash object (in center into the flash object) 
2 : wait 2 s 
3 : go with your cursor down (div t2)
4 : Cursor are cursor-jacked 

(view this youtube video for more details => https://www.youtube.com/watch?v=5Lu8pcWvUVY&feature=youtu.be  )


Actual results:

Cursor are totaly unsibile , when you try to install a xpi , the cursor is totally unvisible.


Expected results:

this can be used for clickjacking / cursorjacking  / spoofing or others , 
this issue works only on Firefox for MAC .

Comment 1

[Mats Palmgren (inactive)]

If you use http: instead of file: then there's an additional dialog to click through:
"Firefox prevented this site from asking you to install..."

Comment 2

[Mats Palmgren (inactive)]

I'm trying to understand why the real mouse pointer isn't reset to normal
when it's outside the Flash object, is it the overlapping boxes with high
z-index that prevents that?

Comment 3

[Jordi Chancel]

with WWebRTC we can remotely activate the microphone and the webcam with this cursorjacking/clickjacking!

Comment 4

[Mats Palmgren (inactive)]

The mouse pointer is still visible over the plugin on Linux, on OSX it's hidden.
Adding onmouseout/over callbacks shows that Gecko is aware the mouse has left
the plugin element on both platforms.  I wonder if we could use that to send
an event to the plugin somehow to inform it the mouse pointer is outside?

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Playing with this bug's testcase (just moving the mouse around over it) I sometimes see *two* cursors in different positions -- one's the standard OS pointer cursor, and I suspect the other is something created by Flash.

So I wonder if there's confusion about the cursor's coordinates, and if it's not actually invisible but offscreen.

Mats, do you see this?

The STR from comment #0 is very vague and hard to understand.  Jordi, I realize that English isn't your first language.  But I understand some French, so please try re-writing the STR in French.  Make it as detailed and precise as you can.

Once I understand it, I'll re-write your STR in English.

Comment 6

[Jordi Chancel]

(In reply to Steven Michaud from comment #5)
> Playing with this bug's testcase (just moving the mouse around over it) I
> sometimes see *two* cursors in different positions -- one's the standard OS
> pointer cursor, and I suspect the other is something created by Flash.
> 
> So I wonder if there's confusion about the cursor's coordinates, and if it's
> not actually invisible but offscreen.
> 
> Mats, do you see this?
> 
> The STR from comment #0 is very vague and hard to understand.  Jordi, I
> realize that English isn't your first language.  But I understand some
> French, so please try re-writing the STR in French.  Make it as detailed and
> precise as you can.
> 
> Once I understand it, I'll re-write your STR in English.

quand vous allez sur l'object flash , le curseur devient invisible , et quand vous allez vers le bas ensuite un faux curseur décalé apparait, (le vrai curseur reste tout de même invisible comme a pu le remarquer Mats Palmgren.
ceci peut conduire a des attaque de clickjacking/cursorjacking/spoofing non négligeable!

je ne comprend pas pourquoi chez toi le curseur apparait normalement après que tu l'ais placé sur l'objet flash et déplacé ensuite vers un div..?

Comment 7

[Mats Palmgren (inactive)]

Yes, the second cursor is part of the clickjacking trick; it's an abs.pos. image
that's intended to look like the real mouse cursor.  When hiding the real cursor
succeeds, it makes you think that's where the mouse pointer is.  Bug 899992 uses
the same trick but is using cursor:url(transparent-image) to hide the real cursor.
You could use cursor:none too.

I think you can ignore that part, it's just a demonstration that hiding the real
mouse pointer can be used for clickjacking.  The interesting part is that the
plugin fails to reset the cursor to normal when moving the mouse outside it.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Thanks Mats.

I remember seeing (some months ago, testing with my Debug Events Plugin from bug 441880) some cases where we *don't* send a mouse-exit event to the plugin.  If I remember right it had something to do with overlapping objects (as are present in this bug's testcase).  When I get the chance (in the next few days?) I'll test again with it to see if that might be the problem here.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> je ne comprend pas pourquoi chez toi le curseur apparait normalement
> après que tu l'ais placé sur l'objet flash et déplacé ensuite vers
> un div..?

I didn't understand your STR (steps to reproduce), so I didn't use
them.  I just moved (not dragged) the mouse around above the testcase.

I *still* don't really understand them.  Could you please re-write
your steps-to-reproduce, in French and in detail?

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

I'll take a look. Give me a day or so.

Comment 11

[Matt Wobensmith [:mwobensmith][:matt:]]

I can see this pretty easily. The cursor simply doesn't reappear after moving first to the SWF (wait 2 seconds) and then to the DIV on the right. At this point, the cursor remains invisible, and indeed one could easily create a fake cursor that could trick the user into clicking something they should not.

I don't know what rating this class of attack gets, but it's clearly a bug.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> The cursor simply doesn't reappear after moving first to the SWF (wait 2 seconds) and then to
> the DIV on the right.

"Move", not "drag"?

Once I know we'll have clear STR, and I'll be able to take a closer look at this bug (replacing the SWF with my Debug Events Plugin).

Comment 13

[Jordi Chancel]

> I don't know what rating this class of attack gets, but it's clearly a bug.

because if the user don't see where he clicks , this can lead to clickjacking or cursor jacking or other spoofing...

you can view my youtube video in the comment 0!

Comment 14

[Jordi Chancel]

i go code a testcase that works remotely , in this testcase i will cursorjacked the webrtc for show the webcam maliciously.

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to Steven Michaud from comment #12)
> > The cursor simply doesn't reappear after moving first to the SWF (wait 2 seconds) and then to
> > the DIV on the right.
> 
> "Move", not "drag"?


Move.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #8)

> I remember seeing (some months ago, testing with my Debug Events
> Plugin from bug 441880) some cases where we *don't* send a
> mouse-exit event to the plugin.  If I remember right it had
> something to do with overlapping objects (as are present in this
> bug's testcase).  When I get the chance (in the next few days?) I'll
> test again with it to see if that might be the problem here.

I replaced the Flash plugin with my Debug Events plugin in this bug's
testcase, and found that I'd remembered correctly.

After you move the mouse over either of the two big divs to the top or
left of the plugin and wait 2 seconds, both of them are resized to
partially overlap the plugin.  Once this happens, moving the mouse out
of the plugin to the top or right (where the overlapping occurs)
doesn't cause a NPCocoaEventMouseExited event to be sent to the
plugin.

It looks like the divs are above the plugin in the z-order.  I'll try
to set the plugin above them and see if it makes any difference.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

> It looks like the divs are above the plugin in the z-order.  I'll
> try to set the plugin above them and see if it makes any difference.

It does -- in this case the bug doesn't happen.

So the bug here is that plugins don't receive mouse-exit events when
moving the mouse out of the plugin over an object that partially
overlaps it and has a higher z-order.

Even if this bug is Mac-only, I strongly suspect the problem is
somewhere in cross-platform code (not in widget code).  Mats, Matt, do
either of you have any idea what it might be?

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Just now I thought of a place in widget code where the problem might be.  I'll look through it later.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

I think we can now say that this bug is confirmed.

Even if no actual proof of clickjacking has been presented, and the testcase we have is both overly complex and not airtight, it's pretty clear that *someone* could use the bug I described in comment #17 to perform a cursorjack/clickjack.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

I've now discovered that this bug is caused at least partly by a bug in ChildViewMouseTracker::ViewForEvent(), here:
https://hg.mozilla.org/mozilla-central/annotate/c962bde5ac0b/widget/cocoa/nsChildView.mm#l6386

The problem is that when a non-plugin object overlaps a plugin object, -[NSView hitTest:] isn't a good way to find which NSView is under the mouse (or more precisely, which NSView the Gecko object under the mouse belongs to).

I'll be working on this.

Comment 21

[Markus Stange [:mstange]]

Can we try to find a solution where it's not necessary to duplicate Gecko's event targeting logic in widget code? For example, I'd be very happy if ChildViewMouseTracker could just always dispatch the event to the top level ChildView in the NSView hierarchy of the target window. If we do that, will Gecko automatically forward events over plugins to the target plugin?

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

So, obviously we should fix the event-related bug we're seeing w/r/t mouse events and plugins - for the sake of correctness and also clickjacking.

However - slightly OT but relevant - for the specific problem of cam/mic UI clickjacking, a bigger solution for this might be to somehow always show the cursor when that UI is displayed. That would prevent this entire class of attacks. It's what the Flash Player does when its own cam/mic UI is active. I guess I'll go about filing a different bug for that.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #21)

> Can we try to find a solution where it's not necessary to duplicate
> Gecko's event targeting logic in widget code? For example, I'd be
> very happy if ChildViewMouseTracker could just always dispatch the
> event to the top level ChildView in the NSView hierarchy of the
> target window. If we do that, will Gecko automatically forward
> events over plugins to the target plugin?

We can't just dispatch the event to the top level ChildView in the
NSView hierarchy.  Yes, Gecko will (probably) automatically forward
events over plugins to their correct targets.  But we need to know in
widget code (in ChildViewMouseTracker::ViewForEvent()) what the target
ChildView will actually be -- otherwise we won't be able to know (in
ChildViewMouseTracker::ReEvaluateMouseEnterState()) when to send a
mouse-entered or mouse-exited event.

We don't need to "duplicate Gecko's event targeting logic".  There's a
NS_QUERY_DOM_WIDGET_HITTEST event, which we're already using in
-[ChildView currentEventShouldFocusPlugin].  For a given nsIWidget, it
queries Gecko to find out whether or not a particular point "hits" the
widget -- which is all we need to know.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a patch that fixes this bug in my tests.

And here's a tryserver build that I've just started:
https://tbpl.mozilla.org/?tree=Try&rev=fba4854b0737

The results should be available in a few hours (or possibly overnight).  When it is I'll ask others to test it.

Comment 25

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #23)
> otherwise we won't be able to know (in
> ChildViewMouseTracker::ReEvaluateMouseEnterState()) when to send a
> mouse-entered or mouse-exited event.

Good point. Though I'd be surprised if Gecko still needed view-level mouse enter/exit events.

> We don't need to "duplicate Gecko's event targeting logic".  There's a
> NS_QUERY_DOM_WIDGET_HITTEST event, which we're already using in
> -[ChildView currentEventShouldFocusPlugin].  For a given nsIWidget, it
> queries Gecko to find out whether or not a particular point "hits" the
> widget -- which is all we need to know.

Oh, good.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8411323 [details] [diff] [review]
Fix


>+    nsChildView* childWidget = (nsChildView*)[childView widget];
>+    // If childView is being destroyed return nil.
>+    if (!childWidget)
>+      return nil;
>+
>+    if (![childView isPluginView])
>+      break;
>+
>+    // If childView is a plugin view, we need to ask Gecko if some other object
>+    // overlaps the part of it over which the mouse is located.
>+    nsAutoRetainCocoaObject kungFuDeathGrip(childView);
>+
>+    NSPoint eventLoc = nsCocoaUtils::ScreenLocationForEvent(aEvent);
>+    eventLoc.y = nsCocoaUtils::FlippedScreenY(eventLoc.y);
>+    LayoutDeviceIntPoint widgetLoc = LayoutDeviceIntPoint::FromUntyped(
>+      childWidget->CocoaPointsToDevPixels(eventLoc) -
>+      childWidget->WidgetToScreenOffset());
>+
>+    WidgetQueryContentEvent hitTest(true, NS_QUERY_DOM_WIDGET_HITTEST,
>+                                    childWidget);
>+    hitTest.InitForQueryDOMWidgetHittest(widgetLoc);
>+    // This might destroy our widget.
>+    childWidget->DispatchWindowEvent(hitTest);
>+    if (![childView widget])
>+      return nil;
>+    if (hitTest.mSucceeded && hitTest.mReply.mWidgetIsHit)

Can you put this part into an override of the NSView "hitTest:" method instead, and keep the existing code in ChildViewMouseTracker as-is? I'm envisioning something like

- (NSView*)hitTest:(NSPoint*)aPoint
{
  NSView* target = [super hitTest:aPoint];
  if (target == self && [self isPluginView]) {
    ...
    if (gecko says this widget is not hit) {
      return nil;
    }
  }
  return target;
}

Comment 27

[Markus Stange [:mstange]]

That would also make sure that for example mouseDown events target the right widget.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #26)

I'm not sure whether what you specifically suggest would work:  -[ChildView hitTest:] needs to return an accurate result, not just nil if the particular plugin isn't "hit".

But it *would* be "neater" to put this logic into -[ChildView hitTest:], if that isn't too much trouble.

I'll see what I can come up with.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

Actually the solution is obvious, now that I think about it:

Have -[ChildView hitTest] call [superview hitTest:] if the plugin it corresponds to isn't "hit".

Now let's see if what's obvious is also correct :-)

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Actually Markus was right (in comment #26).  But there's some weirdness I need to figure out before I can post another patch -- the processing of the NS_QUERY_DOM_WIDGET_HITTEST event sometimes fails mysteriously.

I'll continue working on this next week.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1

Thanks for your suggestion, Markus.  It allowed me to combine my fix for this bug with my old fix for bug 627649.  It also uncovered failures in the handler for NS_QUERY_DOM_WIDGET_HITTEST events that weren't apparent (for some reason) either in my earlier patch for this bug or my patch for bug 627649.

It turns out the handler was doing far more (in its Init() and InitCommon() methods) that was necessary to handle NS_QUERY_DOM_WIDGET_HITTEST events, and was failing when some of these extra calls failed.

Markus, I'd like you to review the widget code changes.  Smaug, I'm asking you to review the DOM event handler changes.

I've started a tryserver build:
https://tbpl.mozilla.org/?tree=Try&rev=7fbeadb11eb2

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

Nice! I particularly like the code removal.
I suppose this will also fix bug 896550?

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

r=masayuki for ContentEventHandler.

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm

>+- (NSView *)hitTest:(NSPoint)aPoint
>+{
>+  NSView* target = [super hitTest:aPoint];
>+  if ((target == self) && [self isPluginView] && mGeckoChild) {
>+    nsAutoRetainCocoaObject kungFuDeathGrip(self);

nit: If you put this kungFuDeathGrip only for dispatching the event, it must not be necessary because the event doesn't cause DOM event. However, the cost must be enough cheap but it guarantees the safe. Therefore, staying this line is OK.

>+
>+    NSPoint cocoaLoc = [[self superview] convertPoint:aPoint toView:self];
>+    LayoutDeviceIntPoint widgetLoc = LayoutDeviceIntPoint::FromUntyped(
>+      mGeckoChild->CocoaPointsToDevPixels(cocoaLoc));
>+
>+    WidgetQueryContentEvent hitTest(true, NS_QUERY_DOM_WIDGET_HITTEST,
>+                                    mGeckoChild);
>+    hitTest.InitForQueryDOMWidgetHittest(widgetLoc);
>+    // This might destroy our widget.
>+    mGeckoChild->DispatchWindowEvent(hitTest);
>+    if (!mGeckoChild) {
>+      return nil;
>+    }

nit: By same reason, this can be gone or replacing with NS_ENSURE_TRUE.

>+    if (hitTest.mSucceeded && !hitTest.mReply.mWidgetIsHit) {
>+      return nil;
>+    }
>+  }
>+  return target;
>+}

Comment 34

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #32)

> I suppose this will also fix bug 896550?

Yes, it does :-)

(In reply to comment #33)

> nit: If you put this kungFuDeathGrip only for dispatching the event,
> it must not be necessary because the event doesn't cause DOM event.

I don't understand.

ContentEventHandler::InitCommon() prior to my patch
(ContentEventHandler::InitBasic() with my patch) contains the
following:

  // If text frame which has overflowing selection underline is dirty,
  // we need to flush the pending reflow here.
  mPresShell->FlushPendingNotifications(Flush_Layout);

  // Flushing notifications can cause mPresShell to be destroyed (bug 577963).
  NS_ENSURE_TRUE(!mPresShell->IsDestroying(), NS_ERROR_FAILURE);

So pretty clearly processing a NS_QUERY_DOM_WIDGET_HITTEST event *can*
cause the widget it's dispatched on to be destroyed.  That's why I put
in the kungFuDeathGrip, and also why I null-checked mGeckoChild after
dispatching the event.

> nit: By same reason, this can be gone or replacing with NS_ENSURE_TRUE.

I'll use NS_ENSURE_TRUE.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #34)

> I'll use NS_ENSURE_TRUE.

Markus pointed out to me that the NS_ENSURE_* macros have been deprecated and replaced.  See bug 672843.

But what I already have fits the new style just fine, so I'll keep it and not use *any* macro.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Steven Michaud from comment #34)
> (In reply to comment #33)
> 
> > nit: If you put this kungFuDeathGrip only for dispatching the event,
> > it must not be necessary because the event doesn't cause DOM event.
> 
> I don't understand.
> 
> ContentEventHandler::InitCommon() prior to my patch
> (ContentEventHandler::InitBasic() with my patch) contains the
> following:
> 
>   // If text frame which has overflowing selection underline is dirty,
>   // we need to flush the pending reflow here.
>   mPresShell->FlushPendingNotifications(Flush_Layout);
> 
>   // Flushing notifications can cause mPresShell to be destroyed (bug
> 577963).
>   NS_ENSURE_TRUE(!mPresShell->IsDestroying(), NS_ERROR_FAILURE);
> 
> So pretty clearly processing a NS_QUERY_DOM_WIDGET_HITTEST event *can*
> cause the widget it's dispatched on to be destroyed.  That's why I put
> in the kungFuDeathGrip, and also why I null-checked mGeckoChild after
> dispatching the event.

Ah, I completely forgot the bug...

Comment 37

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b850412044d7

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b850412044d7

Comment 40

[Jordi Chancel]

thank you very much for the award!!!
I'm very happy !

Comment 41

[Jordi Chancel]

where can i download the firefox fixed?

Comment 42

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

http://nightly.mozilla.org/

Comment 43

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed bug in Fx29.
Verified fixed in Fx32, 2014-05-12.

Comment 44

[Jordi Chancel]

If i have found a regression (same vulnerability) and if i report this , can i win a new award?

Comment 45

[Jordi Chancel]

If i have found a regression (same vulnerability) on the fixed version and if i report this , can i win a new award?

Comment 46

[Matt Wobensmith [:mwobensmith][:matt:]]

We judge each bug on its own merit. Feel free to file new bugs, but don't assume they will be rated the same as this one unless we feel it's appropriate. Thank you.

Comment 47

[Jordi Chancel]

https://bugzilla.mozilla.org/show_bug.cgi?id=1009169

Comment 48

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

Is there a reason that this sec-high went in without sec-approval? being set?

We should probably take this in Aurora and Beta if this is a sec-high.

Does this affect ESR24?

Comment 49

[Steven Michaud [:smichaud] (Retired)]

> Does this affect ESR24?

Yes.  I checked both FF24.0esr and FF24.5.0esr.

Comment 50

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

We should get an ESR24 patch then.

Comment 51

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 for esr24

Carrying forward r+.

Comment 52

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Probably present since earliest days of Cocoa widgets
User impact if declined: Users will be exposed to a sec-high bug
Testing completed (on m-c, etc.): Substantial testing by myself, two weeks on m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

This patch is quite large (and removes a lot of code).  But the reasoning behind it is well understood, and no regressions have surfaced after two weeks on m-c.

Comment 53

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Users will remain exposed to a sec-high bug
Fix Landed on Version: Present on trunk (32 branch) for past two weeks
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 54

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

Oops, wrong patch.

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8423933 [details] [diff] [review]
Fix rev1 for esr24

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Users will remain exposed to a sec-high bug
Fix Landed on Version: Present on trunk (32 branch) for past two weeks
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 56

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8423933 [details] [diff] [review]
Fix rev1 for esr24

Landed on mozilla-esr24:
https://hg.mozilla.org/releases/mozilla-esr24/rev/8d6f12da582e

Comment 57

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8423933 [details] [diff] [review]
Fix rev1 for esr24

Backed this out for bustage:
https://hg.mozilla.org/releases/mozilla-esr24/rev/78d3f0aa0e15

https://tbpl.mozilla.org/?tree=Mozilla-Esr24&rev=8d6f12da582e

Next time I'll do a local build first :-)

Comment 58

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

Landed on mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/70f056b3a700

Comment 59

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

Landed on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/bae7daefa1cb

Comment 60

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 for esr24 with extra esr24-specific changes

Fixed build breakage.  Carrying forward r+ and a+.

Comment 61

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8425078 [details] [diff] [review]
Fix rev1 for esr24 with extra esr24-specific changes

Relanded on mozilla-esr:
https://hg.mozilla.org/releases/mozilla-esr24/rev/6402fb9a21cb

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/70f056b3a700

Comment 63

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed also on latest Fx30 and Fx31, and Fx24.6.0esr.

Unfortunately, it appears that we may have broken Flash mouse cursor hiding in general on Fx24.6.0esr, Mac only. I've filed a separate bug for that - bug 1021815.

Comment 64

[Steven Michaud [:smichaud] (Retired)]

> Unfortunately, it appears that we may have broken Flash mouse cursor
> hiding in general on Fx24.6.0esr, Mac only. I've filed a separate
> bug for that - bug 1021815.

I've confirmed bug 102185 by testing with today's esr24 nightly.  And
I've confirmed that my patch for this bug is the trigger by backing it
out, then seeing that the bug disappears.

Off the top of my head, I have *no* idea how my patch could have had
this effect.  Note that bug 1021815 doesn't happen on any of the other
branches where my patch for this bug landed.

I will dig around a bit, trying to find an esr24-specific fix.  But
I'm not going to make any heroic efforts.  So we need to consider 1)
backing out my patch for this bug on esr24, and leaving it unfixed
there; 2) leaving bug 1021815 unfixed.

Comment 65

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Steven, for the investigation. I'd choose option #1 personally, as I don't think the security bug is so bad as to warrant breaking legit Flash content. My concern is that ESR builds might be using enterprise apps that are built in Flash/Flex, and mouse event issues might cause them to break or be noticeably impaired. 

I'm inclined to say that this does not impede the release of Fx24.6.0esr, but others can weigh in on that.

Comment 66

[Steven Michaud [:smichaud] (Retired)]

I'm going to be spending at least another day on bug 1021815, next week.  We may get lucky -- it may turn out to be something stupid (and easily fixable).

But if we *aren't* lucky, I agree with your assessment.  Especially if you add in 1020763, it looks my patch has completely broken the passing of events to NPAPI plugins on the esr24 branch :-(  Allowing that to happen is much worse than leaving this bug unfixed in esr24.

Comment 67

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

I agree with the back out. I don't think it's suitable to ship ESR with the reported breakage. We need a back out today following which I'll kick off a new build. Matt has agreed to spoke test once the build is complete.

Al - Can you please confirm that the sec team is OK shipping this bug in ESR 24.6.0?

Ryan - Can you please assist with the back out on ESR24?

Comment 68

[Ryan VanderMeulen [:RyanVM]]

I can help with the backout once Al signs off. Please ping me on IRC then.

Comment 69

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

We should back this out on ESR.

Comment 70

[Ryan VanderMeulen [:RyanVM]]

Done.
https://hg.mozilla.org/releases/mozilla-esr24/rev/a65ce0160810

Comment 71

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 for esr24, fixed again

> it may turn out to be something stupid (and easily fixable)

It was :-(  I should have been using screen coordinates in the esr24 version of -[ChildView hitTest:].

Here's a patch that fixes bug 1021815 in my tests.  It's more difficult to tell about bug 1020763, which doesn't have good STR.  But (testing with my Debug Events Plugin from bug 441880) I did notice some weirdness clicking on a plugin that this patch clears up.

Matt, please test this patch.  If you don't see any problems I'll ask someone for a review -- probably Markus Stange.

Comment 72

[Justin Wood (:Callek)]

Gecko 30 patch didn't apply, esr24 patch had to be backed out, wontfix for SeaMonkey 2.26.1 (Based on gecko 29)

Comment 73

[Ryan VanderMeulen [:RyanVM]]

ni? Matt RE: comment 71

Comment 74

[Matt Wobensmith [:mwobensmith][:matt:]]

I built Fx24esr with your patch. Everything seems OK. The hidden mouse cases that broke before appear to be fixed. I also did some testing with a second display and YouTube video controls, as well as some scenarios with tabbing around inside SWFs. 

I say we're fine and we can check again once the patch lands. Thanks Steven!

Comment 75

[Markus Stange [:mstange]]

Comment on attachment 8437334 [details] [diff] [review]
Fix rev1 for esr24, fixed again

>+- (NSView *)hitTest:(NSPoint)aPoint
>+{
>+  NSView* target = [super hitTest:aPoint];
>+  NSWindow *window = [self window];
>+  if (window && (target == self) && [self isPluginView] && mGeckoChild) {
>+    nsAutoRetainCocoaObject kungFuDeathGrip(self);
>+
>+    NSPoint windowLoc = [[self superview] convertPoint:aPoint toView:nil];
>+    NSPoint screenLoc = [window convertBaseToScreen:windowLoc];
>+    screenLoc.y = nsCocoaUtils::FlippedScreenY(screenLoc.y);
>+    nsIntPoint widgetLoc = mGeckoChild->CocoaPointsToDevPixels(screenLoc) -
>+      mGeckoChild->WidgetToScreenOffset();
>+

Why not


+    NSPoint viewLoc = [[self superview] convertPoint:aPoint toView:self];
+    nsIntPoint widgetLoc = mGeckoChild->CocoaPointsToDevPixels(viewLoc);

?

Your code should work, too, but it seems unnecessarily complicated. But since it has already been tested and we're not going to be bothered by it on mozilla-central, it's probably better to keep what you have.

Comment 76

[Steven Michaud [:smichaud] (Retired)]

Actually that might also have worked.

But though the code is more complex, it's simpler (and safer) to put everything in screen coordinates and then subtract the widget offset.  In any case, as you've said, we don't need to make this patch as neat as possible.

Comment 77

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8437334 [details] [diff] [review]
Fix rev1 for esr24, fixed again

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: A sec-high bug will continue to effect esr24 users
Fix Landed on Version: 30
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 78

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8437334 [details] [diff] [review]
Fix rev1 for esr24, fixed again

Landed on mozilla-esr24:
https://hg.mozilla.org/releases/mozilla-esr24/rev/9400d92aa867

Comment 79

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on Fx24.6.0esrpre, 2014-06-19.