Last Comment Bug 472502 - "ASSERTION: Missing a script blocker" loading 400790-1.xul
: "ASSERTION: Missing a script blocker" loading 400790-1.xul
Status: VERIFIED FIXED
: [sg:investigate] post 1.8-branch
: assertion, regression, testcase, verified1.9.0.7, verified1.9.1
Product: Core
Classification: Components
Component: XUL
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: xptoolkit.widgets
:
:
: 385276 436965
  Show dependency treegraph
 
Reported: 2009-01-07 07:45 PST by Jesse Ruderman
Modified: 2009-06-10 11:49 PDT (History)
11 users (show)
jonas: blocking1.9.1+
dveditz: blocking1.9.0.7+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
jruderman: in‑testsuite+
See Also:
Crash Signature:


Attachments
copy of layout/xul/base/src/grid/crashtests/400790-1.xul (529 bytes, application/vnd.mozilla.xul+xml)
2009-01-07 07:45 PST, Jesse Ruderman
no flags Details
patch (612 bytes, patch)
2009-01-08 06:24 PST, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2009-01-07 07:45:18 PST
Created attachment 355761 [details]
copy of layout/xul/base/src/grid/crashtests/400790-1.xul

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.
Comment 1 Olli Pettay [:smaug] 2009-01-07 08:29:16 PST
Hmm, I put the assertion to wrong place. Native anon content (not iframes or frames) is bound during frame construction. Patch coming.
Comment 2 Jonas Sicking (:sicking) 2009-01-07 08:52:04 PST
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.
Comment 3 Olli Pettay [:smaug] 2009-01-07 09:01:54 PST
Indeed. Idiot me.
Comment 4 Olli Pettay [:smaug] 2009-01-07 09:08:11 PST
Ah, fun. nsListBoxBodyFrame does its own frame creation in nsListBoxBodyFrame::GetNextItemBox
Comment 5 Olli Pettay [:smaug] 2009-01-07 10:29:54 PST
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
Comment 6 Jonas Sicking (:sicking) 2009-01-07 11:09:20 PST
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?
Comment 7 Olli Pettay [:smaug] 2009-01-07 14:12:26 PST
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.
Comment 8 Olli Pettay [:smaug] 2009-01-08 06:24:04 PST
Created attachment 355969 [details] [review]
patch

This is the best place for a blocker I can think of.
Comment 9 Jonas Sicking (:sicking) 2009-01-09 11:00:59 PST
Comment on attachment 355969 [details] [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.
Comment 10 Boris Zbarsky (:bz) 2009-01-09 11:17:15 PST
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.
Comment 11 Olli Pettay [:smaug] 2009-01-09 12:10:02 PST
Er, we kind of expect that scripts may run in ReflowFinished. If that is not
the case, caller should be fixed.
Comment 12 Boris Zbarsky (:bz) 2009-01-09 12:21:52 PST
> 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.
Comment 13 Daniel Veditz 2009-01-16 11:13:59 PST
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.
Comment 14 Olli Pettay [:smaug] 2009-01-17 09:51:37 PST
http://hg.mozilla.org/mozilla-central/rev/bcb61d06a3d4

I'll investigate if this is a real security problem.
Comment 15 Olli Pettay [:smaug] 2009-02-02 03:59:47 PST
Any chance to get the 1.9.1 approval?
Comment 16 Olli Pettay [:smaug] 2009-02-02 04:59:03 PST
The patch applies to 1.9.0 too.
Comment 18 Boris Zbarsky (:bz) 2009-02-02 06:36:32 PST
Per comment 17, shouldn't this just be nominated for blocking, not for just for approval?
Comment 19 Olli Pettay [:smaug] 2009-02-02 09:54:36 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/183cdd006e44
Comment 20 Daniel Veditz 2009-02-02 14:49:57 PST
Comment on attachment 355969 [details] [review]
patch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Comment 21 Daniel Veditz 2009-02-12 12:20:25 PST
Doesn't seem to be needed in 1.8. Unhiding comment 6 now that the bug itself is private.
Comment 22 Al Billings [:abillings] 2009-02-13 12:08:40 PST
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)?
Comment 23 Olli Pettay [:smaug] 2009-02-13 12:22:21 PST
Use a debug build and make sure that you don't get that assertion when running the test.
Comment 24 Al Billings [:abillings] 2009-02-13 13:54:06 PST
Tomcat, can you do this? You're the one person in QA who builds all of the debug builds.
Comment 25 Carsten Book [:Tomcat] 2009-02-14 08:50:33 PST
(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
Comment 26 Aakash Desai [:aakashd] 2009-06-10 11:49:18 PDT
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

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