Last Comment Bug 378682 - Crash [@ nsPresContext::GetContainerInternal] when removing window on focus and reloading
: Crash [@ nsPresContext::GetContainerInternal] when removing window on focus a...
Status: VERIFIED FIXED
[sg:critical?] using freed pres shell
: crash, fixed1.8.1.5, testcase, verified1.8.0.13, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-24 16:56 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2013-02-23 17:11 PST (History)
11 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (775 bytes, text/html)
2007-04-24 16:56 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
stack + tracing (9.23 KB, text/html)
2007-04-25 19:15 PDT, Mats Palmgren (:mats)
no flags Details
wip1 (2.63 KB, patch)
2007-04-25 19:18 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 1 (3.59 KB, patch)
2007-04-26 06:54 PDT, Mats Palmgren (:mats)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch rev. 1 -- branch version (2.63 KB, patch)
2007-07-09 19:28 PDT, Mats Palmgren (:mats)
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-24 16:56:51 PDT
Created attachment 262701 [details]
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>
Comment 1 Olli Pettay [:smaug] 2007-04-25 10:40:20 PDT
I can't get trunk to crash, nor branch.
Comment 2 Mats Palmgren (:mats) 2007-04-25 19:15:41 PDT
Created attachment 262835 [details]
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.
Comment 3 Mats Palmgren (:mats) 2007-04-25 19:18:20 PDT
Created attachment 262837 [details] [diff] [review]
wip1

Adding a strong ref on the stack fixes it for me.
Need to review all other pres shell calls from this file...
Comment 4 Boris Zbarsky [:bz] 2007-04-25 19:34:16 PDT
nsCOMPtr, please!
Comment 5 Mats Palmgren (:mats) 2007-04-26 06:54:07 PDT
Created attachment 262886 [details] [diff] [review]
Patch rev. 1

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.
Comment 6 Boris Zbarsky [:bz] 2007-04-26 12:33:40 PDT
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.
Comment 7 Mats Palmgren (:mats) 2007-04-27 06:45:36 PDT
Checked in to trunk at 2007-04-27 04:06	PDT.

-> FIXED
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-27 09:51:53 PDT
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?
Comment 9 Mats Palmgren (:mats) 2007-04-27 13:33:58 PDT
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.
Comment 10 Boris Zbarsky [:bz] 2007-04-29 12:13:59 PDT
> the browser stays in the "loading" state.

Worksforme in both Seamonkey and Firefox.  As does bug 143398...
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-29 12:45:15 PDT
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).
Comment 12 Daniel Veditz [:dveditz] 2007-07-09 15:35:58 PDT
branch is still crashing on a deleted object. Can this patch land there or do we need a branch-specific patch?
Comment 13 Mats Palmgren (:mats) 2007-07-09 19:28:30 PDT
Created attachment 271603 [details] [diff] [review]
Patch rev. 1 -- branch version

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).
Comment 14 Daniel Veditz [:dveditz] 2007-07-10 10:44:47 PDT
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
Comment 15 Mats Palmgren (:mats) 2007-07-11 13:47:36 PDT
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 
Comment 16 juan becerra [:juanb] 2007-08-20 14:17:16 PDT
I was not able to reproduce the crash on Tbird 15012(pre fix) nor 15013. 
Comment 17 Al Billings [:abillings] 2007-08-22 17:27:21 PDT
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.
Comment 18 Carsten Book [:Tomcat] - PTO-back Sept 4th 2007-08-23 07:37:57 PDT
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
Comment 19 Carsten Book [:Tomcat] - PTO-back Sept 4th 2007-08-23 07:58:52 PDT
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 
Comment 20 Daniel Veditz [:dveditz] 2008-02-08 14:23:11 PST
re-adding fixed1.8.1.5 to record when the actual fix happened, even though verification was later due to firedrills.
Comment 22 Gregory Szorc [:gps] 2013-02-23 17:11:33 PST
https://hg.mozilla.org/mozilla-central/rev/37189f38c020

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