Crash when window gets destroyed during an overflow event

RESOLVED FIXED

Status

()

Core
DOM: Events
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: smaug)

Tracking

(5 keywords)

Trunk
x86
Windows XP
crash, regression, testcase, verified1.8.0.12, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
See upcoming testcase.
This regressed after bug 305120 got fixed. With Mozilla1.7, I don't crash with the testcase.

Not sure whether this is security sensitive, but just to be sure, I've made it security sensitive.

Talkback ID: TB18281315M
0x00000000
PresShell::HandlePostedDOMEvents   PresShell::DidDoReflow   PresShell::WillPaint
(Reporter)

Comment 1

11 years ago
Created attachment 220757 [details]
iframe for testcase
(Reporter)

Comment 2

11 years ago
Created attachment 220758 [details]
testcase

I crash when I click the button.
(Reporter)

Comment 3

11 years ago
Created attachment 220760 [details]
testcase2

Here a (maybe) interesting testcase, which I made when trying to make an automated crash testcase. You should not see an iframe with the testcase, but it is still painted.
In a 1.5.0.x debug build I get a stack that looks like the one in talkback

PresShell::HandlePostedDOMEvents() Line 5260 + 0xa
PresShell::DidDoReflow() Line 6788
PresShell::ProcessReflowCommands(int aInterruptible=0x00000001) Line 6936	
PresShell::WillPaint() Line 6514
nsViewManager::FlushPendingInvalidates() Line 4409 + 0xe
nsViewManager::EnableRefresh(unsigned int aUpdateFlags=0x00000000)
nsViewManager::EndUpdateViewBatch(unsigned int aUpdateFlags=0x00000000)
nsViewManager::EndUpdateViewBatch(unsigned int aUpdateFlags=0x00000000)
nsCSSFrameConstructor::RestyleEvent::HandleEvent()
HandleRestyleEvent(PLEvent * aEvent=0x042adfb0)
PL_HandleEvent(PLEvent * self=0x042adfb0)
 ...

Crashes on the line "NS_RELEASE(node->content);", both node and "this" are deleted objects.
Whiteboard: [sg:critical?]
(Reporter)

Comment 5

11 years ago
Created attachment 220934 [details]
testcase3

Not surprisingly, a similar crash happens with the underflow event.
Created attachment 246640 [details] [diff] [review]
possible patch, for trunk

We could make posted events and attribute changes to be really async, I think. Need to still look at few cases to make sure that this doesn't 
regress anything. (Will cancel the review request if I find any problems)
Attachment #246640 - Flags: review?(roc)
This is scary. Can we just have a solution that makes overflow/underflow events asynchronous?
Hmm. Overflow/underflow events is the only use of this mechanism. Why don't we just rip this out of presshell and make nsGfxScrollFrame do this directly and asynchronously?
We need similar thing for both attr changes and event dispatching.
nsGfxScrollFrame is the only thing that posts attribute changes too. Let's make that asynchronous in nsGfxScrollFrame as well.
Attachment #246640 - Flags: review?(roc)
Created attachment 247455 [details] [diff] [review]
proposed patch, for trunk

This should be enough.

I think I was wrong about attribute changes, those shouldn't cause crashes (mutation events don't propagate from native anonymous content).
I'll anyway file a bug to remove PostAttributeChange since that is bad
API, IMO.
Assignee: events → Olli.Pettay
Attachment #246640 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #247455 - Flags: superreview?(roc)
Attachment #247455 - Flags: review?(roc)
+  // nsASyncScrollPortEvent owns  ....
owns aEvent
> nsASyncScrollPortEvent

nsAsyncScrollPortEvent

+  nsScrollPortEvent*      mEvent;

Use nsAutoPtr

Is it really OK to dispatch an event at a dead prescontext?

Can you verify that scrolling menus still work after this?
Created attachment 247947 [details] [diff] [review]
proposed patch v2

menus work and it should be ok to use the prescontext.
Attachment #247455 - Attachment is obsolete: true
Attachment #247947 - Flags: superreview?(roc)
Attachment #247947 - Flags: review?(roc)
Attachment #247455 - Flags: superreview?(roc)
Attachment #247455 - Flags: review?(roc)
Comment on attachment 247947 [details] [diff] [review]
proposed patch v2

But PLEASE change to nsAsync* (not nsASync*)
Attachment #247947 - Flags: superreview?(roc)
Attachment #247947 - Flags: superreview+
Attachment #247947 - Flags: review?(roc)
Attachment #247947 - Flags: review+
Created attachment 248694 [details] [diff] [review]
ASync -> Async
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Roc, do you think the patch is too scary for branches?
I think this is OK for branches.
Created attachment 261794 [details] [diff] [review]
for branches
Attachment #261794 - Flags: superreview?(roc)
Attachment #261794 - Flags: review?(roc)
Attachment #261794 - Flags: superreview?(roc)
Attachment #261794 - Flags: superreview+
Attachment #261794 - Flags: review?(roc)
Attachment #261794 - Flags: review+
Comment on attachment 261794 [details] [diff] [review]
for branches

Note, there should be return after PL_DestroyEvent, scrollPortEvent owns event.
Attachment #261794 - Flags: approval1.8.1.4?
Attachment #261794 - Flags: approval1.8.0.12?
Comment on attachment 261794 [details] [diff] [review]
for branches

So you're saying you intend to check in something different than what you're asking approval on? It's probably best all around if you attach a new patch and request approval on that one. If that's the only change no need for a re-review
Attachment #261794 - Flags: approval1.8.1.4?
Attachment #261794 - Flags: approval1.8.0.12?
Created attachment 262078 [details] [diff] [review]
for branches
Attachment #262078 - Flags: approval1.8.1.4?
Attachment #262078 - Flags: approval1.8.0.12?
Comment on attachment 262078 [details] [diff] [review]
for branches

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Attachment #262078 - Flags: approval1.8.1.4?
Attachment #262078 - Flags: approval1.8.1.4+
Attachment #262078 - Flags: approval1.8.0.12?
Attachment #262078 - Flags: approval1.8.0.12+
Keywords: fixed1.8.0.12, fixed1.8.1.4
verified fixed on the 1.8 branch using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/2007050804 BonEcho/2.0.0.4pre. No crashes with the testcases in Comments 2 and 3. Adding branch verified keyword.
Keywords: fixed1.8.1.4 → verified1.8.1.4
verified fixed on the 1.8.0 branch using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.12pre) Gecko/20070508 Firefox/1.5.0.12pre. No crashes with the
testcases in Comments 2 and 3. Adding branch verified keyword.
Keywords: fixed1.8.0.12 → verified1.8.0.12
Group: security
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.