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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

(4 keywords)

Trunk
crash, regression, testcase, verified1.8.1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.8 +
blocking1.8.0.next +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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>
(Reporter)

Comment 1

10 years ago
Created attachment 278595 [details]
testcase

Updated

10 years ago
Assignee: nobody → Olli.Pettay

Comment 2

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

Comment 3

10 years ago
Created attachment 278599 [details] [diff] [review]
shows where the problem is

Updated

10 years ago
Group: security

Updated

10 years ago
Assignee: Olli.Pettay → nobody
Component: DOM: Events → Layout
QA Contact: events → layout

Comment 4

10 years ago
Maybe component:XBL would be even better.
(Reporter)

Comment 5

10 years ago
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.
Flags: blocking1.9?
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

10 years ago
(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...
> Strong ref to what?

The frame constructor.

Let me look at this stuff a sec.
(Assignee)

Updated

10 years ago
Blocks: 394676
Created attachment 279403 [details] [diff] [review]
Let's do it
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #279403 - Flags: superreview?(jonas)
Attachment #279403 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #279403 - Flags: review? → review?(Olli.Pettay)
(Assignee)

Updated

10 years ago
Flags: in-testsuite?
OS: Windows XP → All
Hardware: PC → All
Summary: Crash [@ NS_ProcessNextEvent_P] with DOMSubtreeModified removing windows, binding and other stuff → [FIX]Crash [@ NS_ProcessNextEvent_P] with DOMSubtreeModified removing windows, binding and other stuff

Comment 10

10 years ago
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.
Attachment #279403 - Flags: review?(Olli.Pettay) → review+

Comment 11

10 years ago
(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
> There is already kungfuDeathGrip inside if (isSafeToFlush && viewManager) {

Ah, indeed.  I'll take that hunk out.
Attachment #279403 - Flags: superreview?(jonas) → superreview+
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.
Created attachment 280774 [details] [diff] [review]
Updated to comments

Fairly safe patch that makes sure we hold strong refs as needed.
Attachment #280774 - Flags: approval1.9?
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.
Flags: blocking1.8.1.7?
Flags: blocking1.8.0.14?
(Assignee)

Updated

10 years ago
Blocks: 396099
Attachment #280774 - Flags: approval1.9? → approval1.9+
Created attachment 280918 [details] [diff] [review]
Merged to tip
Attachment #279403 - Attachment is obsolete: true
Attachment #280774 - Attachment is obsolete: true
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Branch patch attached to bug 267833 as part of the big branch patch there.
(Reporter)

Comment 19

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091504 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
(Reporter)

Updated

10 years ago
Depends on: 396286
(Reporter)

Updated

10 years ago
Depends on: 396587
Flags: blocking1.8.1.8? → blocking1.8.1.8+
Fixed by checkin for bug 267833.
Keywords: fixed1.8.1.8
(Assignee)

Updated

10 years ago
Depends on: 398289
(Reporter)

Comment 21

10 years ago
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.
Keywords: fixed1.8.1.8 → verified1.8.1.8
Whiteboard: [sg:critical?]
Group: security
Flags: blocking1.8.0.14? → blocking1.8.0.15?

Updated

10 years ago
Flags: blocking1.8.0.15? → blocking1.8.0.15+

Comment 22

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/bec8b04b0d01
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ NS_ProcessNextEvent_P]
You need to log in before you can comment on or make changes to this bug.