Closed Bug 336574 Opened 18 years ago Closed 18 years ago

Crash when window gets destroyed during an overflow event

Categories

(Core :: DOM: Events, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: smaug)

References

Details

(5 keywords, Whiteboard: [sg:critical?])

Attachments

(8 files, 2 obsolete files)

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
Attached file iframe for testcase
Attached file testcase
I crash when I click the button.
Attached file 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?]
Attached file testcase3
Not surprisingly, a similar crash happens with the underflow event.
Attached patch possible patch, for trunk (obsolete) — Splinter Review
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)
Attached patch proposed patch, for trunk (obsolete) — Splinter Review
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?
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+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 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.
Attached patch for branchesSplinter Review
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?
Attached patch for branchesSplinter Review
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+
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.
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.
Group: security
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.