Last Comment Bug 466057 - ASSERTION: Someone forgot to block scripts: 'aIsSafeToFlush == nsContentUtils::IsSafeToRunScript()'
: ASSERTION: Someone forgot to block scripts: 'aIsSafeToFlush == nsContentUtils...
Status: RESOLVED FIXED
: [sg:critical?] post 1.8-branch
: assertion, fixed1.9.0.7, fixed1.9.1
Product: Core
Classification: Components
Component: Layout
: Trunk
: x86 Windows XP
: P1 normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: layout
:
: 495035 471166 482578 491063 507457
: 444224 430800 436965 468970
  Show dependency treegraph
 
Reported: 2008-11-20 15:10 PST by timeless
Modified: 2011-03-25 13:51 PDT (History)
11 users (show)
roc: blocking1.9.1+
dveditz: blocking1.9.0.7+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
See Also:
Crash Signature:


Attachments
v1 (17.24 KB, patch)
2008-11-27 08:42 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
with assertion (17.63 KB, patch)
2008-12-10 13:49 PST, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
1.9.0 patch (17.95 KB, patch)
2009-02-02 03:58 PST, Olli Pettay [:smaug]
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review

Summon comment box

Description timeless 2008-11-20 15:10:16 PST
i know there are other bugs about this assertion, but i'm not using domi, and i haven't been using venkman

00 ntdll!DbgBreakPoint
01 xpcom_core!Break(char * aMsg = 0x0012e7dc "###!!! ASSERTION: Someone forgot to block scripts: 'aIsSafeToFlush == nsContentUtils::IsSafeToRunScript()', file c:/home/mozilla.org/mozilla-central/layout/base/nsPresShell.cpp, line 4496")+0x20e [c:\home\mozilla.org\mozilla-central\xpcom\base\nsdebugimpl.cpp @ 481]
02 xpcom_core!NS_DebugBreak_P(unsigned int aSeverity = 1, char * aStr = 0x02130fa0 "Someone forgot to block scripts", char * aExpr = 0x02130f68 "aIsSafeToFlush == nsContentUtils::IsSafeToRunScript()", char * aFile = 0x02130f28 "c:/home/mozilla.org/mozilla-central/layout/base/nsPresShell.cpp", int aLine = 4496)+0x2a4 [c:\home\mozilla.org\mozilla-central\xpcom\base\nsdebugimpl.cpp @ 359]
03 gklayout!PresShell::IsSafeToFlush(int * aIsSafeToFlush = 0x0012ec20)+0xa4 [c:\home\mozilla.org\mozilla-central\layout\base\nspresshell.cpp @ 4496]
04 gklayout!PresShell::DoFlushPendingNotifications(mozFlushType aType = Flush_Layout (4), int aInterruptibleReflow = 1)+0x44 [c:\home\mozilla.org\mozilla-central\layout\base\nspresshell.cpp @ 4517]
05 gklayout!PresShell::WillPaint(void)+0x39 [c:\home\mozilla.org\mozilla-central\layout\base\nspresshell.cpp @ 6021]
06 gklayout!nsViewManager::FlushPendingInvalidates(void)+0x119 [c:\home\mozilla.org\mozilla-central\view\src\nsviewmanager.cpp @ 2239]
07 gklayout!nsViewManager::EnableRefresh(unsigned int aUpdateFlags = 2)+0x53 [c:\home\mozilla.org\mozilla-central\view\src\nsviewmanager.cpp @ 1944]
08 gklayout!nsViewManager::EnableRefresh(unsigned int aUpdateFlags = 2)+0x2d [c:\home\mozilla.org\mozilla-central\view\src\nsviewmanager.cpp @ 1929]
09 gklayout!DocumentViewerImpl::InitPresentationStuff(int aDoInitialReflow = 1, int aReenableRefresh = 1)+0x489 [c:\home\mozilla.org\mozilla-central\layout\base\nsdocumentviewer.cpp @ 752]
0a gklayout!DocumentViewerImpl::Show(void)+0x6f4 [c:\home\mozilla.org\mozilla-central\layout\base\nsdocumentviewer.cpp @ 1915]
0b docshell!nsDocShell::SetVisibility(int aVisibility = 1)+0x41 [c:\home\mozilla.org\mozilla-central\docshell\base\nsdocshell.cpp @ 4015]
0c gklayout!nsSubDocumentFrame::ShowDocShell(void)+0x46b [c:\home\mozilla.org\mozilla-central\layout\generic\nsframeframe.cpp @ 979]
0d gklayout!nsSubDocumentFrame::ShowViewer(void)+0x3f [c:\home\mozilla.org\mozilla-central\layout\generic\nsframeframe.cpp @ 327]
0e gklayout!nsSubDocumentFrame::Init(class nsIContent * aContent = 0x092866e0, class nsIFrame * aParent = 0x09637048, class nsIFrame * aPrevInFlow = 0x00000000)+0x1ba [c:\home\mozilla.org\mozilla-central\layout\generic\nsframeframe.cpp @ 313]
0f gklayout!nsCSSFrameConstructor::InitAndRestoreFrame(class nsFrameConstructorState * aState = 0x0012f2d8, class nsIContent * aContent = 0x092866e0, class nsIFrame * aParentFrame = 0x09637048, class nsIFrame * aPrevInFlow = 0x00000000, class nsIFrame * aNewFrame = 0x090a7188, int aAllowCounters = 1)+0x91 [c:\home\mozilla.org\mozilla-central\layout\base\nscssframeconstructor.cpp @ 6764]
10 gklayout!nsCSSFrameConstructor::ConstructHTMLFrame(class nsFrameConstructorState * aState = 0x0012f2d8, class nsIContent * aContent = 0x092866e0, class nsIFrame * aParentFrame = 0x090d5378, class nsIAtom * aTag = 0x03dd71e8, int aNameSpaceID = 0, class nsStyleContext * aStyleContext = 0x090d1f90, struct nsFrameItems * aFrameItems = 0x0012f3e0, int aHasPseudoParent = 0)+0x9ca [c:\home\mozilla.org\mozilla-central\layout\base\nscssframeconstructor.cpp @ 5567]
11 gklayout!nsCSSFrameConstructor::ConstructFrameInternal(class nsFrameConstructorState * aState = 0x0012f2d8, class nsIContent * aContent = 0x092866e0, class nsIFrame * aParentFrame = 0x090d5378, class nsIAtom * aTag = 0x03dd71e8, int aNameSpaceID = 0, class nsStyleContext * aStyleContext = 0x090d1f90, struct nsFrameItems * aFrameItems = 0x0012f3e0, int aXBLBaseTag = 0)+0x3f7 [c:\home\mozilla.org\mozilla-central\layout\base\nscssframeconstructor.cpp @ 7530]
12 gklayout!nsCSSFrameConstructor::ConstructFrame(class nsFrameConstructorState * aState = 0x0012f2d8, class nsIContent * aContent = 0x092866e0, class nsIFrame * aParentFrame = 0x090d5378, struct nsFrameItems * aFrameItems = 0x0012f3e0)+0x116 [c:\home\mozilla.org\mozilla-central\layout\base\nscssframeconstructor.cpp @ 7402]
13 gklayout!nsCSSFrameConstructor::ContentAppended(class nsIContent * aContainer = 0x09563198, int aNewIndexInContainer = 10)+0x77f [c:\home\mozilla.org\mozilla-central\layout\base\nscssframeconstructor.cpp @ 8580]
14 gklayout!PresShell::ContentAppended(class nsIDocument * aDocument = 0x07adee68, class nsIContent * aContainer = 0x09563198, int aNewIndexInContainer = 10)+0xd5 [c:\home\mozilla.org\mozilla-central\layout\base\nspresshell.cpp @ 4698]
15 gklayout!nsNodeUtils::ContentAppended(class nsIContent * aContainer = 0x09563198, int aNewIndexInContainer = 10)+0xe8 [c:\home\mozilla.org\mozilla-central\content\base\src\nsnodeutils.cpp @ 120]
16 gklayout!nsContentSink::NotifyAppend(class nsIContent * aContainer = 0x09563198, unsigned int aStartIndex = 0xa)+0x73 [c:\home\mozilla.org\mozilla-central\content\base\src\nscontentsink.cpp @ 1332]
17 gklayout!SinkContext::FlushTags(void)+0x223 [c:\home\mozilla.org\mozilla-central\content\html\document\src\nshtmlcontentsink.cpp @ 1383]
18 gklayout!SinkContext::DidAddContent(class nsIContent * aContent = 0x0951a030)+0x259 [c:\home\mozilla.org\mozilla-central\content\html\document\src\nshtmlcontentsink.cpp @ 743]
19 gklayout!SinkContext::CloseContainer(nsHTMLTag aTag = eHTMLTag_iframe (50), int aMalformed = 0)+0x214 [c:\home\mozilla.org\mozilla-central\content\html\document\src\nshtmlcontentsink.cpp @ 977]
1a gklayout!HTMLContentSink::CloseContainer(nsHTMLTag aTag = eHTMLTag_iframe (50))+0xa0 [c:\home\mozilla.org\mozilla-central\content\html\document\src\nshtmlcontentsink.cpp @ 2385]
1b gkparser!CNavDTD::CloseContainer(nsHTMLTag aTag = eHTMLTag_iframe (50), int aMalformed = 0)+0x18a [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\cnavdtd.cpp @ 2800]
1c gkparser!CNavDTD::CloseContainersTo(int anIndex = 2, nsHTMLTag aTarget = eHTMLTag_iframe (50), int aClosedByStartTag = 0)+0xb6 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\cnavdtd.cpp @ 2848]
1d gkparser!CNavDTD::CloseContainersTo(nsHTMLTag aTag = eHTMLTag_iframe (50), int aClosedByStartTag = 0)+0x67 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\cnavdtd.cpp @ 2992]
1e gkparser!CNavDTD::HandleEndToken(class CToken * aToken = 0x0797a6e8)+0x487 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\cnavdtd.cpp @ 1764]
1f gkparser!CNavDTD::HandleToken(class CToken * aToken = 0x0797a6e8, class nsIParser * aParser = 0x07820e40)+0x4ae [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\cnavdtd.cpp @ 760]
20 gkparser!CNavDTD::BuildModel(class nsIParser * aParser = 0x07820e40, class nsITokenizer * aTokenizer = 0x08c77c50, class nsITokenObserver * anObserver = 0x00000000, class nsIContentSink * aSink = 0x04916494)+0x295 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\cnavdtd.cpp @ 332]
21 gkparser!nsParser::BuildModel(void)+0xe2 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\nsparser.cpp @ 2352]
22 gkparser!nsParser::ResumeParse(int allowIteration = 1, int aIsFinalChunk = 1, int aCanInterrupt = 1)+0x1c4 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\nsparser.cpp @ 2221]
23 gkparser!nsParser::ContinueInterruptedParsing(void)+0xd2 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\nsparser.cpp @ 1728]
24 gkparser!nsParser::HandleParserContinueEvent(class nsParserContinueEvent * ev = 0x092868e0)+0x43 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\nsparser.cpp @ 1795]
25 gkparser!nsParserContinueEvent::Run(void)+0x19 [c:\home\mozilla.org\mozilla-central\parser\htmlparser\src\nsparser.cpp @ 162]
26 xpcom_core!nsThread::ProcessNextEvent(int mayWait = 1, int * result = 0x0012f848)+0x1fa [c:\home\mozilla.org\mozilla-central\xpcom\threads\nsthread.cpp @ 511]
27 xpcom_core!NS_ProcessNextEvent_P(class nsIThread * thread = 0x00ce2a60, int mayWait = 1)+0x53 [c:\home\mozilla.org\mozilla-central\dbg-firefox-i686-pc-mingw32\xpcom\build\nsthreadutils.cpp @ 227]
28 gkwidget!nsBaseAppShell::Run(void)+0x5d [c:\home\mozilla.org\mozilla-central\widget\src\xpwidgets\nsbaseappshell.cpp @ 170]
Comment 1 Jonas Sicking (:sicking) 2008-11-20 15:12:56 PST
Smaug: This looks a lot like bug 436965 right?
Comment 2 Olli Pettay [:smaug] 2008-11-21 05:44:31 PST
Actually, maybe not. Similar but not the same.
Comment 3 Daniel Veditz 2008-11-21 07:59:13 PST
timeless: any clue what you're doing when you get the assertion? or do you find it in console spew after the fact?

Guessing this is serious since bug 436965 is.
Comment 4 Olli Pettay [:smaug] 2008-11-22 09:45:56 PST
I think PresShell::IsSafeToFlush is wrong.
It is checking member variables of one particular presshell, but
nsContentUtils::IsSafeToRunScript() is global. So when flushing subframe, 
(!mIsReflowing && !mChangeNestCount) may be true, but scripts aren't safe to run.
Comment 5 Jonas Sicking (:sicking) 2008-11-22 10:53:46 PST
Indeed. Are those variables used for anything other than blocking flusing? If not we could make them static.
Comment 6 Boris Zbarsky (:bz) 2008-11-22 19:04:02 PST
Apart from assertions, they're used for the following:

1)  Blocking event handling (see PresShell::HandleEvent)
2)  Blocking WillPaint() notifications
3)  mIsReflowing is used to prevent posting of reflow events during reflow
4)  There's various code that's checking mIsReflowing by calling IsReflowLocked.
5)  mChangeNestCount is used to detect the "end of the update" in
    DidCauseReflow.  Note that there's an XXX comment about removing that block.

Now it's not clear that some of these can't be switched to checking for a script blocker instead, perhaps.  It might even be more correct.  That might allow us to deal with 1, 2, 5.  We could keep using mIsReflowing for 3 and 4 without basing IsSafeToFlush on it, I guess.
Comment 7 Olli Pettay [:smaug] 2008-11-24 08:29:42 PST
FYI, I run chrome/browser-chrome/mochitest with a patch
which makes PresShell::IsSafeToFlush to return the value of nsContentUtils::IsSafeToRunScript() and all the tests did pass.
I compared the mochitest log to a log of a normal run and didn't see
anything new. (And of course the total count of failed assertions dropped to
2/3 because PresShell::IsSafeToFlush didn't assert anymore.)
Comment 8 Olli Pettay [:smaug] 2008-11-27 08:42:58 PST
Created attachment 350343 [details] [review]
v1

Something like this.
Note, DoFlushPendingNotifications checks if it is safe to flush, so
no need to do that in PresShell::WillPaint.

Passes chrome/browser-chrome/mochitest, and I couldn't see any new assertions.
Comment 9 Jonas Sicking (:sicking) 2008-12-10 10:52:50 PST
Comment on attachment 350343 [details] [review]
v1

I'm not really familiar enough with this code to do a full review.

One comment though, can you assert if the paint-flag or mIsReflowing true while nsContentUtils::IsSafeToRunScript indicates that it is safe to run script.
Comment 10 Olli Pettay [:smaug] 2008-12-10 11:39:10 PST
(In reply to comment #9)
> One comment though, can you assert if the paint-flag or mIsReflowing true while
> nsContentUtils::IsSafeToRunScript indicates that it is safe to run script.
Running tests with such assertion....
Comment 11 Olli Pettay [:smaug] 2008-12-10 13:49:06 PST
Created attachment 352393 [details] [review]
with assertion

No assertions when running chrome/browser-chrome/mochitest
Comment 12 Boris Zbarsky (:bz) 2008-12-19 15:35:20 PST
Comment on attachment 352393 [details] [review]
with assertion

Looks good, since we have a script blocker in the view manager when painting.

Maybe file a followup bug on getting rid of nsIViewManager::IsPainting?
Comment 13 Jonas Sicking (:sicking) 2008-12-19 15:44:33 PST
Hah, just mid-aired with bzs review.

Hmm.. just realized one concern with this patch.

When I originally landed the scriptblockers patch I made IsSafeToFlush return
false if there were script blockers. However that did result in various
problems, i think partially due to there still being a few places where scripts
ran when there were script blockers. IIRC these scripts did run when it was
safe to run script, however such scripts were not able to use layout properly
since flushing never happened.

Bug 423269 seems to be when I backed that out.

It's entirely possible that things have improved since then, that was
definitely the goal, but we should check.
Comment 14 Olli Pettay [:smaug] 2008-12-19 15:51:12 PST
Ok, I'll check if I get any of those Bug 423269 etc. assertions before landing.
Comment 15 Olli Pettay [:smaug] 2008-12-20 06:30:26 PST
http://hg.mozilla.org/mozilla-central/rev/3d10285b201c
Comment 16 Jonas Sicking (:sicking) 2008-12-20 10:03:40 PST
In paricular, it's important to make sure that we never do the outermost BeginUpdate/EndUpdate while there are scripblockers since the last EndUpdate often will run scripts.
Comment 17 Olli Pettay [:smaug] 2008-12-20 10:07:04 PST
We probably want to change EndUpdate so that many of the things which happen
there moves to some script blocker.
Comment 18 Jonas Sicking (:sicking) 2008-12-20 11:07:04 PST
Indeed, we should do that too
Comment 19 Jonas Sicking (:sicking) 2008-12-25 21:51:12 PST
I think what we should do is add an assertion to EndUpdate that ensures that there are no pending scripblockers as any time that happens we are likely to run script while there are scriptblockers. Or truely weed all EndUpdate implementations of stuff that runs script.
Comment 20 Jonas Sicking (:sicking) 2009-01-14 02:19:47 PST
Filed bug 471166 on that.
Comment 21 Samuel Sidler (old account; do not CC) 2009-01-15 01:13:53 PST
Nominating per bug 436965 comment 45.
Comment 22 Jonas Sicking (:sicking) 2009-01-16 11:48:36 PST
Note that I don't think we can land on branch this without also fixing bug 471166.
Comment 23 Samuel Sidler (old account; do not CC) 2009-01-16 11:56:30 PST
Yep. That's also on our list and would need to get landed and baked prior to us taking these on 1.9.0.
Comment 24 Olli Pettay [:smaug] 2009-02-02 03:58:03 PST
Created attachment 360068 [details] [review]
1.9.0 patch

This is the same as the patch for trunk/1.9.1, but applies to 1.9.0 without any --fuzz
Comment 26 Daniel Veditz 2009-02-02 14:43:28 PST
Comment on attachment 360068 [details] [review]
1.9.0 patch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Comment 27 Al Billings [:abillings] 2009-02-13 12:15:09 PST
There seems to be nothing for QA to do here in order to verify this issue. Are there even steps to cause the initial issue?
Comment 28 Frank Wein [:mcsmurf] 2009-05-02 14:22:38 PDT
Might this bug here have caused Bug 491063 (mixed content warning broken, at least on one specific site?)? The bug seems to be the only bug that occurs both in the 1.9.1 and the mozilla-central change log for the regression range.
Comment 29 Jonas Sicking (:sicking) 2009-05-04 00:30:08 PDT
Please mark suspected regressions so we don't loose track of them. The dependency can always be removed if it turns out to be wrong.

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