396108 - Crash [@ PresShell::ResizeReflow] with iframes, binding, while onbeforecopy removing stuff

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: testcase

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

Comment 1

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

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

Comment 2

[Mathieu Fenniak]

I'll take a look today.

Comment 3

[Mathieu Fenniak]

Attachment: patch

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 4

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

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.

Comment 5

[Mathieu Fenniak]

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.

Comment 6

[Martijn Wargers (dead)]

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

Comment 7

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

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 8

[Mathieu Fenniak]

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.

Comment 9

[Jeff Walden [:Waldo]]

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

Comment 10

[Martijn Wargers (dead)]

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

Comment 11

[Martijn Wargers (dead)]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Mathieu Fenniak]

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.

Comment 14

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

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

Comment 15

[Mathieu Fenniak]

Attachment: failed attempt at making mochitest tc

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 16

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

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Mathieu Fenniak]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Martijn Wargers (dead)]

I think the patch may be checked in, right?

Comment 21

[Martijn Wargers (dead)]

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.

Comment 22

[Neil Deakin]

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