Closed
Bug 397954
Opened 17 years ago
Closed 17 years ago
trees can execute script during frame construction
Categories
(Core :: Layout, defect, P3)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: smaug)
References
(Blocks 1 open bug)
Details
(Whiteboard: [sg:critical?][dbaron-1.9:RsCe])
Attachments
(1 file)
2.18 KB,
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
Trees can execute script during frame construction. I'm not entirely sure whose fault this is. With the patch in bug 335053, I see the assertion at the following stack during Firefox startup: ###!!! ASSERTION: should not execute script during frame construction: 'presContext->LayoutPhaseCount(eLayoutPhase_FrameC) == 0', file content/base/src/nsContentUtils.cpp, line 3718 nsContentUtils::AssertLayoutSafeForScript(nsIDocument*) (content/base/src/nsContentUtils.cpp:3717) nsJSContext::ScriptEvaluated(int) (dom/src/base/nsJSEnvironment.cpp:3210) nsJSContext::EvaluateStringWithValue(nsAString_internal const&, void*, nsIPrincipal*, char const*, unsigned int, unsigned int, void*, int*) (dom/src/base/nsJSEnvironment.cpp:1232) nsXBLProtoImplField::InstallField(nsIScriptContext*, JSObject*, nsIURI*) const (content/xbl/src/nsXBLProtoImplField.cpp:126) XBLResolve (content/xbl/src/nsXBLBinding.cpp:210) js_LookupPropertyWithFlags (js/src/jsobj.c:3256) js_LookupProperty (js/src/jsobj.c:3160) js_SetProperty (js/src/jsobj.c:3606) js_Interpret (js/src/jsinterp.c:3843) js_Invoke (js/src/jsinterp.c:1402) js_InternalInvoke (js/src/jsinterp.c:1459) JS_CallFunctionValue (js/src/jsapi.c:4940) nsXBLProtoImplAnonymousMethod::Execute(nsIContent*) (content/xbl/src/nsXBLProtoImplMethod.cpp:355) nsXBLPrototypeBinding::BindingAttached(nsIContent*) (content/xbl/src/nsXBLPrototypeBinding.cpp:491) nsXBLBinding::ExecuteAttachedHandler() (content/xbl/src/nsXBLBinding.cpp:938) nsBindingManager::ProcessAttachedQueue() (content/xbl/src/nsBindingManager.cpp:833) PresShell::DoFlushPendingNotifications(mozFlushType, int) (layout/base/nsPresShell.cpp:4453) PresShell::FlushPendingNotifications(mozFlushType) (layout/base/nsPresShell.cpp:4402) nsBoxObject::GetFrame(int) (layout/xul/base/src/nsBoxObject.cpp:138) nsTreeBoxObject::GetTreeBody() (layout/xul/base/src/tree/src/nsTreeBoxObject.cpp:117) nsTreeBodyFrame::EnsureBoxObject() (layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:380) nsTreeBodyFrame::Init(nsIContent*, nsIFrame*, nsIFrame*) (layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:229) nsCSSFrameConstructor::InitAndRestoreFrame(nsFrameConstructorState const&, nsIContent*, nsIFrame*, nsIFrame*, nsIFrame*, int) (layout/base/nsCSSFrameConstructor.cpp:6682) nsCSSFrameConstructor::ConstructXULFrame(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int, nsStyleContext*, nsFrameItems&, int, int, int*) (layout/base/nsCSSFrameConstructor.cpp:6110) nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int, nsStyleContext*, nsFrameItems&, int) (layout/base/nsCSSFrameConstructor.cpp:7623) nsCSSFrameConstructor::ConstructFrame(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) (layout/base/nsCSSFrameConstructor.cpp:7484) nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsIFrame*, int, nsFrameItems&, int) (layout/base/nsCSSFrameConstructor.cpp:11240) ... If the rule should be that you can't FlushPendingNotifications during frame construction, we should add an assertion to that effect. Or is this a deficiency in the fix for bug 372769? Or something else?
Flags: blocking1.9?
Reporter | ||
Comment 1•17 years ago
|
||
I guess there really isn't anything specific to fields here, since we also assert within ProcessAttachedQueue here: nsContentUtils::AssertLayoutSafeForScript(nsIDocument*) (content/base/src/nsContentUtils.cpp:3717) nsJSContext::ScriptEvaluated(int) (dom/src/base/nsJSEnvironment.cpp:3210) nsCxPusher::Pop() (content/base/src/nsContentUtils.cpp:2510) ~nsCxPusher (content/base/src/nsContentUtils.cpp:2393) nsXBLProtoImplAnonymousMethod::Execute(nsIContent*) (content/xbl/src/nsXBLProtoImplMethod.cpp:362) nsXBLPrototypeBinding::BindingAttached(nsIContent*) (content/xbl/src/nsXBLPrototypeBinding.cpp:491) nsXBLBinding::ExecuteAttachedHandler() (content/xbl/src/nsXBLBinding.cpp:938) nsBindingManager::ProcessAttachedQueue() (content/xbl/src/nsBindingManager.cpp:833) ...and I'd think my patch really ought to be triggering an assertion in some of the XBL code that calls JS_CallFunctionValue.
Yeah, we really "start" executing scripts here once we call nsBindingManager::ProcessAttachedQueue, after that it's just scripts triggering more scripts. Should nsPresShell::IsSafeToFlush be returning false here? If it should then that would be a good place to start looking.
![]() |
||
Comment 3•17 years ago
|
||
FlushPendingNotifications during frame construction is bad. If there are pending restyles that trigger reframes, we could wipe out a frame tree when we're in the middle of constructing stuff for it. We should consider changing the tree code to not trigger a flush here, but also perhaps adjust IsSafeToFlush as Jonas suggests.
Yeah, if FlushPendingNotifications during tree construction is bad then we should at least assert if it happens, and probably change IsSafeToFlush. Ideally both even.
Whiteboard: [sg:critical?]
Flags: blocking1.9? → blocking1.9+
Reporter | ||
Comment 5•17 years ago
|
||
Bug 399715 (not security-sensitive) is likely to be fixed by a fix for this bug.
Reporter | ||
Updated•17 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][dbaron-1.9:RsCe]
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 6•17 years ago
|
||
I added the GetTreeBody call, which causes flushing, to EnsureBoxObject in bug 381862. That wasn't the best idea. Better to just check that treeboxobject isn't currently attached to some other treebodyframe, so cached value should be enough.
Attachment #286001 -
Flags: superreview?(roc)
Attachment #286001 -
Flags: review?(roc)
Attachment #286001 -
Flags: superreview?(roc)
Attachment #286001 -
Flags: superreview+
Attachment #286001 -
Flags: review?(roc)
Attachment #286001 -
Flags: review+
Assignee | ||
Comment 7•17 years ago
|
||
Comment on attachment 286001 [details] [diff] [review] Use possible cached TreeBody Not needed for m9
Attachment #286001 -
Flags: approval1.9?
Comment 8•17 years ago
|
||
Comment on attachment 286001 [details] [diff] [review] Use possible cached TreeBody a=drivers for after M9 freeze
Attachment #286001 -
Flags: approval1.9? → approval1.9+
Priority: -- → P3
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite?
Updated•12 years ago
|
Group: core-security
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•