trees can execute script during frame construction

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
P3
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: dbaron, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][dbaron-1.9:RsCe])

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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

11 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.
(Reporter)

Updated

11 years ago
Blocks: 335053
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.
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

11 years ago
Bug 399715 (not security-sensitive) is likely to be fixed by a fix for this bug.
(Reporter)

Updated

11 years ago
Depends on: 391178
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical?] → [sg:critical?][dbaron-1.9:RsCe]
(Assignee)

Updated

11 years ago
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 6

11 years ago
Created attachment 286001 [details] [diff] [review]
Use possible cached TreeBody

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

11 years ago
Comment on attachment 286001 [details] [diff] [review]
Use possible cached TreeBody

Not needed for m9
Attachment #286001 - Flags: approval1.9?
Comment on attachment 286001 [details] [diff] [review]
Use possible cached TreeBody

a=drivers for after M9 freeze
Attachment #286001 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.