394818 - Crash [@ nsBlockFrame::DoRemoveFrame] with iframe, onbeforecopy and focusing

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase (crashes trunk on mac, windows)

See testcase, which crashes current trunk builds after 200ms.
I think this is a regression from bug 280959, because I need to use the onbeforecopy event handler for the crash to happen.

http://crash-stats.mozilla.com/report/index/088fafe8-5a83-11dc-a246-001a4bd46e84
0  	nsBlockFrame::DoRemoveFrame(nsIFrame*, int, int)  	 mozilla/layout/generic/nsBlockFrame.cpp:5290
1 	nsBlockFrame::RemoveFrame(nsIAtom*, nsIFrame*) 	mozilla/layout/generic/nsBlockFrame.cpp:4976
2 	nsFrameManager::RemoveFrame(nsIFrame*, nsIAtom*, nsIFrame*) 	mozilla/layout/base/nsFrameManager.cpp:690
3 	nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, int, int) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:9527
4 	nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:11075
5 	nsCSSFrameConstructor::RestyleElement(nsIContent*, nsIFrame*, nsChangeHint) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:9940
6 	nsCSSFrameConstructor::ProcessOneRestyle(nsIContent*, nsReStyleHint, nsChangeHint) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:12965
7 	nsCSSFrameConstructor::ProcessPendingRestyles() 	mozilla/layout/base/nsCSSFrameConstructor.cpp:13018
8 	nsCSSFrameConstructor::RestyleEvent::Run() 	mozilla/layout/base/nsCSSFrameConstructor.cpp:13089
9 	nsThread::ProcessNextEvent(int, int*) 	mozilla/xpcom/threads/nsThread.cpp:490
10 	NS_ProcessNextEvent_P(nsIThread*, int) 	nsThreadUtils.cpp:227
11 	nsBaseAppShell::Run() 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154
12 	nsAppStartup::Run() 	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:170
13 	XRE_main 	mozilla/toolkit/xre/nsAppRunner.cpp:3069
14 	main 	mozilla/browser/app/nsBrowserApp.cpp:153
15 	WinMain 	mozilla/browser/app/nsBrowserApp.cpp:166
16 	__tmainCRTStartup 	crtexe.c:589
17 	BaseProcessStart

Comment 1

[Martijn Wargers (dead)]

Maybe related to bug 387632?

Comment 2

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

WFM on Linux.

Comment 3

[Martijn Wargers (dead)]

In that case it's definitely related to bug 387632.

Comment 4

[Johnny Stenback (:jst)]

Sounds like a layout bug to me. Marking blocker, and giving this to dholbert for now. Feel free to unmark or renominate or reassign as needed.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase2 (crashes trunk on mac, windows)

Here's a slightly simplified testcase.

Notes:
  -- It looks like changing certain body style attributes (e.g. setting "display: inline" or "display: none") triggers an "onbeforecopy" signal.  And if "beforeCopy()" runs and removes the documentElement while the iframe is focused, then we get into trouble.
  -- The "setTimeout" wrapper around the style change isn't required for the crash.  It's just in there to demonstrate that the crash doesn't depend on the style change happening right away.

Comment 6

[Daniel Holbert [:dholbert]]

Also: Like bug 387632, this crashes on Windows *and* Mac.  
(but not Linux, as has been said above)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: backtrace just before crash

Here's a backtrace just before crash.  (Crash occurs at the end of this "~nsView" method)

Of particular interest are these (shortened) lines:

#0  nsView::~nsView (this=0x2eee28d0, __in_chrg=3)
#1  in nsView::~nsView (this=0x2eee28d0)
#2  in nsIView::Destroy (this=0x2eee28d0)
#3  in nsFrame::Destroy (this=0x34540d4)
...
#39 in DocumentViewerImpl::FireClipboardEvent
...
#77 in -[ChildView sendFocusEvent:] at mozilla/widget/src/cocoa/nsChildView.mm:2085
...
#82 in nsView::~nsView (this=0x2eee28d0, __in_chrg=3)
#83 in nsView::~nsView (this=0x2eee28d0)
#84 in nsIView::Destroy (this=0x2eee28d0)
#85 in nsFrame::Destroy (this=0x34540d4)
...
#100 0x09eb5274 in nsCSSFrameConstructor::RecreateFramesForContent

First of all, notice that the objects destroyed in levels 0-3 are **the same as the ones being destroyed at levels 82-85.**  Not good...

It looks like this is what happens:
  1. We restyle the body, causing frames to be destroyed and rebuilt
  2. While we're destroying the iFrame, it loses focus.  This triggers some cocoa code for transferring focus, and eventually triggers an "onbeforecopy" event.  (which I guess get can be triggered by focus events)
  3. We run the "beforeCopy" JS code, which modifies the frame tree. This happens **while we're already halfway through destroying the frame tree from the body restyle**.  Again, not good.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: reference for testcase2

Here's a copy of testcase2, but modified so that "onbeforecopy" fires asynchronously via setTimeout(xxx,0).  This shouldn't crash.

Comment 9

[Martijn Wargers (dead)]

The testcases stopped crashing when bug 415921 appeared, so I can't really test whether this was fixed by bug 399852.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> The testcases stopped crashing when bug 415921 appeared, so I can't really test
> whether this was fixed by bug 399852.

Regardless of whether bug 399852 fixed this, this bug should no longer be an issue -- bug 418457 just landed, removing support for onbefore* events.

 --> Resolving as WFM.

(If/when we re-add onbefore* support in the future, we should reinvestigate this, though.)

Comment 11

[Daniel Holbert [:dholbert]]

Pushed 'testcase' and 'testcase2' (with reftest-wait added, because of JS usage) as http://hg.mozilla.org/mozilla-central/rev/d0e0ecbb1af6