394014 - [FIX]Crash [@ NS_ProcessNextEvent_P] with DOMSubtreeModified removing windows, binding and other stuff

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: iframe needed for testcase

See upcoming testcase, which crashes current trunk builds directly or after a few reloads (testcase reloads automatically).

This seems to have regressed between 2007-03-22 and 2007-03-24, that is the time when bug 288392 got fixed (making DOMSubtreeModified  work). So I guess this is a regression from bug 288392 somehow.

http://crash-stats.mozilla.com/report/index/99b10b78-557b-11dc-9ac6-001a4bd46e84
0  	@0x0  	
1 	NS_ProcessNextEvent_P(nsIThread*, int) 	nsThreadUtils.cpp:227
2 	nsBaseAppShell::Run() 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154
3 	nsAppStartup::Run() 	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:170
4 	XRE_main 	mozilla/toolkit/xre/nsAppRunner.cpp:3069
5 	main 	mozilla/browser/app/nsBrowserApp.cpp:153
6 	WinMain 	mozilla/browser/app/nsBrowserApp.cpp:166
7 	__tmainCRTStartup 	crtexe.c:589
8 	BaseProcessStart 	

The binding in the "iframe needed for testcase" file consists of this:
<bindings xmlns="http://www.mozilla.org/xbl">
<binding id="a">
<implementation>
<constructor>
  this.style.outline='';
</constructor>
</implementation>
</binding>
</bindings>

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

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

Hmm, is this a regression, or just something which got exposed because of bug 288392.

Comment 3

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

Attachment: shows where the problem is

Comment 4

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

Maybe component:XBL would be even better.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase2, zipped

Ok, apparently this has nothing to do with domsubtreemodified.
To reproduce with this testcase, unzip and then open "parentframe2.htm".
This also crashes on branch, because of bug 373911, I think.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So we could add strong refs (at the callsites, though), or we could move the binding attachment processing into the callers to make it more explicit...  I'd almost rather do the latter.

Comment 7

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

(In reply to comment #6)
> So we could add strong refs (at the callsites, though)
Strong ref to what?

> binding attachment processing into the callers to make it more explicit...  I'd
> almost rather do the latter.
> 

Yes, that sounds better. Maybe some helper function which takes care or viewmanager handling and binding attachment processing...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Strong ref to what?

The frame constructor.

Let me look at this stuff a sec.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Let's do it

Comment 10

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

Comment on attachment 279403 [details] [diff] [review]
Let's do it


>+  
>+  // Guard against callers flushing without holding a strong reference.
>+  nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);

There is already kungfuDeathGrip inside if (isSafeToFlush && viewManager) {

>Index: layout/reftests/bugs/394676-1-ref.xhtml
...
>Index: layout/reftests/bugs/394676-1.xhtml
Testcases aren't for this bug.

Comment 11

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

(In reply to comment #10)

> >Index: layout/reftests/bugs/394676-1-ref.xhtml
> ...
> >Index: layout/reftests/bugs/394676-1.xhtml
> Testcases aren't for this bug.
> 
Ok, I saw the comments in Bug 394676

Comment 12

[Boris Zbarsky [:bzbarsky]]

> There is already kungfuDeathGrip inside if (isSafeToFlush && viewManager) {

Ah, indeed.  I'll take that hunk out.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Boris, is this ready to land? I can do the dirty work of hunting approvals and landing this. Would be great to get this into the alpha.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Fairly safe patch that makes sure we hold strong refs as needed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

We probably need to fix this on branches too....  I'm not sure when I'll have time to port this to branches, though.  :(  Help there would really be appreciated.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in.  Note that we still need to check in tests for THIS bug.  So leaving the "in-testsuite?" in place.

I'll work on a branch patch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Branch patch attached to bug 267833 as part of the big branch patch there.

Comment 19

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091504 Minefield/3.0a8pre

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed by checkin for bug 267833.

Comment 21

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071004 BonEcho/2.0.0.8pre

I can see the crash with the zipped testcase in Firefox2.0.0.7.

Comment 22

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/bec8b04b0d01