Last Comment Bug 421839 - Crash [@ nsTypedSelection::ScrollPointIntoClipView] with toggling iframe, mousedown and mousemove
: Crash [@ nsTypedSelection::ScrollPointIntoClipView] with toggling iframe, mou...
Status: VERIFIED FIXED
: [sg:critical?] method call on deleted...
: crash, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: Selection
: Trunk
: All All
: P3 critical (vote)
: mozilla1.9.2a1
Assigned To: Mats Palmgren [:mats]
: selection
:
: 448676
:
  Show dependency treegraph
 
Reported: 2008-03-09 18:43 PDT by Martijn Wargers [:mw22] (QA - IRC nick: mw22)
Modified: 2010-09-17 14:38 PDT (History)
16 users (show)
roc: blocking1.9.1+
dveditz: blocking1.9.0.6+
dveditz: wanted1.9.0.x+
asac: wanted1.8.1.x-
asac: wanted1.8.0.x-
bclary: in‑testsuite?
See Also:
Crash Signature:
[@ nsTypedSelection::ScrollPointIntoClipView]


Attachments
testcase (1.73 KB, text/html)
2008-03-09 18:43 PDT, Martijn Wargers [:mw22] (QA - IRC nick: mw22)
no flags Details
testcase2 (1.51 KB, text/html)
2008-07-25 19:05 PDT, Martijn Wargers [:mw22] (QA - IRC nick: mw22)
no flags Details
Stack: frame destruction during ScrollPointIntoView (13.08 KB, text/plain)
2008-07-27 11:05 PDT, Mats Palmgren [:mats]
no flags Details
Patch A, rev. 1 (2.83 KB, patch)
2008-07-27 11:13 PDT, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
Stack: "GetPrimaryFrameFor() called while nsFrameManager is being destroyed!" (3.34 KB, text/plain)
2008-07-27 11:20 PDT, Mats Palmgren [:mats]
no flags Details
Patch B, rev. 1 (769 bytes, patch)
2008-07-27 11:29 PDT, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
Stack: deleting nsView while in use by UpdateWidgetsForView() (14.71 KB, text/html)
2008-11-07 08:27 PST, Mats Palmgren [:mats]
no flags Details
Stack: nsAutoScrollTimer::Notify() using destroyed pres context (8.95 KB, text/plain)
2008-11-07 08:28 PST, Mats Palmgren [:mats]
no flags Details
Stack: nsScrollPortView::ScrollTo() using destroyed nsView (7.83 KB, text/plain)
2008-11-07 08:32 PST, Mats Palmgren [:mats]
no flags Details
Patch rev. 2 [Checkin: Comment 18 & 19] (7.63 KB, patch)
2008-11-07 09:54 PST, Mats Palmgren [:mats]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
crashtest.diff (needs privileges though, so can't be used yet) (4.33 KB, patch)
2008-12-07 10:40 PST, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
mochitest.diff (6.49 KB, patch)
2008-12-07 14:48 PST, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
Patch for 1.9.0 (12.65 KB, patch)
2008-12-26 06:04 PST, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
Patch for 1.9.0 (no UUID change) (11.65 KB, patch)
2008-12-26 17:40 PST, Mats Palmgren [:mats]
roc: review+
roc: superreview+
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review

Summon comment box

Description Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-03-09 18:43:07 PDT
Created attachment 308355 [details]
testcase

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
Comment 1 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-04-02 14:14:23 PDT
This seems to have become worksforme, in current trunk build.
Comment 2 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-07-25 19:05:01 PDT
Created attachment 331390 [details]
testcase2

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
Comment 3 Mats Palmgren [:mats] 2008-07-27 11:05:30 PDT
Created attachment 331510 [details]
Stack: frame destruction during ScrollPointIntoView

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)
Comment 4 Mats Palmgren [:mats] 2008-07-27 11:13:27 PDT
Created attachment 331511 [details] [review]
Patch A, rev. 1
Comment 5 Mats Palmgren [:mats] 2008-07-27 11:20:49 PDT
Created attachment 331513 [details]
Stack: "GetPrimaryFrameFor() called while nsFrameManager is being destroyed!"

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()).
Comment 6 Mats Palmgren [:mats] 2008-07-27 11:29:13 PDT
Created attachment 331516 [details] [review]
Patch B, rev. 1

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)
Comment 7 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-07-29 00:24:38 PDT
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)?
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2008-11-06 18:19:19 PST
Mats, any updates here?
Comment 9 Mats Palmgren [:mats] 2008-11-06 23:14:07 PST
Comment on attachment 331511 [details] [review]
Patch A, rev. 1

The combined patch still crashes unfortunately...
I'm looking in to it.
Comment 10 Mats Palmgren [:mats] 2008-11-07 08:27:24 PST
Created attachment 346898 [details]
Stack: deleting nsView while in use by UpdateWidgetsForView()
Comment 11 Mats Palmgren [:mats] 2008-11-07 08:28:47 PST
Created attachment 346899 [details]
Stack: nsAutoScrollTimer::Notify() using destroyed pres context
Comment 12 Mats Palmgren [:mats] 2008-11-07 08:32:18 PST
Created attachment 346900 [details]
Stack: nsScrollPortView::ScrollTo() using destroyed nsView

Probably as a result of "deleting nsView while in use by UpdateWidgetsForView()"
(got this one on MacOSX, the other one on Linux)
Comment 13 Mats Palmgren [:mats] 2008-11-07 09:54:27 PST
Created attachment 346918 [details] [review]
Patch rev. 2
[Checkin: Comment 18 & 19]


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 14 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-11-09 13:58:37 PST
Comment on attachment 346918 [details] [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.
Comment 15 Brandon Sterne (:bsterne) 2008-12-04 10:37:40 PST
There's a reviewed patch here.  Can we get this landed ASAP as this is on our "Top Security Bugs" list?
Comment 16 Mats Palmgren [:mats] 2008-12-07 10:40:36 PST
Created attachment 351805 [details] [review]
crashtest.diff (needs privileges though, so can't be used yet)
Comment 17 Mats Palmgren [:mats] 2008-12-07 14:48:09 PST
Created attachment 351825 [details] [review]
mochitest.diff

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...
Comment 18 Mats Palmgren [:mats] 2008-12-07 18:59:20 PST
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
Comment 21 Serge Gautherie (:sgautherie) 2008-12-10 15:24:18 PST
(In reply to comment #20)

It seems we both updated the bug at the same time ;->
Comment 22 Mats Palmgren [:mats] 2008-12-10 15:31:32 PST
Comment on attachment 346918 [details] [review]
Patch rev. 2
[Checkin: Comment 18 & 19]

Low risk. No performance impact.
Comment 23 Daniel Veditz 2008-12-19 11:28:02 PST
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 24 Daniel Veditz 2008-12-19 11:29:34 PST
Comment on attachment 346918 [details] [review]
Patch rev. 2
[Checkin: Comment 18 & 19]

Approved for 1.9.0.6, a=dveditz for release-drivers.
Comment 25 Daniel Veditz 2008-12-19 11:30:36 PST
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 '?').
Comment 26 Mats Palmgren [:mats] 2008-12-26 06:04:07 PST
Created attachment 354486 [details] [review]
Patch for 1.9.0

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?
Comment 27 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-12-26 12:51:55 PST
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.
Comment 28 Mats Palmgren [:mats] 2008-12-26 17:38:52 PST
Ok, I think that will work too.
An alternative is to make mIsDestroying public and access it directly.
Comment 29 Mats Palmgren [:mats] 2008-12-26 17:40:21 PST
Created attachment 354527 [details] [review]
Patch for 1.9.0 (no UUID change)
Comment 30 Mats Palmgren [:mats] 2008-12-26 17:54:28 PST
Comment on attachment 354527 [details] [review]
Patch for 1.9.0 (no UUID change)

See comments above regarding the UUID cheat.
Comment 31 Daniel Veditz 2009-01-05 11:31:40 PST
Comment on attachment 354527 [details] [review]
Patch for 1.9.0 (no UUID change)

Approved for 1.9.0.6, a=dveditz for release-drivers.
Comment 32 Marcia Knous [:marcia] 2009-01-06 10:26:09 PST
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 33 Mats Palmgren [:mats] 2009-01-07 15:27:01 PST
Comment on attachment 354527 [details] [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
Comment 34 Al Billings [:abillings] 2009-01-14 17:23:01 PST
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.
Comment 35 Tony Chung [:tchung] 2009-01-15 15:26:29 PST
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.
Comment 36 Tony Chung [:tchung] 2009-01-15 16:14:38 PST
Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre
Comment 37 Alexander Sack 2009-01-27 03:38:43 PST
we don't see any crash on 1.8. Is this a 1.9 regression?
Comment 38 Alexander Sack 2009-01-30 02:01:21 PST
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!
Comment 39 Bob Clary [:bc:] 2009-04-24 10:26:02 PDT
crash test added
http://hg.mozilla.org/mozilla-central/rev/6c67d75c434e
Comment 40 Bob Clary [:bc:] 2009-04-24 10:28:14 PDT
(In reply to comment #39)
> crash test added

actually was a mochitest sorry.
Comment 41 Bob Clary [:bc:] 2009-04-24 15:00:09 PDT
disable due to timeout and dupe call to SimpleTest.finish.
http://hg.mozilla.org/mozilla-central/rev/abc59dbb6e46

I'll fix and reenable later.

Note You need to log in before you can comment on or make changes to this bug.