Closed Bug 393696 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Events, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: mfenniak-moz)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 3 obsolete files)

Attached file 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
This is really bad, we might need to even back out bug 280959 because of it :(
DidDoReflow() may case mViewManager to become null, and that is nothing new.
This is similar problem to what we have in PresShell::DoFlushPendingNotifications
Attached patch patch 1 (obsolete) — Splinter Review
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.
Assignee: nobody → mfenniak-moz
Status: NEW → ASSIGNED
Attachment #278415 - Flags: review?(Olli.Pettay)
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.
Is it possible that cut/copy/paste events get dispatched while reflowing?
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.
Attachment #278415 - Flags: review?(Olli.Pettay) → review-
Attached patch patch 2 (obsolete) — Splinter Review
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.
Attachment #278415 - Attachment is obsolete: true
Attachment #278793 - Flags: review?(Olli.Pettay)
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-
Attachment #278793 - Flags: review?(Olli.Pettay) → review-
Attached patch patch 3 (obsolete) — Splinter Review
Updated patch addresses comments about the last patch.  New patch has been tested to avoid this crash, and pass 280969's unit tests.
Attachment #278793 - Attachment is obsolete: true
Attachment #278956 - Flags: review?(Olli.Pettay)
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. :)
Attachment #278956 - Flags: review?(Olli.Pettay) → review+
Attached patch patch 4Splinter Review
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).
Attachment #278956 - Attachment is obsolete: true
Attachment #278977 - Flags: superreview?(jonas)
Attachment #278977 - Flags: review+
Comment on attachment 278977 [details] [diff] [review]
patch 4

sr+a=jst
Attachment #278977 - Flags: superreview?(jonas)
Attachment #278977 - Flags: superreview+
Attachment #278977 - Flags: approval1.9+
Keywords: checkin-needed
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Could we get this test made into a Mochitest, please, and committed to CVS?
Flags: in-testsuite?
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.