Dispatching events may run scripts, so frame object can be deleted.

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.

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.

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.

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.)

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?

+ 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

Comment on attachment 8420984 [details] [diff] [review] check_frame_ver2.diff This doesn't fix anything.

+ Add solution for bug Any comments are welcome.

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

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.

+ Changes element's path related with DOM objects (according comments)

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.)

+ 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.

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>

And don't hesitate to ask review for a new patch. I'll try to look at it even during this vacation-ish.

+ Changes: live nsIFrame* changed to live nsIContent*

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.

+ Changes: nsCOMPtr<nsIContent> mLiveContent + Changes: Deleted unwanted code + Changes: Added check if GetPrimareFrame returns nullptr

(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.

(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.

+ Changes: IID of nsIPresShell was changed + Changes: new function nsIPresShell::NotifyContentRemoved() + Changes: mLiveContent -> mPointerEventTarget

(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.

(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?

- Changes: deleted PresShell::NotifyContentRemoved() - Changes: deleted "while" when determining aFrame

(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...

(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.

+ Changes: nsIContent* targetContent -> nsCOMPtr<nsIContent> targetContent

(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.

+ Changes: Added sending corresponding event, even if aFrame is null

(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.

+ Changes: Added posibilities to EventStateManager::PreHandleEvent with null aFrame - Changes: Removed unwanted code

(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_) */

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

- Changes: Removed unnecessary assertion

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

+ Changes according with comments

Semiautomatic test

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).

(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

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?

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.

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.

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.

+Changes: Patch was updates according with new sources

We cannot make auto test for check regression according with investigation. This is latest variant for check regression in manual mode.

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.

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)

(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)

(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 :-)

If nobody has objections, we can push latest changes into m-c. Can anybody put "checkin" flag? (I have no rights to do it).

checkin flag isn't needed, just set checkin-needed keyword.

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.

Shouldn't need, since this is behind a pref.