Closed Bug 979497 Opened 6 years ago Closed 5 years ago

Once DispatchPointerFromMouseOrTouch is called, aFrame can be deleted object (i.e. what should be target for the mouse events if a listener for pointer events modifies the DOM)

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox29 --- unaffected
firefox30 --- disabled
firefox38 --- disabled
firefox38.0.5 --- disabled
firefox39 --- disabled
firefox40 --- fixed
firefox-esr31 --- disabled
firefox-esr38 --- disabled
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- unaffected

People

(Reporter: smaug, Assigned: alessarik)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-other, Whiteboard: [post-critsmash-triage])

Attachments

(1 file, 19 obsolete files)

19.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Dispatching events may run scripts, so frame object can be deleted.
Group: layout-core-security, core-security
Attached file pp.html (obsolete) —
it is not very clear, what should be the right behavior here.
IE does dispatch pointerdown to element, after style = none we get mousedown dispatched to same element, then to document and window.
Blocks: pointerevent
This sounds like a use-after-free.
With latest example IE does not dispatch mousedown when child removed during poitnerevent execution
Not sec-critical because pointer events aren't enabled by default.
This is just something we have to fix before enabling them.
Hmm, perhaps this doesn't even need to be sec-bug.
Keywords: sec-critical
Attachment #8385658 - Attachment is obsolete: true
Attachment #8388791 - Flags: review?(bugs)
Keywords: sec-other
The issue is that we don't know yet what the behavior should be.
Note, the spec isn't clear yet, and we need some more testing on what IE does too.
I think jrossi was going to do it, but he's been busy with other stuff I think.

The patch makes us effectively do hittesting again, and jrossi said that dispatching
event possibly to a different target did cause some issues in some version of IE, which
is why they use the parent of the previous target in some cases. But it is not at all clear
what happens if that parent is removed too, or moved to another document etc.
Summary: Once DispatchPointerFromMouseOrTouch is called, aFrame can be deleted object → Once DispatchPointerFromMouseOrTouch is called, aFrame can be deleted object (i.e. what should be target for the mouse events if a listener for pointer events modifies the DOM)
Comment on attachment 8388791 [details] [diff] [review]
Check frame after dispatching pointer event

Someone really needs to test how this all works in IE11.
It may or may not have sane behavior.
And update https://www.w3.org/Bugs/Public/show_bug.cgi?id=24923 then.

I can't really review this before we know better what should be done here.

(I may have some time later this week to test this stuff.)
Attachment #8388791 - Flags: review?(bugs)
Or Matt, by any chance, would you have time to test IE11?
Ok, IE11 does something quite reasonable. We can do similar.
nsEventStateManager::ContentRemoved already tracks changes to DOM. So need to add something there for
pointerevents stuff.
IE does not dispatch anything to node if it has been removed during pointerEvent dispatch
no, but jrossi said mouse event is dispatched to the first ancestor which is still in the DOM.
(In reply to Olli Pettay [:smaug] from comment #12)
> no, but jrossi said mouse event is dispatched to the first ancestor which is
> still in the DOM.

Sounds legit. Is there a getAncestorStillInDOM method lying around?
Attached patch check_frame_ver2.diff (obsolete) — Splinter Review
+ Update code of test for check correct behavior

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

Looks like FireFox succesfully passed correct test without any changes in code
Attachment #8411821 - Attachment is obsolete: true
Attachment #8420984 - Flags: review?(bugs)
Attachment #8420984 - Flags: feedback?(oleg.romashin)
Attachment #8420984 - Flags: feedback?(nicklebedev37)
Comment on attachment 8420984 [details] [diff] [review]
check_frame_ver2.diff

This doesn't fix anything.
Attachment #8420984 - Flags: review?(bugs) → review-
Attached patch check_frame_ver3.diff (obsolete) — Splinter Review
+ Add solution for bug

Any comments are welcome.
Attachment #8420984 - Attachment is obsolete: true
Attachment #8420984 - Flags: feedback?(oleg.romashin)
Attachment #8420984 - Flags: feedback?(nicklebedev37)
Attachment #8423918 - Flags: review?(mbrubeck)
Attachment #8423918 - Flags: review?(bugs)
Attachment #8423918 - Flags: feedback?(oleg.romashin)
Attachment #8423918 - Flags: feedback?(nicklebedev37)
Some comments:
1. I can use queue of nsIContent (from target to root use GetParentNode())
And after dispatching pointer event, find first content IsInDoc(), and after that get GetPrimaryFrame().
But I like way to use nsIFrame with nsWeakFrames. This way more simply and have less additional expenses.
2. Unfortunately, html-test-page cannot check behavior in auto-mode. Needs manual action at the end of test.
If anybody can help with this issue, welcome.
Comment on attachment 8423918 [details] [diff] [review]
check_frame_ver3.diff

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

I'll leave this review to Smaug
Attachment #8423918 - Flags: review?(mbrubeck)
Comment on attachment 8423918 [details] [diff] [review]
check_frame_ver3.diff

The patch operates on frame tree, but
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24923#c16 operates on the DOM tree.

Also, EventStateManager observes dom mutations, so one could use that
to get the nearest in-tree event target in the tree.

And, as mentioned in the w3c bug style changes don't affect to the event path.
But style changes do affect to nsIFrames. They get destroyed when
display: none is used.
Attachment #8423918 - Flags: review?(bugs) → review-
Attached patch check_frame_ver4.diff (obsolete) — Splinter Review
+ Changes element's path related with DOM objects (according comments)
Attachment #8423918 - Attachment is obsolete: true
Attachment #8423918 - Flags: feedback?(oleg.romashin)
Attachment #8423918 - Flags: feedback?(nicklebedev37)
Attachment #8424838 - Flags: review?(bugs)
Attachment #8424838 - Flags: feedback?(nicklebedev37)
Attachment #8424838 - Flags: feedback?
Comment on attachment 8424838 [details] [diff] [review]
check_frame_ver4.diff

>+class ContentQueue : private nsCOMArray<nsIContent>
So this is a bit slow, since it always AddRefs/Releases the members

>+    nsWeakFrame weakFrame(frame);
>+    ContentQueue contentQueue;
>+    if (sPointerEventEnabled && aTargetFrame && aEvent->eventStructType == NS_POINTER_EVENT) {
>+      nsINode* parent = frame->GetContent();
>+      while (parent) {
>+        if (nsIContent* content = parent->GetParent()) {
>+          contentQueue.Push(content);
>+        }
>+        parent = parent->GetParentNode();
>+      }
>+    }
So EventStateManager gets notified when something is removed from document. Why not reuse that.
You may need to change EventStateManager::ContentRemoved to take also container as a parameter.
PresShell::ContentRemoved which calls EventStateManager::ContentRemoved has that param.
Also, we don't want to re-implement event path creation. (and just using GetParentNode() doesn't end up creating the
right path.)
Attachment #8424838 - Flags: review?(bugs) → review-
Attached patch check_frame_ver5.diff (obsolete) — Splinter Review
+ Updates: detect live frame via observe ContentRemoved

I use nsPresShell (not nsEventStateManager). But it seems, I implemented your idea about fast detection live ancestors.
Some code maybe seems raw, unfortunately, but comments are welcome.
Attachment #8424838 - Attachment is obsolete: true
Attachment #8424838 - Flags: feedback?(nicklebedev37)
Attachment #8424838 - Flags: feedback?
Attachment #8427115 - Flags: review?(bugs)
Attachment #8427115 - Flags: feedback?(nicklebedev37)
Attachment #8427115 - Flags: feedback?
Attachment #8427115 - Flags: feedback? → feedback?(oleg.romashin)
Comment on attachment 8427115 [details] [diff] [review]
check_frame_ver5.diff

>+    nsWeakFrame weakFrame(frame);
>+
>+    nsresult rv;
>     if (shell != this) {
>       // Handle the event in the correct shell.
>       // Prevent deletion until we're done with event handling (bug 336582).
>       nsCOMPtr<nsIPresShell> kungFuDeathGrip(shell);
>       // We pass the subshell's root frame as the frame to start from. This is
>       // the only correct alternative; if the event was captured then it
>       // must have been captured by us or some ancestor shell and we
>       // now ask the subshell to dispatch it normally.
>-      return shell->HandlePositionedEvent(frame, aEvent, aEventStatus);
>-    }
>-
>-    return HandlePositionedEvent(frame, aEvent, aEventStatus);
>+      if (sPointerEventEnabled && aTargetFrame && aEvent->eventStructType == NS_POINTER_EVENT) {
>+        shell->mLiveFrame = frame;
>+      }
>+      rv = shell->HandlePositionedEvent(frame, aEvent, aEventStatus);
>+
>+      if (sPointerEventEnabled && aTargetFrame && aEvent->eventStructType == NS_POINTER_EVENT) {
>+        if (!weakFrame.IsAlive() && mLiveFrame) {
>+          *aTargetFrame = mLiveFrame;
So aTargetFrame may now point to a deleted nsIFrame object.
What if no elements were removed from dom, but layout was just changed.
In that case PresShell::ContentRemoved isn't called. Also, event dispatch shouldn't rely on
the layout tree for the legacy mouse/touch events in case pointer event changes the layout.


>+          mLiveFrame = nullptr;
>+        }
>+      }
>+      return rv;
>+    }
>+
>+    if (sPointerEventEnabled && aTargetFrame && aEvent->eventStructType == NS_POINTER_EVENT) {
>+      mLiveFrame = frame;
>+    }
>+    rv = HandlePositionedEvent(frame, aEvent, aEventStatus);
>+
>+    if (sPointerEventEnabled && aTargetFrame && aEvent->eventStructType == NS_POINTER_EVENT) {
>+      if (!weakFrame.IsAlive() && mLiveFrame) {
>+        *aTargetFrame = mLiveFrame;
>+        mLiveFrame = nullptr;
>+      }
>+    }
>+
>+    return rv;
>   }
> 
>   nsresult rv = NS_OK;
> 
>   if (frame) {
>     PushCurrentEventInfo(nullptr, nullptr);
> 
>     // key and IME related events go to the focused frame in this DOM window.
>diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
>--- a/layout/base/nsPresShell.h
>+++ b/layout/base/nsPresShell.h
>@@ -184,17 +184,18 @@ public:
> 
>   //nsIViewObserver interface
> 
>   virtual void Paint(nsView* aViewToPaint, const nsRegion& aDirtyRegion,
>                      uint32_t aFlags) MOZ_OVERRIDE;
>   virtual nsresult HandleEvent(nsIFrame* aFrame,
>                                mozilla::WidgetGUIEvent* aEvent,
>                                bool aDontRetargetEvents,
>-                               nsEventStatus* aEventStatus) MOZ_OVERRIDE;
>+                               nsEventStatus* aEventStatus,
>+                               nsIFrame** aTargetFrame) MOZ_OVERRIDE;
>   virtual NS_HIDDEN_(nsresult) HandleDOMEventWithTarget(
>                                  nsIContent* aTargetContent,
>                                  mozilla::WidgetEvent* aEvent,
>                                  nsEventStatus* aStatus) MOZ_OVERRIDE;
>   virtual NS_HIDDEN_(nsresult) HandleDOMEventWithTarget(nsIContent* aTargetContent,
>                                                         nsIDOMEvent* aEvent,
>                                                         nsEventStatus* aStatus) MOZ_OVERRIDE;
>   virtual bool ShouldIgnoreInvalidation() MOZ_OVERRIDE;
>@@ -811,11 +812,13 @@ protected:
>   bool                      mAsyncResizeTimerIsActive : 1;
>   bool                      mInResize : 1;
> 
>   bool                      mImageVisibilityVisited : 1;
> 
>   bool                      mNextPaintCompressed : 1;
> 
>   static bool               sDisableNonTestMouseEvents;
>+
>+  nsIFrame*                 mLiveFrame;
> };
> 
> #endif /* !defined(nsPresShell_h_) */
>diff --git a/layout/base/tests/bug979497_inner.html b/layout/base/tests/bug979497_inner.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/base/tests/bug979497_inner.html
>@@ -0,0 +1,117 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=979497
>+-->
>+<head>
>+  <title>Test for Bug 979497</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=979497">Mozilla Bug 979497</a>
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+
>+</div>
>+<pre id="test">
>+<script type="application/javascript">
>+
>+/** Test for Bug 979497 **/
>+
>+function ok(condition, msg) {
>+  parent.ok(condition, msg);
>+}
>+
>+function is(a, b, msg) {
>+  parent.is(a, b, msg);
>+}
>+
>+function runTests() {
>+  var killer = document.getElementById('killer');
>+  var div = killer.parentNode.parentNode;
>+  var enableTest = false;
>+  var hitPointerDown = false;
>+  var hitMouseDown = false;
>+  var hitPointerDownParent = false;
>+  var hitMouseDownParent = false;
>+  var hitMouseDownWindow = false;
>+  killer.addEventListener('pointerdown', function(e) {
>+    enableTest = true;
>+    hitPointerDown = true;
>+    e.stopPropagation();
>+    console.log('pointerdown fired');
>+    e.target.parentNode.parentNode.removeChild(e.target.parentNode);
>+  }, false);
>+  killer.addEventListener('mousedown', function(e) {
>+    if(enableTest) {
>+      hitMouseDown = true;
>+      e.stopPropagation();
>+      console.log('mousedown fired');
>+    }
>+  }, false);
>+  div.addEventListener('pointerdown', function(e) {
>+    if(enableTest) {
>+      hitPointerDownParent = true;
>+      e.stopPropagation();
>+      console.log('pointerdown on parent div fired');
>+    }
>+  }, false);
>+  div.addEventListener('mousedown', function(e) {
>+    if(enableTest) {
>+      hitMouseDownParent = true;
>+      e.stopPropagation();
>+      console.log('mousedown on parent div fired');
>+    }
>+  }, false);
>+  window.addEventListener('mousedown', function(e) {
>+    if(enableTest) {
>+      hitMouseDownWindow = true;
>+      console.log('mousedown on window fired');
>+    }
>+  }, false);
>+
>+  synthesizeMouse(killer.parentNode, 3, 3, {type: "mousedown"});
>+  synthesizeMouse(killer, 3, 3, {type: "mousedown"});
>+
>+  is(enableTest, true, "Test should be enable");
>+  is(hitPointerDown, true, "PointerDown should be triggered before delete frame");
>+  is(hitMouseDown, false, "MouseDown should NOT be triggered for deleted frame");
>+  is(hitPointerDownParent, false, "PointerDown should NOT be triggered for parent of deleted frame");
>+  is(hitMouseDownParent, true, "MouseDown should be triggered for parent of deleted frame");
>+  is(hitMouseDownWindow, false, "MouseDown should NOT be triggered for window");
>+
>+  finishTest();
>+}
>+
>+function finishTest() {
>+  // Let window.onerror have a chance to fire
>+  setTimeout(function() {
>+    setTimeout(function() {
>+      window.parent.postMessage("SimpleTest.finish();", "*");
>+    }, 5000);
>+  }, 5000);
>+}
>+
>+window.onload = function () {
>+  SpecialPowers.pushPrefEnv({
>+    "set": [
>+      ["dom.w3c_pointer_events.enabled", true],
>+    ]
>+  }, runTests);
>+}
>+
>+SimpleTest.waitForExplicitFinish();
>+
>+</script>
>+</pre>
>+<div id="div">
>+  <select>
>+    <option>Normal option</option>
>+    <option id='killer'>Hit me to kill FireFox</option>
>+    <option>Normal option</option>
>+  </select>
>+</div>
>+</body>
>+</html>
>diff --git a/layout/base/tests/mochitest.ini b/layout/base/tests/mochitest.ini
>--- a/layout/base/tests/mochitest.ini
>+++ b/layout/base/tests/mochitest.ini
>@@ -202,16 +202,18 @@ skip-if = (buildapp == 'b2g' && toolkit 
> [test_bug842853.html]
> [test_bug842853-2.html]
> [test_bug849219.html]
> [test_bug851485.html]
> [test_bug851445.html]
> support-files = bug851445_helper.html
> [test_bug970964.html]
> support-files = bug970964_inner.html
>+[test_bug979497.html]
>+support-files = bug979497_inner.html
> [test_emulateMedium.html]
> [test_getClientRects_emptytext.html]
> [test_bug858459.html]
> skip-if = toolkit == "gonk" || buildapp == 'b2g' #Bug 931116, b2g desktop specific, initial triage
> 
> # Tests for bugs 441782, 467672 and 570378 do not pass reliably on Windows,
> # because of bug 469208.
> [test_bug332655-1.html]
>diff --git a/layout/base/tests/test_bug979497.html b/layout/base/tests/test_bug979497.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/base/tests/test_bug979497.html
>@@ -0,0 +1,35 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=979497
>+-->
>+<head>
>+  <title>Test for Bug 979497</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <script type="text/javascript;version=1.7">
>+    function setRemoteFrame() {
>+      var iframe = document.getElementById("testFrame");
>+      iframe.src = "bug979497_inner.html";
>+
>+      function messageListener(event) {
>+        eval(event.data);
>+      }
>+
>+      window.addEventListener("message", messageListener, false);
>+    }
>+
>+    function runTest() {
>+      SimpleTest.waitForExplicitFinish();
>+
>+      SpecialPowers.pushPrefEnv({
>+        "set": [
>+          ["dom.w3c_pointer_events.enabled", true]
>+        ]
>+      }, setRemoteFrame);
>+    }
>+  </script>
>+</head>
>+<body onload="runTest();">
>+  <iframe id="testFrame" height="500" width="500"></iframe>
>+</body>
Attachment #8427115 - Flags: review?(bugs)
Attachment #8427115 - Flags: review-
Attachment #8427115 - Flags: feedback?(oleg.romashin)
Attachment #8427115 - Flags: feedback?(nicklebedev37)
And don't hesitate to ask review for a new patch. I'll try to look at it even during this
vacation-ish.
Attached patch check_frame_ver6.diff (obsolete) — Splinter Review
+ Changes: live nsIFrame* changed to live nsIContent*
Attachment #8427115 - Attachment is obsolete: true
Attachment #8437729 - Flags: review?(bugs)
Attachment #8437729 - Flags: feedback?(oleg.romashin)
Attachment #8437729 - Flags: feedback?(nicklebedev37)
Comment on attachment 8437729 [details] [diff] [review]
check_frame_ver6.diff

> 
>   mPaintingIsFrozen = false;
>+
>+  mLiveContent = nullptr;
Since mLiveContent should be nsCOMPtr, no need to initialize to nullptr here.


>@@ -4496,18 +4498,25 @@ PresShell::ContentRemoved(nsIDocument *a
>   } else {
>     oldNextSibling = nullptr;
>   }
> 
>   if (aContainer && aContainer->IsElement()) {
>     mPresContext->RestyleManager()->
>       RestyleForRemove(aContainer->AsElement(), aChild, oldNextSibling);
>   }
> 
>+  // Before removing aChild from tree we should save live ancestor
well, aChild has been removed already, so

> {
>   uint32_t pointerMessage = 0;
>   if (aEvent->eventStructType == NS_MOUSE_EVENT) {
>     WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent();
>     // if it is not mouse then it is likely will come as touch event
>     if (!mouseEvent->convertToPointer) {
>       return NS_OK;
>     }
>     int16_t button = mouseEvent->button;
>     switch (mouseEvent->message) {
>+    case NS_MOUSE_BUTTON_DOWN:
>+      pointerMessage = NS_POINTER_DOWN;
>+      break;
>     case NS_MOUSE_MOVE:
>       if (mouseEvent->buttons == 0) {
>         button = -1;
>       }
>       pointerMessage = NS_POINTER_MOVE;
>       break;
>     case NS_MOUSE_BUTTON_UP:
>       pointerMessage = NS_POINTER_UP;
>       break;
>-    case NS_MOUSE_BUTTON_DOWN:
>-      pointerMessage = NS_POINTER_DOWN;
>-      break;
Unrelated changes. Could you not do in this bug.


>     // loop over all touches and dispatch pointer events on each touch
>     // copy the event
>     switch (touchEvent->message) {
>+    case NS_TOUCH_START:
>+      pointerMessage = NS_POINTER_DOWN;
>+      break;
>     case NS_TOUCH_MOVE:
>       pointerMessage = NS_POINTER_MOVE;
>       break;
>     case NS_TOUCH_END:
>       pointerMessage = NS_POINTER_UP;
>       break;
>-    case NS_TOUCH_START:
>-      pointerMessage = NS_POINTER_DOWN;
>-      break;
ditto
 
>+  NS_ASSERTION(aFrame, "aFrame should be not null");
>+
>   if (sPointerEventEnabled) {
>-    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus);
>-  }
>-
>-  NS_ASSERTION(aFrame, "null frame");
>+    nsWeakFrame weakFrame(aFrame);
>+    nsIContent* targetContent = nullptr;
>+    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus, &targetContent);
>+    MOZ_ASSERT(weakFrame.IsAlive() || (targetContent && targetContent->GetPrimaryFrame()),
>+               "aFrame or targetContent should be live");
Nothing guarantees targetContent->GetPrimaryFrame() returns non-null here. targetContent is possibly in DOM tree,
but has been restyled to display: none;, for example.



>+    if (!weakFrame.IsAlive()) {
>+      if (targetContent) {
>+        aFrame = targetContent->GetPrimaryFrame();
>+      } else {
>+        return NS_OK;
>+      }
>+    }
>+  }


>   bool                      mImageVisibilityVisited : 1;
> 
>   bool                      mNextPaintCompressed : 1;
> 
>   static bool               sDisableNonTestMouseEvents;
>+
>+  nsIContent*               mLiveContent;
This must be nsCOMPtr, otherwise nothing makes sure this stays alive.


Getting closer, but relies still on layout tree to be there when dispatching legacy events.
It is possible that there is only DOM tree, and layout stuff is away because of display: none or such.
Attachment #8437729 - Flags: review?(bugs) → review-
Attached patch check_frame_ver7.diff (obsolete) — Splinter Review
+ Changes: nsCOMPtr<nsIContent> mLiveContent
+ Changes: Deleted unwanted code
+ Changes: Added check if GetPrimareFrame returns nullptr
Attachment #8437729 - Attachment is obsolete: true
Attachment #8437729 - Flags: feedback?(oleg.romashin)
Attachment #8437729 - Flags: feedback?(nicklebedev37)
Attachment #8438390 - Flags: review?(bugs)
Attachment #8438390 - Flags: feedback?(oleg.romashin)
Attachment #8438390 - Flags: feedback?(nicklebedev37)
(In reply to Olli Pettay [:smaug] from comment #31)
> >   if (aContainer && aContainer->IsElement()) {
> >     mPresContext->RestyleManager()->
> >       RestyleForRemove(aContainer->AsElement(), aChild, oldNextSibling);
> >   }
> >+  // Before removing aChild from tree we should save live ancestor
> well, aChild has been removed already, so
As far as I understand, aChild will be removed after this code in 
>   bool didReconstruct;
>   mFrameConstructor->ContentRemoved(aContainer, aChild, oldNextSibling,
>                                     nsCSSFrameConstructor::REMOVE_CONTENT,
>                                     &didReconstruct);

> >+    nsWeakFrame weakFrame(aFrame);
> >+    nsIContent* targetContent = nullptr;
> >+    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus, &targetContent);
> >+    MOZ_ASSERT(weakFrame.IsAlive() || (targetContent && targetContent->GetPrimaryFrame()),
> >+               "aFrame or targetContent should be live");
> Nothing guarantees targetContent->GetPrimaryFrame() returns non-null here.
> targetContent is possibly in DOM tree,
> but has been restyled to display: none;, for example.
During testing I cannot find way how we can get such case.
In all cases GetPrimaryFrame returns non nullptr
(In reply to Maksim Lebedev from comment #33)
> > Nothing guarantees targetContent->GetPrimaryFrame() returns non-null here.
> > targetContent is possibly in DOM tree,
> > but has been restyled to display: none;, for example.
> During testing I cannot find way how we can get such case.
> In all cases GetPrimaryFrame returns non nullptr
GetPrimaryFrame may return nullptr.
If you make Element or some of its parents display: none; and force a layout flush, the frame shouldn't be there
(In reply to Maksim Lebedev from comment #33)
> (In reply to Olli Pettay [:smaug] from comment #31)
> > >   if (aContainer && aContainer->IsElement()) {
> > >     mPresContext->RestyleManager()->
> > >       RestyleForRemove(aContainer->AsElement(), aChild, oldNextSibling);
> > >   }
> > >+  // Before removing aChild from tree we should save live ancestor
> > well, aChild has been removed already, so
> As far as I understand, aChild will be removed after this 

Well, there isn't parent->child link anymore.
(but child->parent is still there)
See, nsINode::doRemoveChildAt
So aChild isn't technically in the tree anymore.
(In reply to Olli Pettay [:smaug] from comment #35)
> If you make Element or some of its parents display: none; and force a layout
> flush, the frame shouldn't be there
In test I have such code:
>killer.addEventListener('pointerdown', function(e) {
>  var t = e.target.parentNode.parentNode;
>  e.target.parentNode.parentNode.removeChild(e.target.parentNode);
>  t.style.display = "none";
>  t.parentNode.style.display = "none";
>  document.body.style.display = "none";
>}
But I cannot get case when mLiveContent->GetPrimaryFrame returns nullptr.
How I should force a layout flush?
(In reply to Olli Pettay [:smaug] from comment #36)
> Well, there isn't parent->child link anymore.
> (but child->parent is still there)
> See, nsINode::doRemoveChildAt
> So aChild isn't technically in the tree anymore.
I can propose this:
>nsINode::doRemoveChildAt(uint32_t aIndex, bool aNotify,
>                         nsIContent* aKid, nsAttrAndChildArray& aChildArray)
>{
>...
>  aChildArray.RemoveChildAt(aIndex);
>  if (doc) {
>    if (nsIPresShell* shell = doc->GetShell()) {
>      shell->NotifyContentRemoved(this, aKid);
>    }
>  }
>  if (aNotify) {
>    nsNodeUtils::ContentRemoved(this, aKid, aIndex, previousSibling);
>  }
>...
>}
I don't understand what that comment is about. Why would you explicitly notify shell right before
doing the generic nsNodeUtils::ContentRemoved(this, aKid, aIndex, previousSibling);.

(And that still doesn't change the fact that aChildArray.RemoveChildAt(aIndex); has been called already.)
But that doesn't affect to the patch in this bug. My comment was just about 
" Before removing aChild from tree we should save live ancestor" being slightly wrong comment.
Comment on attachment 8438390 [details] [diff] [review]
check_frame_ver7.diff


>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>--- a/layout/base/nsIPresShell.h
>+++ b/layout/base/nsIPresShell.h
>@@ -1360,19 +1360,20 @@ public:
>     PAINT_LAYERS = 0x01,
>     /* Composite layers to the window. */
>     PAINT_COMPOSITE = 0x02,
>   };
>   virtual void Paint(nsView* aViewToPaint, const nsRegion& aDirtyRegion,
>                      uint32_t aFlags) = 0;
>   virtual nsresult HandleEvent(nsIFrame* aFrame,
>                                mozilla::WidgetGUIEvent* aEvent,
>                                bool aDontRetargetEvents,
>-                               nsEventStatus* aEventStatus) = 0;
>+                               nsEventStatus* aEventStatus,
>+                               nsIContent** aTargetContent = nullptr) = 0;
You're changing an interface so its IID should be updated.

>+  // Before removing aChild from tree we should save live ancestor
Update this comment a bit, since aChild has been remove from doc already.

>-  NS_ASSERTION(aFrame, "null frame");
>+    nsWeakFrame weakFrame(aFrame);
>+    nsIContent* targetContent = nullptr;
>+    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus, &targetContent);
>+    MOZ_ASSERT(weakFrame.IsAlive() || (targetContent && targetContent->GetPrimaryFrame()),
>+               "aFrame or targetContent should be live");
>+    if (!weakFrame.IsAlive()) {
>+      while (targetContent) {
>+        aFrame = targetContent->GetPrimaryFrame();
>+        if (aFrame) {
>+          break;
>+        } else {
>+          targetContent = targetContent->GetParent();
>+        }
>+      }
I don't understand this while loop. Per spec we should not be looking at objects in the layout tree, but
just dispatch to the nearest ancestor in the DOM tree. It doesn't matter whether or not the ancestor has a primary frame.


>+  // Information about live content (which still stay in DOM tree).
>+  // Used in case we need re-dispatch event after sending pointer event,
>+  // when target of pointer event was deleted during execute user handlers
>+  nsCOMPtr<nsIContent>      mLiveContent;
This should have some other name. LiveContent is very generic.
Perhaps mTargetForPointerEvent.
Attachment #8438390 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #40)
> I don't understand this while loop. Per spec we should not be looking at
> objects in the layout tree, but
> just dispatch to the nearest ancestor in the DOM tree. It doesn't matter
> whether or not the ancestor has a primary frame.
PresShell::HandleEvent uses nsIFrame, we save info as nsIContent,
that's why we should transform targetContent to aFrame via function GetPrimaryFrame.
Some of nsIContent maybe have no frame (for example, after content.style.display = "none"),
that's why we should use loop to get nearest ancestor with not-null nsIFrame.
The spec (or at least https://www.w3.org/Bugs/Public/show_bug.cgi?id=24923) requires use of DOM tree, not layout when searching for the new target for the mouse events after the target for the
pointer event has gone away from DOM tree.
Attached patch check_frame_ver8.diff (obsolete) — Splinter Review
+ Changes: IID of nsIPresShell was changed
+ Changes: new function nsIPresShell::NotifyContentRemoved()
+ Changes: mLiveContent -> mPointerEventTarget
Attachment #8438390 - Attachment is obsolete: true
Attachment #8438390 - Flags: feedback?(oleg.romashin)
Attachment #8438390 - Flags: feedback?(nicklebedev37)
Attachment #8440691 - Flags: review?(bugs)
Attachment #8440691 - Flags: feedback?(oleg.romashin)
Attachment #8440691 - Flags: feedback?(nicklebedev37)
(In reply to Olli Pettay [:smaug] from comment #42)
> The spec (or at least https://www.w3.org/Bugs/Public/show_bug.cgi?id=24923)
> requires use of DOM tree, not layout when searching for the new target for
> the mouse events after the target for the
> pointer event has gone away from DOM tree.
If we use code like:
> >+  DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus, &targetContent);
> >+  MOZ_ASSERT(weakFrame.IsAlive() || targetContent, "aFrame or targetContent should be live");
> >+  if (!weakFrame.IsAlive()) {
> >+    aFrame = targetContent->GetPrimaryFrame();
> >+  }
we could get case, when aFrame equals nullptr
(In reply to Maksim Lebedev from comment #44)
> we could get case, when aFrame equals nullptr
So? We need to use DOM tree, and not care about layout tree if there isn't such.
Yes, it is odd setup, but that is what IE has, I been told, and that is what the spec will have too.
Comment on attachment 8440691 [details] [diff] [review]
check_frame_ver8.diff


>+  if (doc) {
>+    if (nsIPresShell* shell = doc->GetShell()) {
>+      shell->NotifyContentRemoved(this, aKid);
>+    }
>+  }
I don't understand this.
nsNodeUtils::ContentRemoved will call shell->ContentRemoved

>   /**
>+   * Notification sent by a node informing the pres shell that a child content was removed from tree
>+   */
>+  virtual void NotifyContentRemoved(nsINode* aContainer, nsIContent* aChild) = 0;
No need for this

>+PresShell::NotifyContentRemoved(nsINode* aContainer, nsIContent* aChild)
>+{
>+  // After removing aChild from tree we should save information about live ancestor
>+  if (mPointerEventTarget && aChild) {
>+    if (nsContentUtils::ContentIsDescendantOf(mPointerEventTarget, aChild)) {
>+      if (aContainer && aContainer->IsContent()) {
>+        mPointerEventTarget = aContainer->AsContent();
>+      } else {
>+        mPointerEventTarget = nullptr;
>+      }
>+    }
>+  }
You had this in ContentRemoved and it was mostly just fine, only the comment about
"After removing" was slightly wrong.

>+    nsWeakFrame weakFrame(aFrame);
>+    nsIContent* targetContent = nullptr;
>+    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus, &targetContent);
>+    MOZ_ASSERT(weakFrame.IsAlive() || targetContent, "aFrame or targetContent should be live");
>+    if (!weakFrame.IsAlive()) {
>+      while (targetContent) {
>+        aFrame = targetContent->GetPrimaryFrame();
>+        if (aFrame) {
>+          break;
>+        } else {
>+          targetContent = targetContent->GetParent();
>+        }
>+      }
So this is still against the spec.
We need to use the closest ancestor which is still in DOM tree, and not care about layout tree.
Attachment #8440691 - Flags: review?(bugs)
Attachment #8440691 - Flags: review-
Attachment #8440691 - Flags: feedback?(oleg.romashin)
Attachment #8440691 - Flags: feedback?(nicklebedev37)
(In reply to Olli Pettay [:smaug] from comment #47)
> I don't understand this.
> nsNodeUtils::ContentRemoved will call shell->ContentRemoved
nsNodeUtils::ContentRemoved will be called only if aNotify is true
And you have a case when it is not true?
Attached patch check_frame_ver9.diff (obsolete) — Splinter Review
- Changes: deleted PresShell::NotifyContentRemoved()
- Changes: deleted "while" when determining aFrame
Attachment #8440691 - Attachment is obsolete: true
Attachment #8442025 - Flags: review?(bugs)
Attachment #8442025 - Flags: feedback?(oleg.romashin)
Attachment #8442025 - Flags: feedback?(nicklebedev37)
(In reply to Olli Pettay [:smaug] from comment #49)
> And you have a case when it is not true?
I did not get such case, but there are many function with call notify as false:
> static void BlastSubtreeToPieces(nsINode *aNode)
> nsIContent* SVGUseElement::CreateAnonymousContent()
> NS_IMETHODIMP nsXULContextMenuBuilder::UndoAddSeparator()
> nsresult XULDocument::OverlayForwardReference::Merge(...)
Sure, but those aren't relevant here.
Comment on attachment 8442025 [details] [diff] [review]
check_frame_ver9.diff


> 
>-//a4e5ff3a-dc5c-4b3a-a625-d164a9e50619
>+//4e1bb03f-fbfa-4455-a625-d164a9e50619
> #define NS_IPRESSHELL_IID \
>-{ 0xa4e5ff3a, 0xdc5c, 0x4b3a, \
>+{ 0x4e1bb03f, 0xfbfa, 0x4455, \
>   {0xa6, 0x25, 0xd1, 0x64, 0xa9, 0xe5, 0x06, 0x19}}

you could have updated the whole thing ;) 
http://mozilla.pettay.fi/cgi-bin/mozuuid.pl
give you both forms

>+  // After removing aChild from tree we should save information about live ancestor
>+  if (mPointerEventTarget && aChild) {
No need to null check aChild here.
>+    nsWeakFrame weakFrame(aFrame);
>+    nsIContent* targetContent = nullptr;
Make this nsCOMPtr<nsIContent>


>+    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus, &targetContent);
and this should get ... getter_AddRefs(targetContent)

>+    MOZ_ASSERT(weakFrame.IsAlive() || targetContent, "aFrame or targetContent should be live");
>+    if (!weakFrame.IsAlive()) {
>+      if (targetContent) {
>+        aFrame = targetContent->GetPrimaryFrame();
>+      } else {
>+        return NS_OK;
>+      }
>+    }
So we don't need nsWeakFrame anymore, but can just check if targetContent is set?
But how does rest of the method deal with possible null aFrame (which can happen after aFrame = targetContent->GetPrimaryFrame())?.


>+      rv = shell->HandlePositionedEvent(frame, aEvent, aEventStatus);
>+    } else {
>+      rv = HandlePositionedEvent(frame, aEvent, aEventStatus);
>+    }
>+    
>+    // After HandlePositionedEvent we should reestablish content (which still live in tree) in some cases
>+    if (sPointerEventEnabled && aTargetContent && aEvent->eventStructType == NS_POINTER_EVENT) {
>+      if (!weakFrame.IsAlive()) {
>+        *aTargetContent = shell->mPointerEventTarget;
>+        shell->mPointerEventTarget = nullptr;
Since *aTargetContent should be now refcounted, 
shell->mPointerEventTarget.swap(aTargetContent); would work here.



Getting closer...
Attachment #8442025 - Flags: review?(bugs)
Attachment #8442025 - Flags: review-
Attachment #8442025 - Flags: feedback?(oleg.romashin)
Attachment #8442025 - Flags: feedback?(nicklebedev37)
(In reply to Olli Pettay [:smaug] from comment #53)
> >+    MOZ_ASSERT(weakFrame.IsAlive() || targetContent, "aFrame or targetContent should be live");
> >+    if (!weakFrame.IsAlive()) {
> >+      if (targetContent) {
> >+        aFrame = targetContent->GetPrimaryFrame();
> >+      } else {
> >+        return NS_OK;
> >+      }
> >+    }
> So we don't need nsWeakFrame anymore, but can just check if targetContent is set?
If we check only targetContent, we can get chance then targetContent will be null, but aFrame would be deleted.
I think that double check is more safety for our deal. Please, let me know your opinion.
Attached patch check_frame_ver10.diff (obsolete) — Splinter Review
+ Changes: nsIContent* targetContent -> nsCOMPtr<nsIContent> targetContent
Attachment #8442025 - Attachment is obsolete: true
Attachment #8446462 - Flags: review?(bugs)
Attachment #8446462 - Flags: feedback?(oleg.romashin)
Attachment #8446462 - Flags: feedback?(nicklebedev37)
(In reply to Olli Pettay [:smaug] from comment #53)
> But how does rest of the method deal with possible null aFrame (which can
> happen after aFrame = targetContent->GetPrimaryFrame())?.
I have two case of behavior.

First case: After deleting node in pointerdown handler, I set parent.style.display as 'none'.
In this case aFrame will be nullptr. And several string below, some code will be execute:
>    NS_WARN_IF_FALSE(frame, "Nothing to handle this event!");
>    if (!frame)
>      return NS_OK;
As result we have that corresponding mousedown have not been received by parent elements.

Second case: After deleting node in pointerdown handler, I leave parent.style.display as visible.
In this case aFrame will be not null. And several string below, some code will be execute:
>  NS_ASSERTION(capturingContent->GetCurrentDoc() == GetDocument(), "Unexpected document");
But as result we can detect corresponding mousedown event in parent elements.
Attached patch check_frame_ver11.diff (obsolete) — Splinter Review
+ Changes: Added sending corresponding event, even if aFrame is null
Attachment #8446462 - Attachment is obsolete: true
Attachment #8446462 - Flags: review?(bugs)
Attachment #8446462 - Flags: feedback?(oleg.romashin)
Attachment #8446462 - Flags: feedback?(nicklebedev37)
Attachment #8447296 - Flags: review?(bugs)
Attachment #8447296 - Flags: feedback?(oleg.romashin)
Attachment #8447296 - Flags: feedback?(nicklebedev37)
(In reply to Maksim Lebedev from comment #59)
> https://tbpl.mozilla.org/?tree=Try&rev=30d641756eae
This is not green.
Comment on attachment 8447296 [details] [diff] [review]
check_frame_ver11.diff

>     // 1. Give event to event manager for pre event state changes and
>     //    generation of synthetic events.
>-    rv = manager->PreHandleEvent(mPresContext, aEvent, mCurrentEventFrame, aStatus);
>+    if (mCurrentEventFrame) {
>+      rv = manager->PreHandleEvent(mPresContext, aEvent, mCurrentEventFrame, aStatus);
>+    }
So in order to break existing  behavior less, in case pointer events are enabled, ESM needs to handle
the events
>-          nsCOMPtr<nsINode> eventTarget = mCurrentEventContent.get();
>-          nsPresShellEventCB* eventCBPtr = &eventCB;
>-          if (!eventTarget) {
>-            nsCOMPtr<nsIContent> targetContent;
>-            if (mCurrentEventFrame) {
>-              rv = mCurrentEventFrame->
>-                     GetContentForEvent(aEvent, getter_AddRefs(targetContent));
>-            }
>-            if (NS_SUCCEEDED(rv) && targetContent) {
>-              eventTarget = do_QueryInterface(targetContent);
>-            } else if (mDocument) {
>-              eventTarget = do_QueryInterface(mDocument);
>-              // If we don't have any content, the callback wouldn't probably
>-              // do nothing.
>-              eventCBPtr = nullptr;
>-            }
>-          }
>-          if (eventTarget) {
>-            if (aEvent->eventStructType == NS_COMPOSITION_EVENT ||
>-                aEvent->eventStructType == NS_TEXT_EVENT) {
>-              IMEStateManager::DispatchCompositionEvent(eventTarget,
>-                mPresContext, aEvent, aStatus, eventCBPtr);
>-            } else {
>-              EventDispatcher::Dispatch(eventTarget, mPresContext,
>-                                        aEvent, nullptr, aStatus, eventCBPtr);
>-            }
>-          }
>+          DispatchEvent(aEvent, aStatus, &eventCB);
I don't know why you move the code to DispatchEvent. Looks like an unrelated change.

>       // 3. Give event to event manager for post event state changes and
>       //    generation of synthetic events.
>       if (!mIsDestroying && NS_SUCCEEDED(rv)) {
>         rv = manager->PostHandleEvent(mPresContext, aEvent,
So with your patch we might call ESM::PostHandleEvent, but not PreHandleEvent :/

getting closer.
Attachment #8447296 - Flags: review?(bugs)
Attachment #8447296 - Flags: review-
Attachment #8447296 - Flags: feedback?(oleg.romashin)
Attachment #8447296 - Flags: feedback?(nicklebedev37)
Attached patch check_frame_ver12.diff (obsolete) — Splinter Review
+ Changes: Added posibilities to EventStateManager::PreHandleEvent with null aFrame
- Changes: Removed unwanted code
Attachment #8447296 - Attachment is obsolete: true
Attachment #8450151 - Flags: review?(bugs)
Attachment #8450151 - Flags: feedback?(oleg.romashin)
Attachment #8450151 - Flags: feedback?(nicklebedev37)
(In reply to Olli Pettay [:smaug] from comment #61)
> So in order to break existing  behavior less, in case pointer events are
> enabled, ESM needs to handle the events
> So with your patch we might call ESM::PostHandleEvent, but not PreHandleEvent :/
Looks like, more test were fails. Maybe correct way is not calls both functions
(I mean not call ESM::PreHandleEvent and not call ESM::PostHandleEvent). ?!
Well, if we don't call ESM::PreHandleEvent and ESM::PostHandleEvent, we may not handle the mouse
events properly, yet those are dispatched to DOM.

(and I know this is a tricky bug)
Comment on attachment 8450151 [details] [diff] [review]
check_frame_ver12.diff

># HG changeset patch
># User Maksim Lebedev <alessarik@gmail.com>
># Parent 9524ac6dcf21fe3ba5ef4cc56653bbd2194cd7e6
>Bug 979497 - Once DispatchPointerFromMouseOrTouch is called, aFrame can be deleted. r=smaug
>
>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
>--- a/dom/events/EventStateManager.cpp
>+++ b/dom/events/EventStateManager.cpp
>@@ -464,34 +464,46 @@ EventStateManager::OnStopObservingConten
>   aIMEContentObserver->DisconnectFromEventStateManager();
>   NS_ENSURE_TRUE_VOID(mIMEContentObserver == aIMEContentObserver);
>   mIMEContentObserver = nullptr;
> }
> 
> nsresult
> EventStateManager::PreHandleEvent(nsPresContext* aPresContext,
>                                   WidgetEvent* aEvent,
>                                   nsIFrame* aTargetFrame,
>+                                  nsIContent* aTargetContent,
>                                   nsEventStatus* aStatus)
> {
>   NS_ENSURE_ARG_POINTER(aStatus);
>   NS_ENSURE_ARG(aPresContext);
>   if (!aEvent) {
>     NS_ERROR("aEvent is null.  This should never happen.");
>     return NS_ERROR_NULL_POINTER;
>   }
> 
>+  NS_ASSERTION(!aTargetFrame
>+               || aTargetFrame->GetContent() == nullptr
>+               || aTargetFrame->GetContent() == aTargetContent,
>+               "aTargetContent should be related with aTargetFrame");
>+  NS_WARN_IF_FALSE(!aTargetContent
>+                   || aTargetContent->GetPrimaryFrame() == nullptr
>+                   || aTargetContent->GetPrimaryFrame() == aTargetFrame,
>+                   "aTargetFrame should be related with aTargetContent");
>+
>   mCurrentTarget = aTargetFrame;
>   mCurrentTargetContent = nullptr;
> 
>   // Focus events don't necessarily need a frame.
>   if (NS_EVENT_NEEDS_FRAME(aEvent)) {
>-    NS_ASSERTION(mCurrentTarget, "mCurrentTarget is null.  this should not happen.  see bug #13007");
>-    if (!mCurrentTarget) return NS_ERROR_NULL_POINTER;
>+    if (!mCurrentTarget&&!aTargetContent) {
>+      NS_ERROR("mCurrentTarget is null. This should not happen. See bug #13007");
>+      return NS_ERROR_NULL_POINTER;
>+    }
>   }
> #ifdef DEBUG
>   if (aEvent->HasDragEventMessage() && sIsPointerLocked) {
>     NS_ASSERTION(sIsPointerLocked,
>       "sIsPointerLocked is true. Drag events should be suppressed when the pointer is locked.");
>   }
> #endif
>   // Store last known screenPoint and clientPoint so pointer lock
>   // can use these values as constants.
>@@ -1495,22 +1507,24 @@ EventStateManager::BeginTrackingDragGest
> {
>   if (!inDownEvent->widget)
>     return;
> 
>   // Note that |inDownEvent| could be either a mouse down event or a
>   // synthesized mouse move event.
>   mGestureDownPoint = inDownEvent->refPoint +
>     LayoutDeviceIntPoint::FromUntyped(inDownEvent->widget->WidgetToScreenOffset());
> 
>-  inDownFrame->GetContentForEvent(inDownEvent,
>-                                  getter_AddRefs(mGestureDownContent));
>-
>-  mGestureDownFrameOwner = inDownFrame->GetContent();
>+  if (inDownFrame) {
>+    inDownFrame->GetContentForEvent(inDownEvent,
>+                                    getter_AddRefs(mGestureDownContent));
>+
>+    mGestureDownFrameOwner = inDownFrame->GetContent();
>+  }
>   mGestureModifiers = inDownEvent->modifiers;
>   mGestureDownButtons = inDownEvent->buttons;
> 
>   if (Prefs::ClickHoldContextMenu()) {
>     // fire off a timer to track click-hold
>     CreateClickHoldTimer(aPresContext, inDownFrame, inDownEvent);
>   }
> }
> 
>@@ -2673,20 +2687,19 @@ NodeAllowsClickThrough(nsINode* aNode)
> nsresult
> EventStateManager::PostHandleEvent(nsPresContext* aPresContext,
>                                    WidgetEvent* aEvent,
>                                    nsIFrame* aTargetFrame,
>                                    nsEventStatus* aStatus)
> {
>   NS_ENSURE_ARG(aPresContext);
>   NS_ENSURE_ARG_POINTER(aStatus);
> 
>-  bool dispatchedToContentProcess = HandleCrossProcessEvent(aEvent,
>-                                                            aStatus);
>+  bool dispatchedToContentProcess = HandleCrossProcessEvent(aEvent, aStatus);
> 
>   mCurrentTarget = aTargetFrame;
>   mCurrentTargetContent = nullptr;
> 
>   // Most of the events we handle below require a frame.
>   // Add special cases here.
>   if (!mCurrentTarget && aEvent->message != NS_MOUSE_BUTTON_UP &&
>       aEvent->message != NS_MOUSE_BUTTON_DOWN) {
>     return NS_OK;
>@@ -4275,19 +4288,21 @@ EventStateManager::UpdateDragDataTransfe
> }
> 
> nsresult
> EventStateManager::SetClickCount(nsPresContext* aPresContext,
>                                  WidgetMouseEvent* aEvent,
>                                  nsEventStatus* aStatus)
> {
>   nsCOMPtr<nsIContent> mouseContent;
>   nsIContent* mouseContentParent = nullptr;
>-  mCurrentTarget->GetContentForEvent(aEvent, getter_AddRefs(mouseContent));
>+  if (mCurrentTarget) {
>+    mCurrentTarget->GetContentForEvent(aEvent, getter_AddRefs(mouseContent));
>+  }
>   if (mouseContent) {
>     if (mouseContent->IsNodeOfType(nsINode::eTEXT)) {
>       mouseContent = mouseContent->GetParent();
>     }
>     if (mouseContent && mouseContent->IsRootOfNativeAnonymousSubtree()) {
>       mouseContentParent = mouseContent->GetParent();
>     }
>   }
> 
>diff --git a/dom/events/EventStateManager.h b/dom/events/EventStateManager.h
>--- a/dom/events/EventStateManager.h
>+++ b/dom/events/EventStateManager.h
>@@ -88,18 +88,19 @@ public:
>    * the DOM or frames.  Any processing which must not be prevented or
>    * cancelled should occur here.  Any processing which is intended to
>    * be conditional based on either DOM or frame processing should occur in
>    * PostHandleEvent.  Any centralized event processing which must occur before
>    * DOM or frame event handling should occur here as well.
>    */
>   nsresult PreHandleEvent(nsPresContext* aPresContext,
>                           WidgetEvent* aEvent,
>                           nsIFrame* aTargetFrame,
>+                          nsIContent* aTargetContent,
>                           nsEventStatus* aStatus);
> 
>   /* The PostHandleEvent method should contain all system processing which
>    * should occur conditionally based on DOM or frame processing.  It should
>    * also contain any centralized event processing which must occur after
>    * DOM and frame processing.
>    */
>   nsresult PostHandleEvent(nsPresContext* aPresContext,
>                            WidgetEvent* aEvent,
>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>--- a/layout/base/nsIPresShell.h
>+++ b/layout/base/nsIPresShell.h
>@@ -131,22 +131,22 @@ class SourceSurface;
> typedef struct CapturingContentInfo {
>   // capture should only be allowed during a mousedown event
>   bool mAllowed;
>   bool mPointerLock;
>   bool mRetargetToElement;
>   bool mPreventDrag;
>   nsIContent* mContent;
> } CapturingContentInfo;
> 
>-//a4e5ff3a-dc5c-4b3a-a625-d164a9e50619
>+//4e1bb03f-fbfa-4455-6a52-1d469a5e6091
> #define NS_IPRESSHELL_IID \
>-{ 0xa4e5ff3a, 0xdc5c, 0x4b3a, \
>-  {0xa6, 0x25, 0xd1, 0x64, 0xa9, 0xe5, 0x06, 0x19}}
>+{ 0x4e1bb03f, 0xfbfa, 0x4455, \
>+  {0x6a, 0x52, 0x1d, 0x46, 0x9a, 0x5e, 0x60, 0x91}}
> 
> // debug VerifyReflow flags
> #define VERIFY_REFLOW_ON                    0x01
> #define VERIFY_REFLOW_NOISY                 0x02
> #define VERIFY_REFLOW_ALL                   0x04
> #define VERIFY_REFLOW_DUMP_COMMANDS         0x08
> #define VERIFY_REFLOW_NOISY_RC              0x10
> #define VERIFY_REFLOW_REALLY_NOISY_RC       0x20
> #define VERIFY_REFLOW_DURING_RESIZE_REFLOW  0x40
>@@ -1362,19 +1362,20 @@ public:
>     PAINT_LAYERS = 0x01,
>     /* Composite layers to the window. */
>     PAINT_COMPOSITE = 0x02,
>   };
>   virtual void Paint(nsView* aViewToPaint, const nsRegion& aDirtyRegion,
>                      uint32_t aFlags) = 0;
>   virtual nsresult HandleEvent(nsIFrame* aFrame,
>                                mozilla::WidgetGUIEvent* aEvent,
>                                bool aDontRetargetEvents,
>-                               nsEventStatus* aEventStatus) = 0;
>+                               nsEventStatus* aEventStatus,
>+                               nsIContent** aTargetContent = nullptr) = 0;
>   virtual bool ShouldIgnoreInvalidation() = 0;
>   /**
>    * Notify that we're going to call Paint with PAINT_LAYERS
>    * on the pres shell for a widget (which might not be this one, since
>    * WillPaint is called on all presshells in the same toplevel window as the
>    * painted widget). This is issued at a time when it's safe to modify
>    * widget geometry.
>    */
>   virtual void WillPaint() = 0;
>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -4500,18 +4500,25 @@ PresShell::ContentRemoved(nsIDocument *a
>   } else {
>     oldNextSibling = nullptr;
>   }
> 
>   if (aContainer && aContainer->IsElement()) {
>     mPresContext->RestyleManager()->
>       RestyleForRemove(aContainer->AsElement(), aChild, oldNextSibling);
>   }
> 
>+  // After removing aChild from tree we should save information about live ancestor
>+  if (mPointerEventTarget) {
>+    if (nsContentUtils::ContentIsDescendantOf(mPointerEventTarget, aChild)) {
>+      mPointerEventTarget = aContainer;
>+    }
>+  }
>+
>   bool didReconstruct;
>   mFrameConstructor->ContentRemoved(aContainer, aChild, oldNextSibling,
>                                     nsCSSFrameConstructor::REMOVE_CONTENT,
>                                     &didReconstruct);
> 
> 
>   if (((aContainer &&
>       static_cast<nsINode*>(aContainer) == static_cast<nsINode*>(aDocument)) ||
>       aDocument) && aChild->NodeType() == nsIDOMNode::DOCUMENT_TYPE_NODE) {
>@@ -6668,19 +6675,20 @@ FlushThrottledStyles(nsIDocument *aDocum
> 
>   return true;
> }
> 
> static nsresult
> DispatchPointerFromMouseOrTouch(PresShell* aShell,
>                                 nsIFrame* aFrame,
>                                 WidgetGUIEvent* aEvent,
>                                 bool aDontRetargetEvents,
>-                                nsEventStatus* aStatus)
>+                                nsEventStatus* aStatus,
>+                                nsIContent** aTargetContent)
> {
>   uint32_t pointerMessage = 0;
>   if (aEvent->eventStructType == NS_MOUSE_EVENT) {
>     WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent();
>     // if it is not mouse then it is likely will come as touch event
>     if (!mouseEvent->convertToPointer) {
>       return NS_OK;
>     }
>     int16_t button = mouseEvent->button;
>@@ -6702,19 +6710,19 @@ DispatchPointerFromMouseOrTouch(PresShel
>     }
> 
>     WidgetPointerEvent event(*mouseEvent);
>     event.message = pointerMessage;
>     event.button = button;
>     event.pressure = event.buttons ?
>                      mouseEvent->pressure ? mouseEvent->pressure : 0.5f :
>                      0.0f;
>     event.convertToPointer = mouseEvent->convertToPointer = false;
>-    aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aStatus);
>+    aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aStatus, aTargetContent);
>   } else if (aEvent->eventStructType == NS_TOUCH_EVENT) {
>     WidgetTouchEvent* touchEvent = aEvent->AsTouchEvent();
>     // loop over all touches and dispatch pointer events on each touch
>     // copy the event
>     switch (touchEvent->message) {
>     case NS_TOUCH_MOVE:
>       pointerMessage = NS_POINTER_MOVE;
>       break;
>     case NS_TOUCH_END:
>@@ -6747,19 +6755,19 @@ DispatchPointerFromMouseOrTouch(PresShel
>       event.tiltX = touch->tiltX;
>       event.tiltY = touch->tiltY;
>       event.time = touchEvent->time;
>       event.timeStamp = touchEvent->timeStamp;
>       event.mFlags = touchEvent->mFlags;
>       event.button = WidgetMouseEvent::eLeftButton;
>       event.buttons = WidgetMouseEvent::eLeftButtonFlag;
>       event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_TOUCH;
>       event.convertToPointer = touch->convertToPointer = false;
>-      aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aStatus);
>+      aShell->HandleEvent(aFrame, &event, aDontRetargetEvents, aStatus, aTargetContent);
>     }
>   }
>   return NS_OK;
> }
> 
> class ReleasePointerCaptureCaller
> {
> public:
>   ReleasePointerCaptureCaller() :
>@@ -6782,39 +6790,55 @@ public:
> private:
>   int32_t mPointerId;
>   nsCOMPtr<nsIContent> mContent;
> };
> 
> nsresult
> PresShell::HandleEvent(nsIFrame* aFrame,
>                        WidgetGUIEvent* aEvent,
>                        bool aDontRetargetEvents,
>-                       nsEventStatus* aEventStatus)
>+                       nsEventStatus* aEventStatus,
>+                       nsIContent** aTargetContent)
> {
> #ifdef MOZ_TASK_TRACER
>   // Make touch events, mouse events and hardware key events to be the source
>   // events of TaskTracer, and originate the rest correlation tasks from here.
>   SourceEventType type = SourceEventType::UNKNOWN;
>   if (WidgetTouchEvent* inputEvent = aEvent->AsTouchEvent()) {
>     type = SourceEventType::TOUCH;
>   } else if (WidgetMouseEvent* inputEvent = aEvent->AsMouseEvent()) {
>     type = SourceEventType::MOUSE;
>   } else if (WidgetKeyboardEvent* inputEvent = aEvent->AsKeyboardEvent()) {
>     type = SourceEventType::KEY;
>   }
>   AutoSourceEvent taskTracerEvent(type);
> #endif
> 
>+  NS_ASSERTION(aFrame, "aFrame should be not null");
>+
>   if (sPointerEventEnabled) {
>-    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus);
>-  }
>-
>-  NS_ASSERTION(aFrame, "null frame");
>+    nsWeakFrame weakFrame(aFrame);
>+    nsCOMPtr<nsIContent> targetContent;
>+    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents,
>+                                    aEventStatus, getter_AddRefs(targetContent));
>+    if (!weakFrame.IsAlive()) {
>+      if (targetContent) {
>+        aFrame = targetContent->GetPrimaryFrame();
>+        if (!aFrame) {
>+          PushCurrentEventInfo(aFrame, targetContent);
>+          return HandleEventInternal(aEvent, aEventStatus);
>+        }
>+      } else {
>+        MOZ_ASSERT(false, "aFrame or targetContent should be live");
>+        return NS_OK;
>+      }
>+    }
>+  }
> 
>   if (mIsDestroying ||
>       (sDisableNonTestMouseEvents && !aEvent->mFlags.mIsSynthesizedForTests &&
>        aEvent->HasMouseEventMessage())) {
>     return NS_OK;
>   }
> 
>   RecordMouseLocation(aEvent);
> 
>@@ -7247,30 +7271,46 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>           GetPresShell();
>       if (activeShell &&
>           nsContentUtils::ContentIsCrossDocDescendantOf(activeShell->GetDocument(),
>                                                         shell->GetDocument())) {
>         shell = static_cast<PresShell*>(activeShell);
>         frame = shell->GetRootFrame();
>       }
>     }
> 
>+    // Before HandlePositionedEvent we should save mPointerEventTarget in some cases
>+    nsWeakFrame weakFrame(frame);
>+    if (sPointerEventEnabled && aTargetContent && aEvent->eventStructType == NS_POINTER_EVENT) {
>+      shell->mPointerEventTarget = frame->GetContent();
>+    }
>+
>+    nsresult rv;
>     if (shell != this) {
>       // Handle the event in the correct shell.
>       // Prevent deletion until we're done with event handling (bug 336582).
>       nsCOMPtr<nsIPresShell> kungFuDeathGrip(shell);
>       // We pass the subshell's root frame as the frame to start from. This is
>       // the only correct alternative; if the event was captured then it
>       // must have been captured by us or some ancestor shell and we
>       // now ask the subshell to dispatch it normally.
>-      return shell->HandlePositionedEvent(frame, aEvent, aEventStatus);
>-    }
>-
>-    return HandlePositionedEvent(frame, aEvent, aEventStatus);
>+      rv = shell->HandlePositionedEvent(frame, aEvent, aEventStatus);
>+    } else {
>+      rv = HandlePositionedEvent(frame, aEvent, aEventStatus);
>+    }
>+
>+    // After HandlePositionedEvent we should reestablish content (which still live in tree) in some cases
>+    if (sPointerEventEnabled && aTargetContent && aEvent->eventStructType == NS_POINTER_EVENT) {
>+      if (!weakFrame.IsAlive()) {
>+        shell->mPointerEventTarget.swap(*aTargetContent);
>+      }
>+    }
>+
>+    return rv;
>   }
> 
>   nsresult rv = NS_OK;
> 
>   if (frame) {
>     PushCurrentEventInfo(nullptr, nullptr);
> 
>     // key and IME related events go to the focused frame in this DOM window.
>     if (aEvent->IsTargetedAtFocusedContent()) {
>@@ -7495,19 +7535,19 @@ PresShell::HandleEventWithTarget(WidgetE
>   return rv;
> }
> 
> nsresult
> PresShell::HandleEventInternal(WidgetEvent* aEvent, nsEventStatus* aStatus)
> {
>   nsRefPtr<EventStateManager> manager = mPresContext->EventStateManager();
>   nsresult rv = NS_OK;
> 
>-  if (!NS_EVENT_NEEDS_FRAME(aEvent) || GetCurrentEventFrame()) {
>+  if (!NS_EVENT_NEEDS_FRAME(aEvent) || GetCurrentEventFrame() || GetCurrentEventContent()) {
>     bool touchIsNew = false;
>     bool isHandlingUserInput = false;
> 
>     // XXX How about IME events and input events for plugins?
>     if (aEvent->mFlags.mIsTrusted) {
>       switch (aEvent->message) {
>       case NS_KEY_PRESS:
>       case NS_KEY_DOWN:
>       case NS_KEY_UP: {
>@@ -7691,19 +7731,20 @@ PresShell::HandleEventInternal(WidgetEve
>     nsAutoPopupStatePusher popupStatePusher(
>                              Event::GetEventPopupControlState(aEvent));
> 
>     // FIXME. If the event was reused, we need to clear the old target,
>     // bug 329430
>     aEvent->target = nullptr;
> 
>     // 1. Give event to event manager for pre event state changes and
>     //    generation of synthetic events.
>-    rv = manager->PreHandleEvent(mPresContext, aEvent, mCurrentEventFrame, aStatus);
>+    rv = manager->PreHandleEvent(mPresContext, aEvent, mCurrentEventFrame,
>+                                 mCurrentEventContent, aStatus);
> 
>     // 2. Give event to the DOM for third party and JS use.
>     if (NS_SUCCEEDED(rv)) {
>       bool wasHandlingKeyBoardEvent =
>         nsContentUtils::IsHandlingKeyBoardEvent();
>       if (aEvent->eventStructType == NS_KEY_EVENT) {
>         nsContentUtils::SetIsHandlingKeyBoardEvent(true);
>       }
>       if (aEvent->IsAllowedToDispatchDOMEvent()) {
>@@ -7725,20 +7766,20 @@ PresShell::HandleEventInternal(WidgetEve
>               eventTarget = do_QueryInterface(mDocument);
>               // If we don't have any content, the callback wouldn't probably
>               // do nothing.
>               eventCBPtr = nullptr;
>             }
>           }
>           if (eventTarget) {
>             if (aEvent->eventStructType == NS_COMPOSITION_EVENT ||
>                 aEvent->eventStructType == NS_TEXT_EVENT) {
>-              IMEStateManager::DispatchCompositionEvent(eventTarget,
>-                mPresContext, aEvent, aStatus, eventCBPtr);
>+              IMEStateManager::DispatchCompositionEvent(eventTarget, mPresContext,
>+                                                        aEvent, aStatus, eventCBPtr);
>             } else {
>               EventDispatcher::Dispatch(eventTarget, mPresContext,
>                                         aEvent, nullptr, aStatus, eventCBPtr);
>             }
>           }
>         }
>       }
> 
>       nsContentUtils::SetIsHandlingKeyBoardEvent(wasHandlingKeyBoardEvent);
>diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
>--- a/layout/base/nsPresShell.h
>+++ b/layout/base/nsPresShell.h
>@@ -194,19 +194,20 @@ public:
>   virtual gfxSize GetCumulativeResolution() MOZ_OVERRIDE;
> 
>   //nsIViewObserver interface
> 
>   virtual void Paint(nsView* aViewToPaint, const nsRegion& aDirtyRegion,
>                      uint32_t aFlags) MOZ_OVERRIDE;
>   virtual nsresult HandleEvent(nsIFrame* aFrame,
>                                mozilla::WidgetGUIEvent* aEvent,
>                                bool aDontRetargetEvents,
>-                               nsEventStatus* aEventStatus) MOZ_OVERRIDE;
>+                               nsEventStatus* aEventStatus,
>+                               nsIContent** aTargetContent) MOZ_OVERRIDE;
>   virtual nsresult HandleDOMEventWithTarget(
>                                  nsIContent* aTargetContent,
>                                  mozilla::WidgetEvent* aEvent,
>                                  nsEventStatus* aStatus) MOZ_OVERRIDE;
>   virtual nsresult HandleDOMEventWithTarget(nsIContent* aTargetContent,
>                                                         nsIDOMEvent* aEvent,
>                                                         nsEventStatus* aStatus) MOZ_OVERRIDE;
>   virtual bool ShouldIgnoreInvalidation() MOZ_OVERRIDE;
>   virtual void WillPaint() MOZ_OVERRIDE;
>@@ -836,12 +837,17 @@ protected:
> 
>   bool                      mAsyncResizeTimerIsActive : 1;
>   bool                      mInResize : 1;
> 
>   bool                      mImageVisibilityVisited : 1;
> 
>   bool                      mNextPaintCompressed : 1;
> 
>   static bool               sDisableNonTestMouseEvents;
>+
>+  // Information about live content (which still stay in DOM tree).
>+  // Used in case we need re-dispatch event after sending pointer event,
>+  // when target of pointer event was deleted during execute user handlers
>+  nsCOMPtr<nsIContent>      mPointerEventTarget;
> };
> 
> #endif /* !defined(nsPresShell_h_) */
Attachment #8450151 - Flags: review?(bugs)
Oops, sorry about that above.

So ESM::PreHandleEvent and PostHandleEvent need to be audited for this new setup when there might
not be a frame for mouse/touch events.
Looks like tests end up triggering 
'!aTargetFrame || aTargetFrame->GetContent() == nullptr || aTargetFrame->GetContent() == aTargetContent' assertion
Group: dom-core-security, layout-core-security
Attached patch check_frame_ver13.diff (obsolete) — Splinter Review
- Changes: Removed unnecessary assertion
Attachment #8450151 - Attachment is obsolete: true
Attachment #8450151 - Flags: feedback?(oleg.romashin)
Attachment #8450151 - Flags: feedback?(nicklebedev37)
Attachment #8462632 - Flags: review?(bugs)
Attachment #8462632 - Flags: feedback?(oleg.romashin)
Attachment #8462632 - Flags: feedback?(nicklebedev37)
Comment on attachment 8462632 [details] [diff] [review]
check_frame_ver13.diff


> EventStateManager::PreHandleEvent(nsPresContext* aPresContext,
>                                   WidgetEvent* aEvent,
>                                   nsIFrame* aTargetFrame,
>+                                  nsIContent* aTargetContent,
>                                   nsEventStatus* aStatus)
Changes to EventStateManager look surprisingly simple :)


>+  NS_WARN_IF_FALSE(!aTargetFrame
>+                   || aTargetFrame->GetContent() == nullptr
>+                   || aTargetFrame->GetContent() == aTargetContent,
>+                   "aTargetContent should be related with aTargetFrame");
Nit,
>+  NS_WARN_IF_FALSE(!aTargetFrame ||
>+                   !aTargetFrame->GetContent() ||
>+                   aTargetFrame->GetContent() == aTargetContent,
>+                   "aTargetContent should be related with aTargetFrame");



>-  bool dispatchedToContentProcess = HandleCrossProcessEvent(aEvent,
>-                                                            aStatus);
>+  bool dispatchedToContentProcess = HandleCrossProcessEvent(aEvent, aStatus);
Please don't do unrelated changes.


>+      } else {
>+        MOZ_ASSERT(false, "aFrame or targetContent should be live");
There is actually a case when this doesn't hold.
If document's root element is removed from the document.
So, remove this MOZ_ASSERT

>+    // Before HandlePositionedEvent we should save mPointerEventTarget in some cases
>+    nsWeakFrame weakFrame(frame);
>+    if (sPointerEventEnabled && aTargetContent && aEvent->eventStructType == NS_POINTER_EVENT) {
>+      shell->mPointerEventTarget = frame->GetContent();
Could you MOZ_ASSERT here that MOZ_ASSERT(!frame->GetContent() || shell->GetDocument() == frame->GetContent()->OwnerDoc());


>+  // Information about live content (which still stay in DOM tree).
>+  // Used in case we need re-dispatch event after sending pointer event,
>+  // when target of pointer event was deleted during execute user handlers
executing
Attachment #8462632 - Flags: review?(bugs) → review+
Attached patch check_frame_ver14.diff (obsolete) — Splinter Review
+ Changes according with comments
Attachment #8462632 - Attachment is obsolete: true
Attachment #8462632 - Flags: feedback?(oleg.romashin)
Attachment #8462632 - Flags: feedback?(nicklebedev37)
Attachment #8463894 - Flags: review?(bugs)
Attachment #8463894 - Flags: feedback?(oleg.romashin)
Attachment #8463894 - Flags: feedback?(nicklebedev37)
Attached file check_frame_test_ver8.diff (obsolete) —
Semiautomatic test
Attachment #8463914 - Flags: review?(bugs)
Attachment #8463914 - Flags: feedback?(oleg.romashin)
Attachment #8463914 - Flags: feedback?(nicklebedev37)
Comment on attachment 8463914 [details]
check_frame_test_ver8.diff

>+  function runTest() {
>+    var killer = document.getElementById('killer');
>+    var div = killer.parentNode.parentNode;
>+    var parent = div.parentNode;
>+    setTimeout(function() {
>+      synthesizeMouse(killer.parentNode, 3, 3, {type: "mousedown"});
Why not click?

>+<head>
>+  <title>Test for Bug 979497</title>
>+  <meta name="author" content="Maksim Lebedev" />
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <script type="text/javascript">
>+    function prepareTest() {
>+      SimpleTest.waitForExplicitFinish();
>+      SpecialPowers.pushPrefEnv({
>+        "set": [
>+          ["dom.w3c_pointer_events.enabled", false]
Er, why is this needed? 

I don't understand the "semiautomatic" part. And would this work only on desktop?
Enabling only on desktop should be fine (so disable on b2g and Android where select is handled differently).
Attachment #8463914 - Flags: review?(bugs) → review-
Attachment #8463894 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #73)
> >+      synthesizeMouse(killer.parentNode, 3, 3, {type: "mousedown"});
> Why not click?
You mean synthetizeMouse without type (In this case function call mousedown and mouseup) ?
In this case items of select do not open.
> >+        "set": [
> >+          ["dom.w3c_pointer_events.enabled", false]
> Er, why is this needed? 
By previous reason. Only in this case items of select will be opened.
> I don't understand the "semiautomatic" part. And would this work only on desktop?
I tried to use different code which can send events to content.
(For example, synthetizePointer with pointerdown,
but in this case "onpointerdown" event doesn't fire on target.
And items of select will not be close (but only marked as blue line))
(For example, synthetizeMouse with mousedown,
but in this case "onpointerdown" event doesn't fire on target too.
But "onmousedown" can be detected.)
Test needs that real user should click in opens item,
only in this case we can detect dangerous behavior (I mean without patch).
> Enabling only on desktop should be fine
> (so disable on b2g and Android where select is handled differently).
I don't know behavior on others platforms. On windows test doesn't work in auto mode.

What's why test was named as "semiautomatic".
If you know some feature for repair this, let me know about it.
What auto mode?

We should only check in automatically run tests.
(In reply to Olli Pettay [:smaug] from comment #75)
> What auto mode?
I mean without action from user.
> We should only check in automatically run tests.
As I described in comment 74 test have several issues with auto mode.
That's why I didn't put test in "mochitest.ini"

Maybe we can land only patch without test?
we probably could yes
Comment on attachment 8463914 [details]
check_frame_test_ver8.diff

- Changes: Removed flag "patch" on test
Attachment #8463914 - Attachment is patch: false
Comment on attachment 8388791 [details] [diff] [review]
Check frame after dispatching pointer event

As I correctly understand, we should remove this patch before other patches from this bug will be landed on m-c. But I have no right to do this. Can anyone help me to do this?
Flags: needinfo?(oleg.romashin)
Flags: needinfo?(bugs)
Comment on attachment 8463894 [details] [diff] [review]
check_frame_ver14.diff

If nobody have any objections, we will land only this patch without tests.
We can do that, but please try to figure out some way to test this. We want to have tests to
catch regressions.
Flags: needinfo?(bugs)
Attachment #8463894 - Flags: feedback?(nicklebedev37)
Attachment #8463914 - Flags: feedback?(nicklebedev37)
Some investigation about why test does not work:
1.Looks like "<select>" item has handler (with "system_group" mark) on "mousedown" event (which can close opened "select" item). According handler with "pointerdown" event is not exist.
2.Syntetized event maybe has difference with real event.
Flags: needinfo?(oleg.romashin)
Some investigation:
3. "Select" does not handled PointerDown event. 
Expected: On PointerDown event "select" should be opened. But it doesn't happen.
Some investigation:
4. When PointerDown was sent by script, EventTargetChain doesn't contain "OptionElement" and "SelectElement". On real PointerDown, EventTargetChain contains both items.
Some investigation:
5. Scheme "MouseDown-SetPointerCapture-MouseDown" does not work, in this case EventTargetChain still does not contain "OptionElement" and "SelectElement"
Looks like, SetPointerCapture in this case does not work. There is reason in code:
PresShell::HandleEvent()
...
    if (aEvent->mClass == ePointerEventClass && aEvent->message != NS_POINTER_DOWN) {
...
        nsIContent* pointerCapturingContent = GetPointerCapturingContent(pointerId);

Why we need check for NS_POINTER_DOWN here? Is it realy necessary?
Some investigation:
6. Scheme "target.dispatchEvent(new MouseEvent())" does not work. In this case nsPresShell::HandleEvent does not be called and MouseEvent is not translated into according PointerEvent.
Yes, dispatchEvent does only the DOM Event dispatch, and presshell is above the DOM.
Some investigation:
7. Difference between syntetic MouseDown event and real MouseDown event:
    In function PresShell::HandleEvent(aFrame, ...) we get different aFrame values:
      [ViewportFrame] in case syntetic MouseDown,
      [nsListControlFrame] in case real MouseDown event.
Some investigation:
8. Syntetic mouse event uses:
     nsDOMWindowUtils::SendMouseEventCommon()
       nsCOMPtr<nsIWidget> widget = GetWidget(&offset);
   Real mouse event uses:
     nsWindow::DispatchMouseEvent()
       result = DispatchWindowEvent(&event);
   As result we have different widgets and different frames.
Some investigation:
9. Looks like we should find [ChildWindow] related with opened "Select" and "Options". In this case we can do something like that: "_getDOMWindowUtils([ChildWindow]).sendMouseEvent()" - it helps us to send event into correct Widget and related PresShell.
In this case we can make correct auto-test in mochitest system.
Otherwise incorrect PresShell cannot do break-behavior and we cannot simulate break-behavior in auto-test.

Can we found needed ChildWindow related with opened "SelectElement" - it is opened issue.
Attached patch check_frame_ver15.diff (obsolete) — Splinter Review
+Changes: Patch was updates according with new sources
Attachment #8463894 - Attachment is obsolete: true
Attachment #8463894 - Flags: feedback?(oleg.romashin)
Attachment #8585499 - Flags: review?(bugs)
We cannot make auto test for check regression according with investigation.
This is latest variant for check regression in manual mode.
Attachment #8463914 - Attachment is obsolete: true
Attachment #8463914 - Flags: feedback?(oleg.romashin)
Attachment #8585503 - Flags: feedback?(bugs)
Comment on attachment 8585503 [details] [diff] [review]
check_frame_manual_test_ver9.diff

Not sure what feedback is needed for this. We shouldn't land this as mochitest
anyway, if it doesn't actually test the issue.
Attachment #8585503 - Flags: feedback?(bugs)
Comment on attachment 8585499 [details] [diff] [review]
check_frame_ver15.diff


>@@ -501,17 +507,18 @@ EventStateManager::PreHandleEvent(nsPres
>     }
>     ++gMouseOrKeyboardEventCounter;
>   }
> 
>   WheelTransaction::OnEvent(aEvent);
> 
>   // Focus events don't necessarily need a frame.
>   if (NS_EVENT_NEEDS_FRAME(aEvent)) {
>-    if (!mCurrentTarget) {
>+    if (!mCurrentTarget && !aTargetContent) {
This is now misleading. We need frame, but return early only if both frame and content are null.
NS_EVENT_NEEDS_FRAME doesn't anymore mean what it used to. 
Do we need the whole i'f (NS_EVENT_NEEDS_FRAME(aEvent)) { ' anymore? Could we just have 
if (!mCurrentTarget && !aTargetContent) {
  return;
}
?

>+  NS_ASSERTION(aFrame, "aFrame should be not null");
>+
>   if (sPointerEventEnabled) {
>-    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus);
>-  }
>-
>-  NS_ASSERTION(aFrame, "null frame");
>+    nsWeakFrame weakFrame(aFrame);

Some odd indentation here.

>+    if (!weakFrame.IsAlive()) {
>+      if (targetContent) {
>+        aFrame = targetContent->GetPrimaryFrame();
>+        if (!aFrame) {
>+          PushCurrentEventInfo(aFrame, targetContent);
>+          return HandleEventInternal(aEvent, aEventStatus);
I don't see where you call PopCurrentEventInfo


>+    // Before HandlePositionedEvent we should save mPointerEventTarget in some cases
>+    nsWeakFrame weakFrame(frame);
>+    if (sPointerEventEnabled && aTargetContent && ePointerEventClass == aEvent->mClass) {
Could you please assign frame to weakFrame inside this if


>+    // After HandlePositionedEvent we should reestablish content (which still live in tree) in some cases
>+    if (sPointerEventEnabled && aTargetContent && ePointerEventClass == aEvent->mClass) {
>+      if (!weakFrame.IsAlive()) {
>+        shell->mPointerEventTarget.swap(*aTargetContent);
>+      }
Since we seem to use the weakFrame only here.


almost there. (I don't know why I gave r+ earlier)
Attachment #8585499 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #95)
> >+  NS_ASSERTION(aFrame, "aFrame should be not null");
> >+
> >   if (sPointerEventEnabled) {
> >-    DispatchPointerFromMouseOrTouch(this, aFrame, aEvent, aDontRetargetEvents, aEventStatus);
> >-  }
> >-
> >-  NS_ASSERTION(aFrame, "null frame");
> >+    nsWeakFrame weakFrame(aFrame);
> 
> Some odd indentation here.
I cannot see anything wrong. In final source code all looks ok.
+ Added calling PopCurrentEventInfo()
+ Added nsWeakFrame initialization only under *if* condition
- Removed check NS_EVENT_NEEDS_FRAME(aEvent)
Attachment #8585499 - Attachment is obsolete: true
Attachment #8587410 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #95)
> almost there. (I don't know why I gave r+ earlier)
All happens in first time :-)
Attachment #8587410 - Flags: review?(bugs) → review+
If nobody has objections, we can push latest changes into m-c.
Can anybody put "checkin" flag? (I have no rights to do it).
Flags: needinfo?(bugs)
checkin flag isn't needed, just set checkin-needed keyword.
Flags: needinfo?(bugs)
I have no rights for any changing in header of this bug.
Does it need clearance from the security team to land? Assuming not because comment 4.
Keywords: checkin-needed
Shouldn't need, since this is behind a pref.
Attachment #8388791 - Attachment is obsolete: true
Attachment #8585503 - Attachment is obsolete: true
Attachment #8585503 - Attachment is patch: true
Attachment #8585503 - Attachment mime type: text/x-patch → text/plain
https://hg.mozilla.org/mozilla-central/rev/53b6cf5805c8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Whiteboard: [post-critsmash-triage]
Group: core-security → core-security-release
Group: core-security-release
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.