Crash [@ DocumentViewerImpl::LoadComplete] with focusing and removing iframe on reload

RESOLVED FIXED

Status

()

defect
P2
critical
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: martijn.martijn, Assigned: roc)

Tracking

({crash, regression, testcase})

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [reviewed patch in hand], crash signature)

Attachments

(2 attachments)

Posted file testcase
See testcase, which crashes current trunk build after 500ms.

The data uri consists of this:
<script>window.addEventListener('focus', function(e) {window.frameElement.parentNode.removeChild(window.frameElement);}, true)</script>

This regressed between 2008-02-26 and 2008-02-27:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-26+04&maxdate=2008-02-27+06&cvsroot=%2Fcvsroot
So I think a regression from bug 317189.

http://crash-stats.mozilla.com/report/index/e13d8130-ebe2-11dc-994b-001a4bd46e84
0  	DocumentViewerImpl::LoadComplete(unsigned int)  	 mozilla/layout/base/nsDocumentViewer.cpp:1014
1 	nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) 	mozilla/docshell/base/nsDocShell.cpp:5030
2 	nsWebShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) 	mozilla/docshell/base/nsWebShell.cpp:1013
3 	nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) 	nsCOMPtr.cpp:96
4 	nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) 	mozilla/docshell/base/nsDocShell.cpp:4930
Flags: blocking1.9?
+'ing this as it's a recent regression from a fix in Feb.  roc, can you take a look?
Assignee: nobody → roc
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Doesn't seem to crash on Mac. Will try Windows but first I have to install the Vista SDK :-(
If you add ac_add_options --disable-parental-controls, you don't need to install the Vista SDK.
Sorry, you also need to have the patches from bug 425979 applied.
Here's what happens:

DocumentViewerImpl::LoadComplete() on the <iframe> calls  mPresShell->UnsuppressPainting(), which calls PresShell::UnsupressAndInvalidate(), which calls CheckForFocus, which calls nsGlobalWindow::Focus, which leads to the dispatch of a focus event, which invokes the handler which removes the <iframe> which tears down the <iframe>'s frameloader which destroys its docshell which calls DocumentViewerImpl::Destroy on the same object we're currently in LoadComplete for.

This is very much like bug 378682. That bug was fixed by making DocumentViewerImpl::LoadComplete() robust against docshell/docviewer/etc destruction, so I guess we need to extend that fix to cover the code introduced in bug 317189.
Posted patch fixSplinter Review
Same approach as bug 378682. Very straightforward, zero risk.
Attachment #313922 - Flags: superreview?(mats.palmgren)
Attachment #313922 - Flags: review?(mats.palmgren)
Whiteboard: [needs review]
Per discussion with dbaron and roc, wouldn't hold the release for this if it were the last bug on the list (we're at that point now).  Moving to wanted1.9.0.x+.

Will take a patch if reviews are passed.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
Comment on attachment 313922 [details] [diff] [review]
fix

r+sr=dbaron
Attachment #313922 - Flags: superreview?(mats.palmgren)
Attachment #313922 - Flags: superreview+
Attachment #313922 - Flags: review?(mats.palmgren)
Attachment #313922 - Flags: review+
Comment on attachment 313922 [details] [diff] [review]
fix

fixes crash regression, zero risk.
Attachment #313922 - Flags: approval1.9?
Whiteboard: [needs review] → [reviewed patch in hand]
Comment on attachment 313922 [details] [diff] [review]
fix

a1.9=beltzner
Attachment #313922 - Flags: approval1.9? → approval1.9+
Checked in. I need to check in a testcase for this.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Flags: wanted1.9.0.x+
Crash Signature: [@ DocumentViewerImpl::LoadComplete]
Depends on: 844525
You need to log in before you can comment on or make changes to this bug.