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

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Damon Sicore (:damons)]

+'ing this as it's a recent regression from a fix in Feb.  roc, can you take a look?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

sure

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Doesn't seem to crash on Mac. Will try Windows but first I have to install the Vista SDK :-(

Comment 4

[Martijn Wargers (dead)]

If you add ac_add_options --disable-parental-controls, you don't need to install the Vista SDK.

Comment 5

[Martijn Wargers (dead)]

Sorry, you also need to have the patches from bug 425979 applied.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Same approach as bug 378682. Very straightforward, zero risk.

Comment 8

[Damon Sicore (:damons)]

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.

Comment 9

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

Comment on attachment 313922 [details] [diff] [review]
fix

r+sr=dbaron

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 313922 [details] [diff] [review]
fix

fixes crash regression, zero risk.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 313922 [details] [diff] [review]
fix

a1.9=beltzner

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. I need to check in a testcase for this.

Comment 13

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1babe9fe6027

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/1babe9fe6027