Crash [@ nsTypedSelection::ScrollPointIntoClipView] with toggling iframe, mousedown and mousemove

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Selection
P3
critical
VERIFIED FIXED
10 years ago
5 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: mats)

Tracking

(4 keywords)

Trunk
mozilla1.9.2a1
crash, testcase, verified1.9.0.6, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.6 +
wanted1.9.0.x +
wanted1.8.1.x -
wanted1.8.0.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] method call on deleted nsView (post 1.8-branch), crash signature)

Attachments

(10 attachments, 4 obsolete attachments)

1.73 KB, text/html
Details
1.51 KB, text/html
Details
13.08 KB, text/plain
Details
3.34 KB, text/plain
Details
14.71 KB, text/html
Details
8.95 KB, text/plain
Details
7.83 KB, text/plain
Details
7.63 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.33 KB, patch
Details | Diff | Splinter Review
11.65 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
This seems to have become worksforme, in current trunk build.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 2

10 years ago
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
(Reporter)

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 3

10 years ago
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)
Assignee: nobody → mats.palmgren
Status: REOPENED → ASSIGNED
(Assignee)

Updated

10 years ago
Whiteboard: [sg:critical?] method call on deleted nsView
(Assignee)

Comment 4

10 years ago
Created attachment 331511 [details] [diff] [review]
Patch A, rev. 1
Attachment #331511 - Flags: superreview?(roc)
Attachment #331511 - Flags: review?(roc)
(Assignee)

Comment 5

10 years ago
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()).
(Assignee)

Comment 6

10 years ago
Created attachment 331516 [details] [diff] [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)
Attachment #331516 - Flags: superreview?(roc)
Attachment #331516 - Flags: review?(roc)
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)?
(Reporter)

Updated

10 years ago
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
Mats, any updates here?
(Assignee)

Updated

10 years ago
Attachment #331511 - Attachment is obsolete: true
Attachment #331511 - Flags: superreview?(roc)
Attachment #331511 - Flags: review?(roc)
(Assignee)

Comment 9

10 years ago
Comment on attachment 331511 [details] [diff] [review]
Patch A, rev. 1

The combined patch still crashes unfortunately...
I'm looking in to it.
(Assignee)

Updated

10 years ago
Attachment #331516 - Attachment is obsolete: true
Attachment #331516 - Flags: superreview?(roc)
Attachment #331516 - Flags: review?(roc)
(Assignee)

Comment 10

10 years ago
Created attachment 346898 [details]
Stack: deleting nsView while in use by UpdateWidgetsForView()
(Assignee)

Comment 11

10 years ago
Created attachment 346899 [details]
Stack: nsAutoScrollTimer::Notify() using destroyed pres context
(Assignee)

Comment 12

10 years ago
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)
(Assignee)

Updated

10 years ago
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 13

10 years ago
Created attachment 346918 [details] [diff] [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.
Attachment #346918 - Flags: superreview?(roc)
Attachment #346918 - Flags: review?(roc)
Attachment #346918 - Flags: superreview?(roc)
Attachment #346918 - Flags: superreview+
Attachment #346918 - Flags: review?(roc)
Attachment #346918 - Flags: review+
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.
Whiteboard: [sg:critical?] method call on deleted nsView → [sg:critical?] method call on deleted nsView [needs landing]
There's a reviewed patch here.  Can we get this landed ASAP as this is on our "Top Security Bugs" list?
(Assignee)

Comment 16

10 years ago
Created attachment 351805 [details] [diff] [review]
crashtest.diff (needs privileges though, so can't be used yet)
(Assignee)

Comment 17

10 years ago
Created attachment 351825 [details] [diff] [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...
(Assignee)

Comment 18

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Depends on: 448676
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical?] method call on deleted nsView [needs landing] → [sg:critical?] method call on deleted nsView
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

10 years ago
Attachment #351825 - Attachment is obsolete: true
(In reply to comment #20)

It seems we both updated the bug at the same time ;->
(Assignee)

Comment 22

10 years ago
Comment on attachment 346918 [details] [diff] [review]
Patch rev. 2
[Checkin: Comment 18 & 19]

Low risk. No performance impact.
Attachment #346918 - Flags: approval1.9.0.6?
(Assignee)

Updated

10 years ago
Flags: blocking1.9.0.6?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.6?
Flags: blocking1.9.0.6+
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?
Whiteboard: [sg:critical?] method call on deleted nsView → [sg:moderate] method call on deleted nsView
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.
Attachment #346918 - Flags: approval1.9.0.6? → approval1.9.0.6+
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 '?').
Whiteboard: [sg:moderate] method call on deleted nsView → [sg:critical?] method call on deleted nsView
(Assignee)

Comment 26

10 years ago
Created attachment 354486 [details] [diff] [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?
Attachment #354486 - Flags: superreview?(roc)
Attachment #354486 - Flags: review?(roc)
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.
(Assignee)

Comment 28

10 years ago
Ok, I think that will work too.
An alternative is to make mIsDestroying public and access it directly.
(Assignee)

Comment 29

10 years ago
Created attachment 354527 [details] [diff] [review]
Patch for 1.9.0 (no UUID change)
Attachment #354486 - Attachment is obsolete: true
Attachment #354527 - Flags: superreview?(roc)
Attachment #354527 - Flags: review?(roc)
Attachment #354486 - Flags: superreview?(roc)
Attachment #354486 - Flags: review?(roc)
Attachment #354527 - Flags: superreview?(roc)
Attachment #354527 - Flags: superreview+
Attachment #354527 - Flags: review?(roc)
Attachment #354527 - Flags: review+
(Assignee)

Comment 30

10 years ago
Comment on attachment 354527 [details] [diff] [review]
Patch for 1.9.0 (no UUID change)

See comments above regarding the UUID cheat.
Attachment #354527 - Flags: approval1.9.0.6?
Attachment #354527 - Flags: approval1.9.0.6? → approval1.9.0.6+
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.
Keywords: fixed1.9.1 → verified1.9.1
(Assignee)

Comment 33

10 years ago
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
(Assignee)

Updated

10 years ago
Keywords: fixed1.9.0.6
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.
Keywords: fixed1.9.0.6 → verified1.9.0.6

Updated

10 years ago
Keywords: verified1.9.1

Comment 35

10 years ago
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.
Keywords: verified1.9.1

Comment 36

10 years ago
Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED

Comment 37

10 years ago
we don't see any crash on 1.8. Is this a 1.9 regression?
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?

Comment 38

9 years ago
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!
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x-
Whiteboard: [sg:critical?] method call on deleted nsView → [sg:critical?] method call on deleted nsView (post 1.8-branch)
crash test added
http://hg.mozilla.org/mozilla-central/rev/6c67d75c434e
Flags: in-testsuite? → in-testsuite+
(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.
Flags: in-testsuite+ → in-testsuite?

Updated

8 years ago
Crash Signature: [@ nsTypedSelection::ScrollPointIntoClipView]
(Assignee)

Comment 43

5 years ago
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
You need to log in before you can comment on or make changes to this bug.