layout/xul/base/src/grid/crashtests/400790-1.xul triggers ###!!! ASSERTION: Missing a script blocker!: '!nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/content/xul/content/src/nsXULElement.cpp, line 853 Looks like this assertion was added in bug 436965.

Hmm, I put the assertion to wrong place. Native anon content (not iframes or frames) is bound during frame construction. Patch coming.

But shouldn't there always be a scriptblocker while BindToTree is called? When we're binding native anon content we're inside frameconstruction, and when we're binding normal content we're inside BeginUpdate/EndUpdate. More to the point, while we're inside BindToTree we're in an inconsistent state and it would be unsafe to run script.

Indeed. Idiot me.

Ah, fun. nsListBoxBodyFrame does its own frame creation in nsListBoxBodyFrame::GetNextItemBox

This is the interesting part of the stack. #21 0x00002aaab0ba59ec in nsCSSFrameConstructor::CreateListBoxContent (this=0xa4b540, aPresContext=<value optimized out>, aParentFrame=0x1d3daf8, aPrevFrame=0x21d5ee0, aChild=0x1f3a3b0, aNewFrame=0x7fff70884220, aIsAppend=0, aIsScrollbar=0, aFrameState=0x0) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/base/nsCSSFrameConstructor.cpp:12262 #22 0x00002aaab0d6a4e2 in nsListBoxBodyFrame::GetNextItemBox (this=0x1d3daf8, aBox=0x21d5ee0, aOffset=<value optimized out>, aCreated=0x7fff7088427c) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:1236 #23 0x00002aaab0d6a645 in nsListBoxBodyFrame::CreateRows (this=0x1d3daf8) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:1074 #24 0x00002aaab0d6aa2a in nsListBoxBodyFrame::ReflowFinished (this=0x3a7f) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp:476 #25 0x00002aaab0be9a31 in PresShell::HandlePostedReflowCallbacks (this=0x31a9680) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/base/nsPresShell.cpp:4381 #26 0x00002aaab0bf3765 in PresShell::DidDoReflow (this=0x31a9680) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/base/nsPresShell.cpp:6181 #27 0x00002aaab0bf6d84 in PresShell::ProcessReflowCommands (this=0x31a9680, aInterruptible=1) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/base/nsPresShell.cpp:6369 #28 0x00002aaab0bf6f34 in PresShell::DoFlushPendingNotifications (this=0x31a9680, aType=Flush_Layout, aInterruptibleReflow=1) at /home/smaug/mozilla/mozilla_cvs/hg/mozilla/layout/base/nsPresShell.cpp:4487

So nsCSSFrameConstructor::CreateListBoxContent creating frames results in XBL bindings being attached which causes BindToTree to be called? (would be good with the rest of the stack until BindToTree). Does this mean that if you stick a <script> or <iframe> inside a binding and bind it to an element inside a listbox you can run script from nsListBoxBodyFrame::GetNextItemBox? If so, should we mark this bug security sensitive? I suspect so. Is the right place to add a scriptblocker somewhere in frame 25-27 in comment 5?

Marking security sensitive for now, although I think or hope this doesn't need to be. The bound element is something in scrollbar, which is in native anon. I'll look at this tomorrow, if I just have time.

This is the best place for a blocker I can think of.

Comment on attachment 355969 [details] [diff] [review] patch I bz really is a better person to review this. My concern here is if the caller of this function is prepared for scripts executing before the function returns.

It's not quite. We should probably avoid doing the flush in HandlePostedReflowCallbacks if we've had Destroy() called, at the very least. We might also want to hold a deathgrip there.

Er, we kind of expect that scripts may run in ReflowFinished. If that is not the case, caller should be fixed.

> Er, we kind of expect that scripts may run in ReflowFinished. That's what I thought, but if ReflowFinished returns true, we'll call FlushPendingNotifications in the caller. If the presshell got destroyed by the script, mViewManager will be null. I guess we assert-but-don't-crash in that case, ok. It's still probably a good idea to add some safety to the caller, per above, but I'm ok with a followup bug for that.

Did this turn out to be a security problem and need to land on the 1.9.0 branch if we take bug 436965? Please adjust sg:investigate to the appropriate severity.

http://hg.mozilla.org/mozilla-central/rev/bcb61d06a3d4 I'll investigate if this is a real security problem.

Any chance to get the 1.9.1 approval?

The patch applies to 1.9.0 too.

Per comment 17, shouldn't this just be nominated for blocking, not for just for approval?

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

Doesn't seem to be needed in 1.8. Unhiding comment 6 now that the bug itself is private.

The reftest in comment 0 passes now? Is there any particular way to verify this fix on 1.9.0.7 beyond this? Would the reftest catch this (I assume so)?

Use a debug build and make sure that you don't get that assertion when running the test.

Tomcat, can you do this? You're the one person in QA who builds all of the debug builds.

(In reply to comment #24) > Tomcat, can you do this? You're the one person in QA who builds all of the > debug builds. also done :) verified 1.9.0.7 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021407 Firefox/3.0.7pre - don't see this assertion in the debug build

verified FIXED (no assertion via attached testcase) on debug builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028