Last Comment Bug 336574 - Crash when window gets destroyed during an overflow event
: Crash when window gets destroyed during an overflow event
Status: RESOLVED FIXED
[sg:critical?]
: crash, regression, testcase, verified1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 305120
  Show dependency treegraph
 
Reported: 2006-05-04 02:19 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2007-08-17 10:21 PDT (History)
6 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
iframe for testcase (480 bytes, text/html)
2006-05-04 02:20 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase (343 bytes, text/html)
2006-05-04 02:21 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase2 (1002 bytes, text/html)
2006-05-04 02:28 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase3 (1.06 KB, text/html)
2006-05-05 09:01 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
possible patch, for trunk (5.97 KB, patch)
2006-11-26 16:20 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
proposed patch, for trunk (10.07 KB, patch)
2006-12-04 12:26 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
proposed patch v2 (10.04 KB, patch)
2006-12-08 02:38 PST, Olli Pettay [:smaug]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
ASync -> Async (10.04 KB, patch)
2006-12-14 16:08 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
for branches (3.03 KB, patch)
2007-04-17 05:53 PDT, Olli Pettay [:smaug]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
for branches (3.04 KB, patch)
2007-04-18 20:51 PDT, Olli Pettay [:smaug]
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-04 02:19:03 PDT
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 [:mwargers] (not working for Mozilla) 2006-05-04 02:20:15 PDT
Created attachment 220757 [details]
iframe for testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-04 02:21:29 PDT
Created attachment 220758 [details]
testcase

I crash when I click the button.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-04 02:28:58 PDT
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.
Comment 4 Daniel Veditz [:dveditz] 2006-05-04 15:50:39 PDT
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 [:mwargers] (not working for Mozilla) 2006-05-05 09:01:34 PDT
Created attachment 220934 [details]
testcase3

Not surprisingly, a similar crash happens with the underflow event.
Comment 6 Olli Pettay [:smaug] 2006-11-26 16:20:13 PST
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)
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-11-28 15:23:31 PST
This is scary. Can we just have a solution that makes overflow/underflow events asynchronous?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-11-28 15:26:05 PST
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] 2006-11-29 02:38:32 PST
We need similar thing for both attr changes and event dispatching.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-11-30 17:25:46 PST
nsGfxScrollFrame is the only thing that posts attribute changes too. Let's make that asynchronous in nsGfxScrollFrame as well.
Comment 11 Olli Pettay [:smaug] 2006-12-04 12:26:22 PST
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.
Comment 12 Olli Pettay [:smaug] 2006-12-04 12:27:43 PST
+  // nsASyncScrollPortEvent owns  ....
owns aEvent
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-12-06 14:09:11 PST
> 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] 2006-12-08 02:38:37 PST
Created attachment 247947 [details] [diff] [review]
proposed patch v2

menus work and it should be ok to use the prescontext.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-12-14 15:55:13 PST
Comment on attachment 247947 [details] [diff] [review]
proposed patch v2

But PLEASE change to nsAsync* (not nsASync*)
Comment 16 Olli Pettay [:smaug] 2006-12-14 16:08:16 PST
Created attachment 248694 [details] [diff] [review]
ASync -> Async
Comment 17 Olli Pettay [:smaug] 2006-12-14 16:24:37 PST
Checked in.
Comment 18 Olli Pettay [:smaug] 2007-04-16 18:00:43 PDT
Roc, do you think the patch is too scary for branches?
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-04-16 20:23:18 PDT
I think this is OK for branches.
Comment 20 Olli Pettay [:smaug] 2007-04-17 05:53:43 PDT
Created attachment 261794 [details] [diff] [review]
for branches
Comment 21 Olli Pettay [:smaug] 2007-04-18 13:51:06 PDT
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] 2007-04-18 16:32:54 PDT
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] 2007-04-18 20:51:56 PDT
Created attachment 262078 [details] [diff] [review]
for branches
Comment 24 Daniel Veditz [:dveditz] 2007-04-19 10:20:52 PDT
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 - use ni] 2007-05-08 15:08:34 PDT
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 - use ni] 2007-05-08 16:36:20 PDT
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.

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