See testcase, to get the crash, download the testcase to your computer, because it makes use of enhanced privileges. Then quickly move your mouse back and fort across the iframe. I haven't been able to find a regression range. It seems like the latest builds are easier to crash. But I have been able to crash a 2008-01-02 build at least. Maybe this will be fixed by the patch for bug 421083? http://crash-stats.mozilla.com/report/index/3d1ebc57-ee42-11dc-8b8c-001a4bd46e84 0 @0xa0b5ed8d 1 nsTypedSelection::ScrollPointIntoClipView(nsPresContext*, nsIView*, nsPoint&, int*) mozilla/layout/generic/nsSelection.cpp:5544 2 nsTypedSelection::ScrollPointIntoView(nsPresContext*, nsIView*, nsPoint&, int, int*) mozilla/layout/generic/nsSelection.cpp:5594 3 nsTypedSelection::DoAutoScrollView(nsPresContext*, nsIView*, nsPoint&, int) mozilla/layout/generic/nsSelection.cpp:5716 4 nsTypedSelection::StartAutoScrollTimer(nsPresContext*, nsIView*, nsPoint&, unsigned int) mozilla/layout/generic/nsSelection.cpp:5416 5 nsFrame::HandleDrag(nsPresContext*, nsGUIEvent*, nsEventStatus*) mozilla/layout/generic/nsFrame.cpp:2144 6 nsFrame::HandleEvent(nsPresContext*, nsGUIEvent*, nsEventStatus*) mozilla/layout/generic/nsFrame.cpp:1510 7 nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor&) mozilla/layout/base/nsPresShell.cpp:1224 8 xul.dll@0x2b1a59

This seems to have become worksforme, in current trunk build.

I guess I was wrong, I can still see this crash occuring with this testcase (also uses enhanced privileges). http://crash-stats.mozilla.com/report/index/3335720f-5ab3-11dd-90b2-001a4bd43ef6?p=1

The problem is that nsViewManager::Composite() flushes pending notifications and thus destroys arbitrary layout objects. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsSelection.cpp&rev=3.311&root=/cvsroot&mark=5549,5553#5543 (in this case the whole shell, and thus all its frames and their views - one of the views we're currently scrolling on the call stack)

After fixing the crash we still get warning (printf): GetPrimaryFrameFor() called while nsFrameManager is being destroyed! from this point: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.1117&root=/cvsroot&mark=5941#5907 We have called Destroy() for the current (this) shell in this case so we shouldn't call GetPrimaryFrameFor() (from GetCurrentEventFrame()).

Fixes the GetPrimaryFrameFor() warning. BTW, should we make that an NS_ERROR/WARNING rather than a DEBUG printf? http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsFrameManager.cpp&rev=1.259&root=/cvsroot&mark=330#322 (let me know if you want to handle this issue in a separate bug)

Make it an NS_WARNING, yes please. This patch or a new bug is fine. Should we always return null from GetCurrentEventFrame if mIsDestroying (even if mCurrentEventFrame is non-null)?

Mats, any updates here?

Comment on attachment 331511 [details] [diff] [review] Patch A, rev. 1 The combined patch still crashes unfortunately... I'm looking in to it.

Probably as a result of "deleting nsView while in use by UpdateWidgetsForView()" (got this one on MacOSX, the other one on Linux)

This patch includes the earlier two patches, with roc's nit in comment 7. Additionally, 1. add "StopAutoScrollTimer()" in DisconnectFromPresShell(), this will Cancel an outstanding timer and is necessary since it uses a raw pres context pointer (and other pointers) which could be stale. 2. use nsWeakView in UpdateWidgetsForView() to detect view destruction 3. keep strong refs on the pres shell and view manager over the viewManager->Composite() call, and give up if presShell->IsDestroying() after it. I have high hopes for the UpdateWidgetsForView() fix - I think that's a bug I've been hunting for years... Tested on Linux, MacOSX and XP. There are still a few assertions for the first testcase, but nothing too serious, they can be fixed separately.

Comment on attachment 346918 [details] [diff] [review] Patch rev. 2 [Checkin: Comment 18 & 19] It's possible for notifications to *move* views, which could make UpdateWidgetsForView behave strangely, but since it can only move views once I don't think it's possible to get into an infinite loop.

There's a reviewed patch here. Can we get this landed ASAP as this is on our "Top Security Bugs" list?

Mochitest version of crash tests, it just loads them into an iframe and let it run for 2 seconds - a bit lame but better than nothing I guess...

http://hg.mozilla.org/mozilla-central/rev/6efa9c970d64 http://hg.mozilla.org/mozilla-central/rev/f7d4bb87cda0 I also landed the mochitests briefly: http://hg.mozilla.org/mozilla-central/rev/7effc03f2f45 but it caused Tinderbox orange "Unable to restore focus, expect failures and timeouts." so I backed it out: http://hg.mozilla.org/mozilla-central/rev/ae945d36888c I filed bug 468390 for the remaining assertion for the first testcase. I don't get any significant assertions for the 2nd testcase (on Linux). -> FIXED

Fixed for 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9269759f81b7 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ecf3f6ae8ed7

Comment on attachment 346918 [details] [diff] [review] Patch rev. 2 [Checkin: Comment 18 & 19] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ecf3f6ae8ed7 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9269759f81b7

(In reply to comment #20) It seems we both updated the bug at the same time ;->

Comment on attachment 346918 [details] [diff] [review] Patch rev. 2 [Checkin: Comment 18 & 19] Low risk. No performance impact.

Unless we know of a non-privileged way to trigger this crash it's more sg:moderate than potentially sg:critical. I guess maybe you could get the user to play a mouseclicking/dragging game and then toggle the iframe during that?

Comment on attachment 346918 [details] [diff] [review] Patch rev. 2 [Checkin: Comment 18 & 19] Approved for 1.9.0.6, a=dveditz for release-drivers.

I guess worst-case-scenario thinking: we don't know that we _can't_ trigger this without privs, so it _is_ potentially critical (thus the '?').

The fix requires nsIPresShell::IsDestroying() which isn't present in 1.9.0 (it was added as part of bug 455984). This patch adds it and changes the UUID for nsIPresShell (no other changes). What's the policy for UUID changes on the 1.9.0 branch? Should I add a nsIPresShell_MOZILLA_1_9_0_BRANCH subclass like we did in the 1.8 branch?

You should just not change the UUID here. The new method is not virtual and will not break any users of the interface, so I think we can get away with just not changing the UUID. It's a bit of a cheat but the only alternative is to create nsIPresShell_MOZILLA_1_9_0_BRANCH which sucks.

Ok, I think that will work too. An alternative is to make mIsDestroying public and access it directly.

Comment on attachment 354527 [details] [diff] [review] Patch for 1.9.0 (no UUID change) See comments above regarding the UUID cheat.

Comment on attachment 354527 [details] [diff] [review] Patch for 1.9.0 (no UUID change) Approved for 1.9.0.6, a=dveditz for release-drivers.

verified fixed on the 1.9.1 branch using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090106 Shiretoko/3.1b3pre. No crash with the testcase.

Comment on attachment 354527 [details] [diff] [review] Patch for 1.9.0 (no UUID change) Checked in on CVS trunk for 1.9.0.6: mozilla/layout/base/nsPresShell.cpp 3.1121 mozilla/layout/base/nsIPresShell.h 3.223 mozilla/layout/generic/nsFrameSelection.h 1.29 mozilla/layout/generic/nsSelection.cpp 3.312 mozilla/view/src/nsViewManager.cpp 3.486

Verified for 1.9.0.6 using testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre.

sorry for the inconvenience. i meant to detect any bugs before the branch merge that were still RESO fixed, but had a verified1.9.1 keyword next to them.

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre

we don't see any crash on 1.8. Is this a 1.9 regression?

for now marking as not wanted on 1.8 branches. if you think its a real issue there please flip back the wanted flags. Thanks!

crash test added http://hg.mozilla.org/mozilla-central/rev/6c67d75c434e

(In reply to comment #39) > crash test added actually was a mochitest sorry.

disable due to timeout and dupe call to SimpleTest.finish. http://hg.mozilla.org/mozilla-central/rev/abc59dbb6e46 I'll fix and reenable later.

bug421839-2-page.html doesn't seem to be used in tests at all: http://mxr.mozilla.org/mozilla-central/search?string=bug421839-2-page.html&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central Perhaps it should be removed?

I think test_bug421839-2.html was supposed to have an <iframe> loading it: http://hg.mozilla.org/mozilla-central/rev/6c67d75c434e You can compare what I initially landed, but backed out due to orange: http://hg.mozilla.org/mozilla-central/rev/7effc03f2f45