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]

Smaug: This looks a lot like bug 436965 right?

Actually, maybe not. Similar but not the same.

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.

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.

Indeed. Are those variables used for anything other than blocking flusing? If not we could make them static.

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.

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.)

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 on attachment 350343 [details] [diff] [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.

(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....

No assertions when running chrome/browser-chrome/mochitest

Comment on attachment 352393 [details] [diff] [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?

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.

Ok, I'll check if I get any of those Bug 423269 etc. assertions before landing.

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.

We probably want to change EndUpdate so that many of the things which happen there moves to some script blocker.

Indeed, we should do that too

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.

Filed bug 471166 on that.

Nominating per bug 436965 comment 45.

Note that I don't think we can land on branch this without also fixing bug 471166.

Yep. That's also on our list and would need to get landed and baked prior to us taking these on 1.9.0.

This is the same as the patch for trunk/1.9.1, but applies to 1.9.0 without any --fuzz

Comment on attachment 360068 [details] [diff] [review] 1.9.0 patch Approved for 1.9.0.7, a=dveditz for release-drivers.

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?

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.

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.