397954 - trees can execute script during frame construction

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, if FlushPendingNotifications during tree construction is bad then we should at least assert if it happens, and probably change IsSafeToFlush. Ideally both even.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Not needed for m9

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a=drivers for after M9 freeze