237975 - [FIXr]Crash with xbl generated link, opening in new tab [@ nsAttributeTextNode::DetachListener ]

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040317 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040317 Firefox/0.8.0+

See it at:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/blockquote_new.xhtml

The attached testcase you must open local on your computer.

to reproduce it:
Open the the link you see in a new tab (in the background) and repeat those
steps very quickly (ten times or so)
Then close each tab very quickly again.

In this way, you get the highest chance to get a crash, although you have a
chance of a crash right the first time.

See also:
http://forums.mozillazine.org/viewtopic.php?t=62253

This post mentions that builds younger than 2004-01-27 are vulnerable to this crash:
http://forums.mozillazine.org/viewtopic.php?t=62253#438689

Reproducible: Always
Steps to Reproduce:
1.See details
2.
3.

Actual Results:  
Crash

Expected Results:  
No crash

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase for the crash

Comment 2

[Martijn Wargers (dead)]

Attachment: Talkback crash details

Comment 3

[Boris Zbarsky [:bzbarsky]]

Martin, the talkback data is not useful on its own.  What's the talkback
incident id so we can look up the stack?

Comment 4

[Martijn Wargers (dead)]

Oh, sorry, but somehow I am not able to send the talkback.
Every time it shows me an error message:
"The Agent is unable to connect to the server. Please check your proxy servers
settings or try again later."
I've tried it many times. Shutted down the firewall. Still no go :(

Comment 5

[Martijn Wargers (dead)]

Uhm, I was not able to send the talkback to:
http://talkback.mozilla.org/spiral-bin/Collector.dll

But to Netscape (!?) worked:
http://talkback.netscape.com/spiral-bin/Collector.dll

Incident ID:
TB31483088W

It's not the same one as I attached, it's a fresh crash, at:
19-3-2004 19:20

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

0x7a1ffe1e
0x021fd71d
gklayout.dll + 0x9fa9a (0x00f8fa9a)
gklayout.dll + 0x9fa5c (0x00f8fa5c)
gklayout.dll + 0x9fa33 (0x00f8fa33)
gklayout.dll + 0xc583c (0x00fb583c)
xpcom.dll + 0x3cd6d (0x6107cd6d)
gklayout.dll + 0xcac19 (0x00fbac19)
gklayout.dll + 0xca98f (0x00fba98f)
gklayout.dll + 0x1534d4 (0x010434d4)
gklayout.dll + 0x7f36e (0x00f6f36e)
gklayout.dll + 0x7a3a2 (0x00f6a3a2)
gklayout.dll + 0x7f514 (0x00f6f514)
xpcom.dll + 0x3cd6d (0x6107cd6d)
gklayout.dll + 0xd17fb (0x00fc17fb)
xpc3250.dll + 0xb113 (0x60c3b113)
jsd3250.dll + 0x4cf4 (0x60654cf4)
js3250.dll + 0x1a90b (0x60d4a90b)
js3250.dll + 0x1a189 (0x60d4a189)
js3250.dll + 0x2616 (0x60d32616)
gklayout.dll + 0x14dd73 (0x0103dd73)
xpcom.dll + 0x28f64 (0x61068f64)
appshell.dll + 0x7c49 (0x600e7c49)
Mozilla.exe + 0x1df4 (0x00401df4)
appshell.dll + 0x7c49 (0x600e7c49)
Mozilla.exe + 0x1df4 (0x00401df4)
Mozilla.exe + 0x1969 (0x00401969)
Mozilla.exe + 0x2d2b (0x00402d2b)
Mozilla.exe + 0xdd5e (0x0040dd5e)
kernel32.dll + 0x1eb69 (0x77e5eb69) 

Comment 7

[Boris Zbarsky [:bzbarsky]]

Martijn, the talkback servers are claimed to be working correctly now.... would
you mind sending a talkback to mozilla.org again?  The stack in comment 6 is not
helpful either (no symbols).

Comment 8

[Martijn Wargers (dead)]

Ok, done.
I've got Talkback ID: TB7533K

Comment 9

[Boris Zbarsky [:bzbarsky]]

chofmann, could you get the stack info?

Comment 10

[chris hofmann]

I've gotta get my account stuff sent to jpatel so I can get to talkback query
server...  getting that done now...  jay can you dig this one out of the tb
database?

Comment 11

[chris hofmann]

0x0283857a 0ba0a391
Email Address	-------
Product ID	Mozilla1.7
Build ID	2004031615
Trigger Time	2004-03-29 00:25:26.0
Platform	Win32
Operating System	Windows NT 5.0 build 2195
Module	null
URL visited	http://bugzilla.mozilla.org/show_bug.cgi?id=237975
User Comments	Crasher for Boris: [Bug 237975] Crash with xbl generated link,
opening in new tab
Since Last Crash	null sec
Total Uptime	null sec
Trigger Reason	Access violation
Source File Name	
Trigger Line No.	
Stack Trace 	
0x0283857a
nsAttributeTextNode::DetachListener
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/content/base/src/nsTextNode.cpp,
line 356]
nsAttributeTextNode::~nsAttributeTextNode
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/content/base/src/nsTextNode.cpp,
line 117]
nsAttributeTextNode::`scalar deleting destructor'
nsGenericDOMDataNode::Release
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericDOMDataNode.cpp,
line 82]
nsCOMPtr_base::~nsCOMPtr_base
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/xpcom/glue/nsCOMPtr.cpp,
line 82]
nsEventStateManager::~nsEventStateManager
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventStateManager.cpp,
line 310]
nsEventStateManager::`scalar deleting destructor'
nsJSEventListener::Release
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/dom/src/events/nsJSEventListener.cpp,
line 79]
nsPresContext::~nsPresContext
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsPresContext.cpp,
line 209]
GalleyContext::`scalar deleting destructor'
nsPresContext::Release
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsPresContext.cpp,
line 233]
nsCOMPtr_base::~nsCOMPtr_base
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/xpcom/glue/nsCOMPtr.cpp,
line 82]
nsDOMEvent::`scalar deleting destructor'
XPCJSRuntime::GCCallback
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp,
line 549]
0x088b0674

Comment 12

[Boris Zbarsky [:bzbarsky]]

Chris, what are the local variables on that last stack frame?  In particular,
what are the values of |mListener|, |mParent|, and |target| there?

Comment 13

[:Mook]

Since it looks like nobody has touched the bug, I guess I can at least get a
stack trace even if I can't fix anything:

Attached is the stack trace plus the memory bz was looking for.  I can't figure
out what mParent is, so I included the thing being returned by GetParent() for
good measure.  That doesn't look like it makes any sense though.

Firefox, debug build.  WinXP SP1; VC7.1/VS.NET 2003
checkout start: Mon Apr 12 01:51:16 PDT 2004
checkout finish: Mon Apr 12 02:07:04 PDT 2004

As I said in the MZ thread, this seems to have regressed near 2004-01-27 - and
the code seems to have been added as v.3.41 of nsTextNode.cpp on 2004-01-26.
(mozilla/content/base/src/nsTextNode.cpp)

HTH - If not, mail me and I'll be quiet in the future :)

(nsIContent*)(mParentPtrBits & ~kParentBitMask)	0x02e08bb0 {
	mDocument=0xfeeefeee { ... } 
	mParentPtrBits=0xfeeefeee 
}	nsIContent *

mListener	{
	mRawPtr=0x02e098e8 {
		mRefCnt={mValue=0x00000001 } 
		_mOwningThread={mThread=0x003062a8 } 
		mNameSpaceID=0x00000000 ...
	} 
}	nsRefPtr<nsAttributeTextNode::nsAttrChangeListener>

Before QI on nsTextNode line 350:
target	{
	mRawPtr	0x0012f884	nsIDOMEventTarget * {
		nsISupports	{
			__vfptr	0x011c8abf	*
			[0x0]	0x8308458b	*
			[0x1]	0x0c7401e0	*
			[0x2]	0x51fc4d8b	*
		}	nsISupports
	}
}	nsCOMPtr<nsIDOMEventTarget>


 Index  Function
--------------------------------------------------------------------------------
*1      gklayout.dll!nsIContent::GetParent()  Line 103
 2      gklayout.dll!nsAttributeTextNode::DetachListener()  Line 348 + 0x8
 3      gklayout.dll!nsAttributeTextNode::~nsAttributeTextNode()  Line 117
 4      gklayout.dll!nsAttributeTextNode::`scalar deleting destructor'()  + 0xf
 5      gklayout.dll!nsGenericDOMDataNode::Release()  Line 81 + 0xd0
 6      gklayout.dll!nsTextNode::Release()  Line 151 + 0xc
 7      gklayout.dll!nsCOMPtr<nsIContent>::~nsCOMPtr<nsIContent>()  Line 510
 8      gklayout.dll!nsEventStateManager::~nsEventStateManager()  Line 306 + 0xbd
 9      gklayout.dll!nsEventStateManager::`scalar deleting destructor'()  + 0xf
 10     gklayout.dll!nsEventStateManager::Release()  Line 353 + 0xd3
 11     gklayout.dll!nsPresContext::~nsPresContext()  Line 205 + 0x12
 12     gklayout.dll!GalleyContext::~GalleyContext()  Line 59 + 0x8
 13     gklayout.dll!GalleyContext::`scalar deleting destructor'()  + 0xf
 14     gklayout.dll!nsPresContext::Release()  Line 233 + 0xd6
 15     gklayout.dll!nsCOMPtr<nsIPresContext>::~nsCOMPtr<nsIPresContext>()  Line 510
 16     gklayout.dll!nsDOMEvent::~nsDOMEvent()  Line 251 + 0x4d
 17     gklayout.dll!nsDOMEvent::`scalar deleting destructor'()  + 0xf
 18     gklayout.dll!nsDOMEvent::Release()  Line 254 + 0xd3
 19     xpc3250.dll!XPCJSRuntime::GCCallback(JSContext * cx=0x02baddf0,
JSGCStatus status=JSGC_END)  Line 556 + 0x12
 20     gklayout.dll!DOMGCCallback(JSContext * cx=0x02baddf0, JSGCStatus
status=JSGC_END)  Line 1881 + 0x17
 21     js3250.dll!js_GC(JSContext * cx=0x02baddf0, unsigned int
gcflags=0x00000000)  Line 1419 + 0xc
 22     js3250.dll!js_ForceGC(JSContext * cx=0x02baddf0, unsigned int
gcflags=0x00000000)  Line 1000 + 0xd
 23     js3250.dll!JS_GC(JSContext * cx=0x02baddf0)  Line 1681 + 0xb
 24     gklayout.dll!nsJSContext::Notify(nsITimer * timer=0x02d84630)  Line 1834
+ 0xd
 25     xpcom.dll!nsTimerImpl::Fire()  Line 386
 26     xpcom.dll!nsTimerManager::FireNextIdleTimer()  Line 616
 27     gkwidget.dll!nsAppShell::Run()  Line 142
 28     appshell.dll!nsAppShellService::Run()  Line 524
 29     firefox.exe!main1(int argc=0x00000001, char * * argv=0x00308758,
nsISupports * nativeApp=0x00a2b638, const nsXREAppData & aAppData={...})  Line
1287 + 0x20
 30     firefox.exe!xre_main(int argc=0x00000001, char * * argv=0x00308758,
const nsXREAppData & aAppData={...})  Line 1807 + 0x29
 31     firefox.exe!main(int argc=0x00000001, char * * argv=0x00308758)  Line 51
+ 0x11
 32     firefox.exe!mainCRTStartup()  Line 398 + 0x11
 33     kernel32.dll!77e814c7() 
 

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

That helps a good deal!  It looks like the mParent pointer is just bogus at
that point.   Based on where we're destroying the node from, it looks like the
node has a ref to it from the ESM, which makes it hang about long after its
parent and frame are gone (because the ESM is hanging about because JS holds a
ref to an event and hasn't GCed).  So the point is, the parent is gone, and we
never got told about it (nothing calls SetParent() on the generated content
nodes that I can see).	Then when we get destroyed we try to mess with the
parent, and that fails.

What this patch does is to set the parent to null when our frame goes away,
which is when we want it happening.  Mook, could you check whether this helps? 
I can't reproduce the crash at all in SeaMonkey (possibly something in the
chrome does something differently to events), so I can't test whether this
helps....

Comment 15

[:Mook]

That patch does indeed appear to fix it - at least I can't seem to trigger it anymore, using a build that was exactly the same except for the patch.  Thanks!

Actually, I did manage to get the crash in Seamonkey as well - you have to make sure new tabs open in background, which IIRC is not the default in Seamonkey.  There were no trunk Win32 Fire* builds at the time to test with :)

Comment 16

[Boris Zbarsky [:bzbarsky]]

> you have to make sure new tabs open in background

Exactly what I was doing.

The problem is that the crash is timing-dependent, gc-dependent, and
OS-allocation-pattern dependent (and I was testing on Linux).

David, what do you think of the patch?  See the XXX comments in particular... I
can't quite decide where this functionality most cleanly belongs.

Comment 17

[Martijn Wargers (dead)]

I've managed to get a debug build running on my home machine.
So I can confirm also that the testcase doesn't seem to crash anymore with the
patch.
It always seemed to me that Firefox was far more susceptible to the crash.

Comment 18

[Martijn Wargers (dead)]

Attachment: Same test-case but with ::first-letter

From the comments in the patch, I've made a similar testcase, but with a
first-letter css-rule in it.
This indeed still crashes with the patch from Boris.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: OK, if that's what you're seeing then this is definitely the right approach

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, I meant this.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146103 [details] [diff] [review]
Er, I meant this.

David, what do you think?  See comment 14 for a description of the problem I'm
trying to solve....

Comment 22

[Martijn Wargers (dead)]

I've compiled the last patch from Boris, and I don't get a crash, with at least
2 minutes of vigorously middle-clicking on the link and closing the tabs.

Comment 23

[Adam Hauner]

Attachment: stack TB7533K

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 146103 [details] [diff] [review]
Er, I meant this.

r+sr=dbaron, but maybe call |aParent| something like |aRealContent|?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Checked in on the 1.8a trunk with the change to aRealContent.

Leaving open for now, because I think this should go into 1.7....

Comment 26

[chris hofmann]

Comment on attachment 146103 [details] [diff] [review]
Er, I meant this.

a=chofmann  gor 1.7

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in on the 1.7 branch.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Er, and marking fixed, even.

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with trunk build 2004-07-25-09 on Windows XP.