Last Comment Bug 394014 - [FIX]Crash [@ NS_ProcessNextEvent_P] with DOMSubtreeModified removing windows, binding and other stuff
: [FIX]Crash [@ NS_ProcessNextEvent_P] with DOMSubtreeModified removing windows...
Status: VERIFIED FIXED
[sg:critical?]
: crash, regression, testcase, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on: 396286 396587 398289
Blocks: 394676 396099
  Show dependency treegraph
 
Reported: 2007-08-28 09:21 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
10 users (show)
dveditz: blocking1.8.1.8+
asac: blocking1.8.0.next+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
iframe needed for testcase (868 bytes, text/html)
2007-08-28 09:21 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase (344 bytes, text/html)
2007-08-28 09:22 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
shows where the problem is (3.86 KB, patch)
2007-08-28 09:56 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
testcase2, zipped (1.30 KB, application/zip)
2007-08-28 11:31 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Let's do it (7.51 KB, patch)
2007-09-02 21:05 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
bugs: review+
jonas: superreview+
Details | Diff | Review
Updated to comments (6.94 KB, patch)
2007-09-13 11:44 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: approval1.9+
Details | Diff | Review
Merged to tip (7.05 KB, patch)
2007-09-14 12:14 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-28 09:21:02 PDT
Created attachment 278594 [details]
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 [:mwargers] (not working for Mozilla) 2007-08-28 09:22:07 PDT
Created attachment 278595 [details]
testcase
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-08-28 09:27:14 PDT
Hmm, is this a regression, or just something which got exposed because of bug 288392.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-08-28 09:56:17 PDT
Created attachment 278599 [details] [diff] [review]
shows where the problem is
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-08-28 09:58:54 PDT
Maybe component:XBL would be even better.
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-28 11:31:38 PDT
Created attachment 278608 [details]
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 [:bz] (Out June 25-July 6) 2007-08-28 22:52:06 PDT
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] (high review load, please consider other reviewers) 2007-08-30 03:19:58 PDT
(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 [:bz] (Out June 25-July 6) 2007-08-30 06:11:52 PDT
> Strong ref to what?

The frame constructor.

Let me look at this stuff a sec.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-09-02 21:05:14 PDT
Created attachment 279403 [details] [diff] [review]
Let's do it
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-09-03 00:33:09 PDT
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] (high review load, please consider other reviewers) 2007-09-03 00:36:38 PDT
(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 [:bz] (Out June 25-July 6) 2007-09-03 00:43:47 PDT
> There is already kungfuDeathGrip inside if (isSafeToFlush && viewManager) {

Ah, indeed.  I'll take that hunk out.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2007-09-06 17:08:58 PDT
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 [:bz] (Out June 25-July 6) 2007-09-13 11:44:15 PDT
Created attachment 280774 [details] [diff] [review]
Updated to comments

Fairly safe patch that makes sure we hold strong refs as needed.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-09-13 11:47:03 PDT
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 [:bz] (Out June 25-July 6) 2007-09-14 12:14:25 PDT
Created attachment 280918 [details] [diff] [review]
Merged to tip
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-09-14 12:17:45 PDT
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 [:bz] (Out June 25-July 6) 2007-09-14 14:21:02 PDT
Branch patch attached to bug 267833 as part of the big branch patch there.
Comment 19 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-09-15 10:16:33 PDT
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 [:bz] (Out June 25-July 6) 2007-10-02 12:24:53 PDT
Fixed by checkin for bug 267833.
Comment 21 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-04 15:35:10 PDT
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:] 2009-04-24 10:36:22 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/bec8b04b0d01

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