1366251 - Force painting can paint during frame construction by interrupting debugger JavaScript that runs in response to XBL binding global object creation

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

[Markus Stange [:mstange]]

Not sure if security sensitive.

One of my try pushes has hit this assertion: https://treeherder.mozilla.org/logviewer.html#?job_id=100375411&repo=try&lineNumber=28679

> Assertion failure: mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0 (painting in the middle of frame construction), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/layout/base/nsAutoLayoutPhase.cpp:39
> #01: mozilla::PresShell::Paint(nsView *,nsRegion const &,unsigned int) [layout/base/PresShell.cpp:6396]
> #02: mozilla::dom::TabChild::RecvSetDocShellIsActive(bool const &,bool const &,unsigned __int64 const &) [dom/ipc/TabChild.cpp:2560]
> #03: mozilla::dom::TabChild::ForcePaint(unsigned __int64) [dom/ipc/TabChild.cpp:3349]
> #04: `anonymous namespace'::HangMonitorChild::InterruptCallback [dom/ipc/ProcessHangMonitor.cpp:342]
> #05: InterruptCallback [dom/ipc/ProcessHangMonitor.cpp:1104]
> #06: JSContext::handleInterrupt() [js/src/vm/Runtime.cpp:597]
> #07: ??? (???:???)
> #08: EnterBaseline [js/src/jit/BaselineJIT.cpp:164]
> #09: js::jit::EnterBaselineMethod(JSContext *,js::RunState &) [js/src/jit/BaselineJIT.cpp:200]
> #10: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:400]
> #11: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:488]
> #12: InternalCall [js/src/vm/Interpreter.cpp:515]
> #13: Interpret [js/src/vm/Interpreter.cpp:3028]
> #14: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:410]
> #15: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:488]
> #16: InternalCall [js/src/vm/Interpreter.cpp:515]
> #17: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:534]
> #18: js::Wrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/Wrapper.cpp:166]
> #19: js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/CrossCompartmentWrapper.cpp:353]
> #20: js::Proxy::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/Proxy.cpp:479]
> #21: js::proxy_Call(JSContext *,unsigned int,JS::Value *) [js/src/proxy/Proxy.cpp:739]
> #22: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:293]
> #23: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:452]
> #24: InternalCall [js/src/vm/Interpreter.cpp:515]
> #25: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:534]
> #26: js::Call(JSContext *,JS::Handle<JS::Value>,JSObject *,JS::Handle<JS::Value>,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.h:112]
> #27: js::Debugger::fireNewGlobalObject(JSContext *,JS::Handle<js::GlobalObject *>,JS::MutableHandle<JS::Value>) [js/src/vm/Debugger.cpp:2145]
> #28: js::Debugger::slowPathOnNewGlobalObject(JSContext *,JS::Handle<js::GlobalObject *>) [js/src/vm/Debugger.cpp:2202]
> #29: js::Debugger::onNewGlobalObject(JSContext *,JS::Handle<js::GlobalObject *>) [js/src/vm/Debugger.h:1805]
> #30: JS_FireOnNewGlobalObject(JSContext *,JS::Handle<JSObject *>) [js/src/jsapi.cpp:1909]
> #31: xpc::CreateSandboxObject(JSContext *,JS::MutableHandle<JS::Value>,nsISupports *,xpc::SandboxOptions &) [js/xpconnect/src/Sandbox.cpp:1227]
> #32: XPCWrappedNativeScope::EnsureContentXBLScope(JSContext *) [js/xpconnect/src/XPCWrappedNativeScope.cpp:307]
> #33: xpc::GetXBLScope(JSContext *,JSObject *) [js/xpconnect/src/XPCWrappedNativeScope.cpp:335]
> #34: mozilla::dom::FindAssociatedGlobal<nsISupports> [obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1609]
> #35: mozilla::dom::FindAssociatedGlobal<mozilla::dom::ParentObject> [obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1621]
> #36: mozilla::dom::XULElementBinding::Wrap(JSContext *,nsXULElement *,nsWrapperCache *,JS::Handle<JSObject *>,JS::MutableHandle<JSObject *>) [obj-firefox/dom/bindings/XULElementBinding.cpp:8992]
> #37: mozilla::dom::XULElementBinding::Wrap<nsXULElement>(JSContext *,nsXULElement *,JS::Handle<JSObject *>) [obj-firefox/dist/include/mozilla/dom/XULElementBinding.h:47]
> #38: nsXULElement::WrapNode(JSContext *,JS::Handle<JSObject *>) [dom/xul/nsXULElement.cpp:2025]
> #39: nsINode::WrapObject(JSContext *,JS::Handle<JSObject *>) [dom/base/nsINode.cpp:2946]
> #40: mozilla::dom::Element::WrapObject(JSContext *,JS::Handle<JSObject *>) [dom/base/Element.cpp:526]
> #41: XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>,nsIXPConnectJSObjectHolder * *,xpcObjectHelper &,nsID const *,bool,nsresult *) [js/xpconnect/src/XPCConvert.cpp:792]
> #42: NativeInterface2JSObject [js/xpconnect/src/nsXPConnect.cpp:619]
> #43: nsXPConnect::WrapNativeToJSVal(JSContext *,JSObject *,nsISupports *,nsWrapperCache *,nsID const *,bool,JS::MutableHandle<JS::Value>) [js/xpconnect/src/nsXPConnect.cpp:686]
> #44: nsContentUtils::WrapNative(JSContext *,nsISupports *,nsWrapperCache *,nsID const *,JS::MutableHandle<JS::Value>,bool) [dom/base/nsContentUtils.cpp:6572]
> #45: nsXBLProtoImpl::InitTargetObjects(nsXBLPrototypeBinding *,nsIContent *,JS::MutableHandle<JSObject *>,bool *) [dom/xbl/nsXBLProtoImpl.cpp:220]
> #46: nsXBLProtoImpl::InstallImplementation(nsXBLPrototypeBinding *,nsXBLBinding *) [dom/xbl/nsXBLProtoImpl.cpp:70]
> #47: nsXBLPrototypeBinding::InstallImplementation(nsXBLBinding *) [dom/xbl/nsXBLPrototypeBinding.cpp:319]
> #48: nsXBLBinding::InstallImplementation() [dom/xbl/nsXBLBinding.cpp:589]
> #49: nsXBLService::LoadBindings(nsIContent *,nsIURI *,nsIPrincipal *,nsXBLBinding * *,bool *) [dom/xbl/nsXBLService.cpp:544]
> #50: nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState &,nsIContent *,nsContainerFrame *,nsIAtom *,int,bool,nsStyleContext *,unsigned int,nsTArray<nsIAnonymousContentCreator::ContentInfo> *,nsCSSFrameConstructor::FrameConstructionItemList &) [layout/base/nsCSSFrameConstructor.cpp:5780]
> #51: nsCSSFrameConstructor::AddFCItemsForAnonymousContent(nsFrameConstructorState &,nsContainerFrame *,nsTArray<nsIAnonymousContentCreator::ContentInfo> &,nsCSSFrameConstructor::FrameConstructionItemList &,unsigned int) [layout/base/nsCSSFrameConstructor.cpp:11036]
> #52: nsCSSFrameConstructor::BeginBuildingScrollFrame(nsFrameConstructorState &,nsIContent *,nsStyleContext *,nsContainerFrame *,nsIAtom *,bool,nsContainerFrame * &) [layout/base/nsCSSFrameConstructor.cpp:4612]
> #53: nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent *) [layout/base/nsCSSFrameConstructor.cpp:2919]
> #54: nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element *,nsILayoutHistoryState *) [layout/base/nsCSSFrameConstructor.cpp:2428]
> #55: nsCSSFrameConstructor::ContentRangeInserted(nsIContent *,nsIContent *,nsIContent *,nsILayoutHistoryState *,bool,bool,TreeMatchContext *) [layout/base/nsCSSFrameConstructor.cpp:7942]
> #56: nsCSSFrameConstructor::ContentInserted(nsIContent *,nsIContent *,nsILayoutHistoryState *,bool) [layout/base/nsCSSFrameConstructor.cpp:7828]
> #57: mozilla::PresShell::Initialize(int,int) [layout/base/PresShell.cpp:1798]
> #58: nsContentSink::StartLayout(bool) [dom/base/nsContentSink.cpp:1240]
> #59: nsHtml5TreeOpExecutor::StartLayout(bool *) [parser/html/nsHtml5TreeOpExecutor.cpp:627]
> #60: nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor *,nsIContent * *,bool *) [parser/html/nsHtml5TreeOperation.cpp:999]
> #61: nsHtml5TreeOpExecutor::RunFlushLoop() [parser/html/nsHtml5TreeOpExecutor.cpp:459]
> #62: nsHtml5ExecutorFlusher::Run() [parser/html/nsHtml5StreamParser.cpp:131]
> #63: mozilla::SchedulerGroup::Runnable::Run() [xpcom/threads/SchedulerGroup.cpp:370]
> #64: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1271]
> #65: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:393]
> #66: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:96]
> #67: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:301]
> #68: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:238]
> #69: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:232]
> #70: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:212]
> #71: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:158]
> #72: nsAppShell::Run() [widget/windows/nsAppShell.cpp:271]
> #73: XRE_RunAppShell() [toolkit/xre/nsEmbedFunctions.cpp:893]
> #74: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:269]
> #75: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:238]
> #76: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:232]
> #77: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:212]
> #78: XRE_InitChildProcess(int,char * * const,XREChildData const *) [toolkit/xre/nsEmbedFunctions.cpp:713]
> #79: mozilla::BootstrapImpl::XRE_InitChildProcess(int,char * * const,XREChildData const *) [toolkit/xre/Bootstrap.cpp:65]
> #80: content_process_main(mozilla::Bootstrap *,int,char * * const) [ipc/contentproc/plugin-container.cpp:64]
> #81: NS_internal_main(int,char * *,char * *) [browser/app/nsBrowserApp.cpp:285]
> #82: wmain [toolkit/xre/nsWindowsWMain.cpp:118]
> #83: __scrt_common_main_seh [f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253]
> #84: kernel32.dll + 0x53c45
> #85: ntdll.dll + 0x637f5
> #86: ntdll.dll + 0x637c8

Looks like nsCSSFrameConstructor::AddFrameConstructionItemsInternal can call nsXBLService::LoadBindings which runs JS.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Gah.  nsXBLService::LoadBindings is explicitly supposed to NOT run JS.  The frame constructor takes great pains to avoid doing that by queueing up things like XBL constructors for later execution, once we are at a safe point.

What the stack is showing is that when we create the XBL global (which is lazily-created), if the JS debugger is active it gets notified about the new global and goes and runs a bunch of script which we then interrupt and do painting.  This is highly unexpected from the point of view the caller of LoadBindings.

More generally, I think it's unexpected from the point of view of _any_ global object creators that interruption and possible reentry can happen from just creating a global object.

So some possible things we could do:

1) We could have our interrupt callback check nsContentUtils::IsSafeToRunScript() (which is false in the stack above) and not do whatever reentry-ish stuff it was considering doing.  This seems like the most general solution.

2) We could defer the JS_FireOnNewglobalObject call to a scriptrunner instead of making it pretty much immediately after constructing the new global.  I don't know whether that would violate some sort of JS/debugger invariants.  Jim, do you know?

3) We could just eagerly create the content XBL scope before we do any frame construction.  I don't know how often we end up creating it in practice, though.  At first glance, scrollbars don't need it.  Videocontrols obviously do.

Comment 2

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

Bug 1380307 is a (currently) public duplicate of this.

Comment 3

[Jim Blandy :jimb]

In the long term, we're not going to have the debugger server written in JS any longer. It's been a horrible pain in the neck, because of issues exactly like this. (Also, the only thing an onNewGlobalHook is ever supposed to do is just decide whether the global is interesting, and possibly make it a debuggee, so why it got killed by the slow script dialog is beyond me.)

In the immediate term, the only thing that matters is that JS_FireOnNewGlobalObject be called before any JS code can run in the global. So if we can move the call later, while still being confident that we're not hiding anything from the debugger, that'd be great.

Comment 4

[Markus Stange [:mstange]]

(In reply to Jim Blandy :jimb from comment #3)
> so why it got killed by the slow script dialog is beyond me.)

It didn't - we're using the JS Interrupt callback when switching tabs in order to paint the new tab as quickly as possible.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Should we disable the interrupt callback during JS_FireOnNewGlobalObject? If the code that runs is always expected to be simple, then there shouldn't be any harm in it.

Comment 6

[Jim Blandy :jimb]

If it's not simple, then that's a bug. So I think disabling the interrupt callback then should be okay.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That would certainly be easier than option 2 from comment 1, since it could be centralized.

Option 1 from comment 1 is also fairly centralized, but relies on a scriptblocker being around...  Of course running script while a scriptblocker is around is also kinda weird.  :(

Comment 8

[Jim Blandy :jimb]

Well, if a scriptblocker is around, there's no need to run onNewGlobal notifications yet. There's be no harm done to the Debugger API if onNewGlobal notifications were delayed until all scriptblockers were out of scope.

Comment 9

[Mats Palmgren (inactive)]

This doesn't seem like a bug in Layout code per se, so it's probably
better owned by some other team.  The solutions discussed above suggests
tweaking dom/ipc or JS engine code.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This doesn't look like it belongs in the IPC component; the Recv method at the top of the stack in comment #0 is one of the ones that's called manually.

dom/ipc mostly belongs to the DOM module, so let's try that.

Comment 11

[Boris Zbarsky [:bzbarsky]]

With XBL scopes (and XBL) gone, I believe this is a nonissue now. Brian, I assume the custom element replacements for XBL don't end up doing script stuff during frame construction, right?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Boris Zbarsky [:bzbarsky] from comment #11)

With XBL scopes (and XBL) gone, I believe this is a nonissue now. Brian, I assume the custom element replacements for XBL don't end up doing script stuff during frame construction, right?

The Custom Elements themselves have no mechanism to do that. There may be exceptions if an element implemented xpcom and had a method directly called,m but it's not a general problem anymore so I agree with resolving this.