393696 - Crash [@ PresShell::ResizeReflow] with onbeforecut removing element, iframe and position: fixed

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build when clicking on the text in the testcase.
This regressed between 2007-06-25 and 2007-06-26, I guess a regression from bug 280959 somehow.

http://crash-stats.mozilla.com/report/index/392fe228-5333-11dc-ab08-001a4bd43ef6
0  	PresShell::ResizeReflow(int, int)  	 mozilla/layout/base/nsPresShell.cpp:2501
1 	PresShell::ResizeReflow(nsIView*, int, int) 	mozilla/layout/base/nsPresShell.cpp:5767
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:1086
7 	nsWindow::DispatchWindowEvent(nsGUIEvent*) 	mozilla/widget/src/windows/nsWindow.cpp:1106
8 	nsWindow::OnResize(nsRect&) 	mozilla/widget/src/windows/nsWindow.cpp:5815
9 	nsWindow::ProcessMessage(unsigned int, unsigned int, long, long*) 	mozilla/widget/src/windows/nsWindow.cpp:4820
10 	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long) 	mozilla/widget/src/windows/nsWindow.cpp:1299
11 	InternalCallWinProc 	
12 	UserCallWinProcCheckWow

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is really bad, we might need to even back out bug 280959 because of it :(

Comment 2

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

DidDoReflow() may case mViewManager to become null, and that is nothing new.
This is similar problem to what we have in PresShell::DoFlushPendingNotifications

Comment 3

[Mathieu Fenniak]

Attachment: patch 1

Crash occurs during simulated mouse click fired in nsPresShell::CompleteMoveInner, which is during a reflow.  Assertions begin firing after the beforecut event is fired, in nsCSSFrameConstructor::ContentRemoved, due to frames being constructed/destructed during reflow.  By the time control returns to nsPresShell::DidDoReflow, it appears the PresShell object has been freed.

I wasn't sure of the best approach to addressing this, so I stuck to code that I know.  Attached patch avoids firing the events during reflow, which seems logical to me.  beforecut is still fired in the given test case; it fires due to the real mouse click, but is avoided during the simulated mouse click.

Comment 4

[Mathieu Fenniak]

I tried to create similar crashes related to the plaintext & html editor sites that fire these events, but I didn't find any crashes.  I don't believe they are effected.

Comment 5

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

Is it possible that cut/copy/paste events get dispatched while reflowing?

Comment 6

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

Comment on attachment 278415 [details] [diff] [review]
patch 1

Couldn't you create a helper method which takes event type as parameter 
and then dispatches event if !reflowing. Return value
could be whether the 
command is enabled or not.

Comment 7

[Mathieu Fenniak]

Attachment: patch 2

This much nicer patch introduces a FireClipboardEvent function that replaces code that was firing the clipboard events.  FireClipboardEvent does not fire events during reflow, which prevents this crash.

I'm not sure if it is possible for cut/copy/paste (rather than beforecut/beforecopy/beforepaste) to occur during a reflow, but they are prevented from firing in that case as well by this new patch.

I've run this patch against the unit tests from 280959 as well, and they continue to pass.

Comment 8

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

Comment on attachment 278793 [details] [diff] [review]
patch 2

> NS_IMETHODIMP DocumentViewerImpl::CopySelection()
> {
>   NS_ENSURE_TRUE(mPresShell, NS_ERROR_NOT_INITIALIZED);

You can remove this, because FireClipboardEvent checks that too.
Same elsewhere.


>+  PRBool preventDefault;
>+  nsresult rv = FireClipboardEvent(NS_COPY, &preventDefault);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  if (preventDefault)
>+    return NS_OK;
>+
>+  // It's possible that firing copy event closed the window.
>+  if (!mPresShell)
>+    return NS_OK;

Maybe after each FireClipboardEvent call (if needed and when applicable):
if (NS_FAILED(rv) || preventDefault)
  return rv;

And move the if(!mPresShell) to FireClipboardEvent, maybe
NS_ENSURE_STATE(mPresShell) before returning from the method.

>+nsresult DocumentViewerImpl::FireClipboardEvent(PRUint32 msg,
>+                                                PRBool* aPreventDefault)
>+{
>+  NS_ENSURE_ARG_POINTER(aPreventDefault);

This is internal method, so we can assume that it is called properly.
So no need for NS_ENSURE_ARG_POINTER


>+  nsresult rv = FireClipboardEvent(NS_BEFORECOPY, &preventDefault);

Couldn't you pass aCopyable as the second parameter?

I could still review the new patch, so marking r-

Comment 9

[Mathieu Fenniak]

Attachment: patch 3

Updated patch addresses comments about the last patch.  New patch has been tested to avoid this crash, and pass 280969's unit tests.

Comment 10

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

Comment on attachment 278956 [details] [diff] [review]
patch 3

>+  nsresult rv = mPresShell->IsReflowLocked(&isReflowing);
>+  if (NS_FAILED(rv) || isReflowing)
>+    return NS_ERROR_ABORT;

Should this actually return NS_OK, so that the current behavior wouldn't
change.

Although the patch is a bit hackish, IMO it is ok enough. :)

Comment 11

[Mathieu Fenniak]

Attachment: patch 4

Changed NS_ERROR_ABORT -> NS_OK.  Carrying over r+.

Agreed that this is a little hacky, but that matches the crash event being fired from hacky looking code in nsPresShell::CompleteMoveInner (simulating a mouse click).

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 278977 [details] [diff] [review]
patch 4

sr+a=jst

Comment 13

[Martijn Wargers (dead)]

Checking in nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.543; previous revision: 1.542
done

Checked into trunk.

Thanks for fixing, Mathieu.

Comment 14

[Jeff Walden [:Waldo]]

Could we get this test made into a Mochitest, please, and committed to CVS?