336574 - Crash when window gets destroyed during an overflow event

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Martijn Wargers (dead)]

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

Comment 1

[Martijn Wargers (dead)]

Attachment: iframe for testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

I crash when I click the button.

Comment 3

[Martijn Wargers (dead)]

Attachment: 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.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase3

Not surprisingly, a similar crash happens with the underflow event.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is scary. Can we just have a solution that makes overflow/underflow events asynchronous?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

We need similar thing for both attr changes and event dispatching.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsGfxScrollFrame is the only thing that posts attribute changes too. Let's make that asynchronous in nsGfxScrollFrame as well.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

+  // nsASyncScrollPortEvent owns  ....
owns aEvent

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch v2

menus work and it should be ok to use the prescontext.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 247947 [details] [diff] [review]
proposed patch v2

But PLEASE change to nsAsync* (not nsASync*)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ASync -> Async

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Checked in.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Roc, do you think the patch is too scary for branches?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think this is OK for branches.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for branches

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 261794 [details] [diff] [review]
for branches

Note, there should be return after PL_DestroyEvent, scrollPortEvent owns event.

Comment 22

[Daniel Veditz [:dveditz]]

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

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for branches

Comment 24

[Daniel Veditz [:dveditz]]

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

Comment 25

[Marcia Knous [:marcia]]

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.

Comment 26

[Marcia Knous [:marcia]]

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.