See testcase, I crash with current trunk build when clicking on the text in the testcase. This doesn't happen in a 2007-07-25 build, but does happen in a 2007-07-26 build, so a regression from bug 280959. The binding I used in the testcase is just an empty binding. http://crash-stats.mozilla.com/report/index/a7830899-6251-11dc-b4a0-001a4bd43ed6 0 PresShell::ResizeReflow(int, int) mozilla/layout/base/nsPresShell.cpp:2503 1 PresShell::ResizeReflow(nsIView*, int, int) mozilla/layout/base/nsPresShell.cpp:5782 2 nsViewManager::DoSetWindowDimensions(int, int) mozilla/view/src/nsViewManager.h:279 3 nsViewManager::SetWindowDimensions(int, int) mozilla/view/src/nsViewManager.cpp:365 4 nsViewManager::DispatchEvent(nsGUIEvent*, nsEventStatus*) mozilla/view/src/nsViewManager.cpp:960 5 HandleEvent mozilla/view/src/nsView.cpp:168 6 nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) mozilla/widget/src/windows/nsWindow.cpp:1075 7 nsWindow::DispatchWindowEvent(nsGUIEvent*) mozilla/widget/src/windows/nsWindow.cpp:1095 8 nsWindow::OnResize(nsRect&) mozilla/widget/src/windows/nsWindow.cpp:5796 9 nsWindow::ProcessMessage(unsigned int, unsigned int, long, long*) mozilla/widget/src/windows/nsWindow.cpp:4801 10 nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long) mozilla/widget/src/windows/nsWindow.cpp:1288 etc..

Mathieu, would you have time to look at this? The bug is perhaps somewhat similar to bug 393696.

I'll take a look today.

Addressed crash with the same approach taken in 393696, but this time the events were being fired from the editors which weren't similarly protected by 393696's patch. Crash fixed, tested against 280959's unit tests successfully as well. There is an assertion being generated by this test case in PresShell::ClearPreferenceStyleRules (nsPresShell.cpp line 1823), but it doesn't seem to be related to the crash here, and I'm not sure if it is important.

Comment on attachment 280897 [details] [diff] [review] patch Looks good to me. >+ // if event handler return'd false (PreventDefault) >+ if (status == nsEventStatus_eConsumeNoDefault) >+ *aPreventDefault = PR_TRUE; One could write this also *aPreventDefault = (status == nsEventStatus_eConsumeNoDefault); >+ if (mDidPreDestroy) >+ return NS_ERROR_NOT_INITIALIZED; >+ >+ return NS_OK; Could be also return mDidPreDestroy ? NS_ERROR_NOT_INITIALIZED : NS_OK; No need to change those if you prefer the current style. But I'd like to see mochitest for this bug, so r=me with that.

I'm having trouble creating a mochitest for this bug. It was easy to get a mochitest file that crashed when a real mouse click occurred, but getting the same reaction from a synthetic mouse click has been a no-go so far. Synthesizing the mousedown with nsDOMWindowUtils::SendMouseEvent just doesn't cause the same behavior. Specifically, the real mouse click causes ChangeFocusWith to be called on line 2206 of nsEventStateManager.cpp, while the synthetic mouse click never hits the condition required there. That call is part of the chain that causes the crash. Even when the synthetic mouse click *does* cause the beforecopy event handler to be hit, and does cause the removal of the windows, it doesn't enter that function and begin the crash. I've tried to make the synthetic mousedown event as close to the real one as possible, even creating a new version of SendMouseEvent that sets the event target & flags to match the real mouse click -- still a no go. I am out of ideas for what else to try -- when the event hits nsPresShell::HandleEvent, the args appear identical between the real and the synthetic mousedown event, but it doesn't crash.

Fwiw, I had intermediate testcases (from the unminimized one) that directly crashed, but I'm afraid I don't have those anymore.

Better to get the patch in soon. So you could ask sr. I'll try to look at what is the problem with synthetic mousedown.

Comment on attachment 280897 [details] [diff] [review] patch Requesting approval1.9 - crash fix, reduces some related code duplication where crash could also occur. A similar patch landed for bug 393696 without incident.

bz, see comment 5, since I seem to recall you wrote the event-sending utility functions in the Mochitest stuff.

bz wasn't CC-ed to this bug, bz, see comment 9.

This seems to be worksforme now, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007092105 Minefield/3.0a8pre which is after the patch for bug 396587 was checked in.

There are two sets of event dispatch functions in mochitest. I didn't write the DOMWindowUtils stuff; I wrote the DOM-only dispatch stuff. For the DOMWindowUtils stuff, you want Neil Deakin.

Confirmed, crash is fixed. Would still be nice to land this patch for the code cleanup in firing copy/cut/paste events from editors, and the added safety of not firing these events during reflow.

(In reply to comment #13) > Confirmed, crash is fixed. Would still be nice to land this patch for the code > cleanup in firing copy/cut/paste events from editors, and the added safety of > not firing these events during reflow. > I agree!

Attaching my failed attempts at making a mochitest testcase for this crash. Created a targeted "SendMouseEvent" function and manually set event.flags, attempting to make the synthetic mouse click as similar to the real one as possible, but as mentioned in comment #5, couldn't trigger the crash.

Comment on attachment 281827 [details] [diff] [review] failed attempt at making mochitest tc Um, that can't work because event.target is cleared in PresShell before DOM event dispatch. Did you try to use synthesizeMouse http://lxr.mozilla.org/seamonkey/source/testing/mochitest/tests/SimpleTest/EventUtils.js#180 without event type (so that mousedown, mouseup and click get dispatched)?

synthesizeMouse doesn't send a click, just down and up, no? Though that should probably be fixed...

Yes, I did try to use synthesizeMouse. However, it only works with targets that are XUL elements (aTarget.boxObject). So I resorted to using sendMouseEvent directly, using hardcoded values for {aX, aY} that were copied from a real mouse click. The original crash occurred on mousedown, so I never added a mouseup event. Setting event.target was an effort to make the event being received by PresShell from the synthetic call match the "real" event. I believed that if the synthetic mouse event was identical to the real event, the same code path ought to occur and cause the crash. Setting event.target meant I was getting identical events through, where the onbeforecopy handler would fire (& remove the elements, which originally caused the crash), but still not crashing.

I should paste in the e-mail conversation enn and I had about this: Boris Zbarsky wrote: > Neil, I have two questions about the synthesizeMouse() function in > EventUtils.js: > > 1) It only works for XUL (because it uses .boxObject). I assume there's no > reason not to use .ownerDocument.getBoxObjectFor() instead? It should just be changed to use getBoundingClientRect > 2) It doesn't actually send a click when invoked by default. Should it? Yes, it should. Don't know why I left that out.

I think the patch may be checked in, right?

Checking in html/nsHTMLDataTransfer.cpp; /cvsroot/mozilla/editor/libeditor/html/nsHTMLDataTransfer.cpp,v <-- nsHTMLData Transfer.cpp new revision: 1.130; previous revision: 1.129 done Checking in text/nsPlaintextDataTransfer.cpp; /cvsroot/mozilla/editor/libeditor/text/nsPlaintextDataTransfer.cpp,v <-- nsPla intextDataTransfer.cpp new revision: 1.53; previous revision: 1.52 done Checking in text/nsPlaintextEditor.cpp; /cvsroot/mozilla/editor/libeditor/text/nsPlaintextEditor.cpp,v <-- nsPlaintext Editor.cpp new revision: 1.109; previous revision: 1.108 done Checking in text/nsPlaintextEditor.h; /cvsroot/mozilla/editor/libeditor/text/nsPlaintextEditor.h,v <-- nsPlaintextEd itor.h new revision: 1.31; previous revision: 1.30 done Checked into trunk.

(In reply to comment #19) > > 2) It doesn't actually send a click when invoked by default. Should it? > > Yes, it should. Don't know why I left that out. > I actually was wrong about that. It does fire a click event, as the EventStateManager does this when a mousedown event occurs.