Closed Bug 378682 Opened 17 years ago Closed 17 years ago

Crash [@ nsPresContext::GetContainerInternal] when removing window on focus and reloading

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)

Details

(5 keywords, Whiteboard: [sg:critical?] using freed pres shell)

Crash Data

Attachments

(4 files, 1 obsolete file)

testcase
775 bytes, text/html
Details
stack + tracing
9.23 KB, text/html
Details
Patch rev. 1
3.59 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Patch rev. 1 -- branch version
2.63 KB, patch
dveditz
: approval1.8.1.5+
dveditz
: approval1.8.0.13+
Details | Diff | Splinter Review
Attached file testcase — 
See testcase, which crashes Mozilla within 200ms after load.
It also crashes branch builds, so I'm marking it security sensitive for now (please  open up, if not necessary).

Talkback ID: TB31509967G
nsPresContext::GetContainerInternal  [mozilla/layout/base/nsprescontext.cpp, line 1117]
PresShell::UnsuppressAndInvalidate  [mozilla/layout/base/nspresshell.cpp, line 4412]
PresShell::UnsuppressPainting  [mozilla/layout/base/nspresshell.cpp, line 4457]

The iframe consists of this code:
<html><head></head>
<body>
<iframe></iframe>
<script>
window.frames[0].focus();
setTimeout(doe, 200);
function doe() {
window.frames[0].location.reload();
}
function doe2() {
window.addEventListener('focus', function(e) {window.frameElement.parentNode.removeChild(window.frameElement) }, true);
}
setTimeout(doe2, 50);
</script>
</body>
</html>
I can't get trunk to crash, nor branch.
Assignee: nobody → mats.palmgren
Whiteboard: [sg:critical?] using freed pres shell
Attached file stack + tracing — 
DocumentViewerImpl::LoadComplete calls mPresShell->UnsuppressPainting()
which ends up destroying the pres shell, doc viewer, doc shell...
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsDocumentViewer.cpp&rev=1.518&root=/cvsroot&mark=1085,1013#1013

The pres shell code expects the caller to hold a strong ref, which
'mPresShell' is, but DocumentViewerImpl::Destroy nulls it...
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsDocumentViewer.cpp&rev=1.518&root=/cvsroot&mark=1458,1606#1564

This attachment shows two stack traces, the first is the stack
when DocumentViewerImpl::Destroy() is called while we are in
DocumentViewerImpl::LoadComplete().  I was expecting to see
LoadComplete() on the stack but the Visual debugger seems confused
by the JS related stack frames? The second stack is the crash. 
I have some trace printfs in there as well.
Attached patch wip1 (obsolete) — — Splinter Review 
Adding a strong ref on the stack fixes it for me.
Need to review all other pres shell calls from this file...
nsCOMPtr, please!
Attached patch Patch rev. 1 — — Splinter Review 
Changed to using nsCOMPtr.  I found one more call that I'm vaguely
suspicious about, mPresShell->HidePopups(), so I'm protecting that one
as well just in case.
Attachment #262837 - Attachment is obsolete: true
Attachment #262886 - Flags: superreview?(bzbarsky)
Attachment #262886 - Flags: review?(bzbarsky)
Comment on attachment 262886 [details] [diff] [review]
Patch rev. 1

Looks ok.  We probably want this on branch too, esp. once the HidePopups thing lands there.
Attachment #262886 - Flags: superreview?(bzbarsky)
Attachment #262886 - Flags: superreview+
Attachment #262886 - Flags: review?(bzbarsky)
Attachment #262886 - Flags: review+
Checked in to trunk at 2007-04-27 04:06	PDT.

-> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Resolution: --- → FIXED
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070427 Minefield/3.0a4pre

I'm seeing a small issue, though, after the iframe has disappeared, the browser stays in the "loading" state. I suspect that is an already filed bug, but I haven't found one. Should I file a new bug on that?
Status: RESOLVED → VERIFIED
Yeah I saw that too, the testcase in this bug is changing 'location' of
a nested iframe, which I think is handled by bug 143398.
> the browser stays in the "loading" state.

Worksforme in both Seamonkey and Firefox.  As does bug 143398...
Bug 143398 is indeed also worksforme, however the testcase in this bug is not worksforme, so I've filed it as bug 379206 (made it security sensitive, which sucks).
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
branch is still crashing on a deleted object. Can this patch land there or do we need a branch-specific patch?
Same thing, but one place less to fix because of bug 374570:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsDocumentViewer.cpp&rev=1.442.4.19&root=/cvsroot&mark=1215#1176
I have verified that this patch fixed the crash in a 1.8 Windows build,
although the throbber continues to spin as Martijn noted (bug 379206).
Attachment #271603 - Flags: approval1.8.1.5?
Attachment #271603 - Flags: approval1.8.0.13?
Comment on attachment 271603 [details] [diff] [review]
Patch rev. 1 -- branch version

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Attachment #271603 - Flags: approval1.8.1.5?
Attachment #271603 - Flags: approval1.8.1.5+
Attachment #271603 - Flags: approval1.8.0.13?
Attachment #271603 - Flags: approval1.8.0.13+
MOZILLA_1_8_BRANCH
mozilla/layout/base/nsDocumentViewer.cpp 	1.442.4.20 

MOZILLA_1_8_0_BRANCH
mozilla/layout/base/nsDocumentViewer.cpp 	1.442.4.7.2.6
Keywords: fixed1.8.0.13, fixed1.8.1.5
I was not able to reproduce the crash on Tbird 15012(pre fix) nor 15013. 
This does not reproduce in Thunderbird 1.5.0.13 (2007080918) using
Thunderbrowse. It also doesn't reproduce in Thunderbird 1.5.0.12 though.
verified fixed 1.8.1.7 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070822 BonEcho/2.0.0.7pre ID:2007082203

no crash on testcase - adding verified keyword
Keywords: fixed1.8.1.5 → verified1.8.1.7
also verified 1.8.0.13 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.13pre) Gecko/20070822 Firefox/1.5.0.13pre - no crash on testcase

Adding verified keyword
Keywords: fixed1.8.0.13 → verified1.8.0.13
Group: security
re-adding fixed1.8.1.5 to record when the actual fix happened, even though verification was later due to firedrills.
Keywords: fixed1.8.1.5
Crash Signature: [@ nsPresContext::GetContainerInternal]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: