564461 - "ASSERTION: This is unsafe! Fix the caller!" and more with mutation events, XBL, contenteditable

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers a bunch of assertions:

###!!! ASSERTION: no frame, see bug #188946: 'frame', file editor/libeditor/base/nsEditor.cpp, line 3968

###!!! ASSERTION: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file layout/base/nsPresContext.h, line 1262

###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file content/base/src/nsContentUtils.cpp, line 3392

###!!! ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()', file content/base/src/nsContentUtils.cpp, line 5941

###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file content/events/src/nsEventDispatcher.cpp, line 514

(and then the last two assertions repeat)

Comment 1

[(no longer active)]

With my patch in bug 336104, I only see one warning, and no assertions:

WARNING: *** XBL doc with no root element! Something went horribly wrong! ***: file /Users/ehsanakhgari/moz/src/content/xbl/src/nsXBLService.cpp, line 443

That said, even with that patch not applied, I only get the first assertion with the attached test case (as well as the same warning).  Are you sure it's the right test case?  Do I need to do something else besides just loading the test case?

Comment 2

[Jesse Ruderman]

Since the bug is security-sensitive and the testcase involves XBL, you have to download the testcase to use it.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Since the bug is security-sensitive and the testcase involves XBL, you have to
> download the testcase to use it.

Oh, I didn't know that.

So, when I load the testcase from a file:// URI, I don't get any assertions or warnings, I just get this on my error console:

Warning: An XBL file is malformed. Did you forget the XBL namespace on the bindings tag?
Source File: file:///Users/ehsanakhgari/moz/564461.xhtml
Line: 0

Which may explain why I don't get any assertions/warnings.

Comment 4

[(no longer active)]

I managed to reproduce the problem.  I'm starting to investigate.

Comment 5

[(no longer active)]

So, what happens here is that when nsXBLService::LoadBindings is called for the body element after its MozBinding style property is set, all the three cases in nsXBLService::LoadBindingDocumentInfo fail, and it goes ahead and calls nsXBLService::FetchBindingDocument, which goes ahead and loads the original file from disk asynchronously, which contains the bindings element, and then execution follows up until the first assertion:

#0	0x003e0c19 in NS_DebugBreak_P at nsDebugImpl.cpp:258
#1	0x12033bc2 in nsAutoLayoutPhase::Enter at nsPresContext.h:1261
#2	0x12033c60 in nsAutoLayoutPhase::nsAutoLayoutPhase at nsPresContext.h:1228
#3	0x1202f95c in nsCSSFrameConstructor::ContentAppended at nsCSSFrameConstructor.cpp:6504
#4	0x120aafdb in PresShell::ContentAppended at nsPresShell.cpp:4803
#5	0x123e1e60 in nsNodeUtils::ContentAppended at nsNodeUtils.cpp:138
#6	0x123c668e in nsGenericElement::doInsertChildAt at nsGenericElement.cpp:3622
#7	0x123c686d in nsGenericElement::InsertChildAt at nsGenericElement.cpp:3553
#8	0x123c59b2 in nsINode::ReplaceOrInsertBefore at nsGenericElement.cpp:4309
#9	0x123c4029 in nsINode::ReplaceOrInsertBefore at nsGenericElement.cpp:555
#10	0x123ae4b0 in nsGenericElement::InsertBefore at nsGenericElement.h:499
#11	0x124b59fe in nsHTMLBodyElement::InsertBefore at nsHTMLBodyElement.cpp:93
#12	0x1278b99c in InsertElementTxn::DoTransaction at InsertElementTxn.cpp:129
#13	0x1276f93b in nsEditor::DoTransaction at nsEditor.cpp:648
#14	0x1276d45d in nsEditor::InsertNode at nsEditor.cpp:1350
#15	0x1275b356 in nsTextEditRules::CreateBogusNodeIfNeeded at nsTextEditRules.cpp:1355
#16	0x1275e345 in nsTextEditRules::Init at nsTextEditRules.cpp:153
#17	0x12a82234 in nsHTMLEditRules::Init at nsHTMLEditRules.cpp:257
#18	0x12a5b3ec in nsHTMLEditor::InitRules at nsHTMLEditor.cpp:421
#19	0x12751f2f in nsPlaintextEditor::EndEditorInit at nsPlaintextEditor.cpp:225
#20	0x12759401 in nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger at nsTextEditUtils.cpp:134
#21	0x12a5e0bd in nsHTMLEditor::Init at nsHTMLEditor.cpp:268
#22	0x150f0473 in nsEditingSession::SetupEditorOnWindow at nsEditingSession.cpp:491
#23	0x150f0aa8 in nsEditingSession::MakeWindowEditable at nsEditingSession.cpp:207
#24	0x1253950f in nsHTMLDocument::EditingStateChanged at nsHTMLDocument.cpp:3274
#25	0x1253a34e in nsHTMLDocument::ChangeContentEditableCount at nsHTMLDocument.cpp:3020
#26	0x124a61bb in nsGenericHTMLElement::BindToTree at nsGenericHTMLElement.cpp:889
#27	0x125ec270 in nsXBLBinding::InstallAnonymousContent at nsXBLBinding.cpp:361
#28	0x125ef5ca in nsXBLBinding::GenerateAnonymousContent at nsXBLBinding.cpp:690
#29	0x1260e06a in nsXBLService::LoadBindings at nsXBLService.cpp:640
#30	0x120221e7 in nsCSSFrameConstructor::AddFrameConstructionItemsInternal at nsCSSFrameConstructor.cpp:5104
#31	0x12022b62 in nsCSSFrameConstructor::AddFrameConstructionItems at nsCSSFrameConstructor.cpp:5040
#32	0x1202c4d2 in nsCSSFrameConstructor::ContentRangeInserted at nsCSSFrameConstructor.cpp:7154
#33	0x1202ceb2 in nsCSSFrameConstructor::ContentInserted at nsCSSFrameConstructor.cpp:6813
#34	0x1202de3a in nsCSSFrameConstructor::RecreateFramesForContent at nsCSSFrameConstructor.cpp:9123
#35	0x1202e976 in nsCSSFrameConstructor::ProcessRestyledFrames at nsCSSFrameConstructor.cpp:7983
#36	0x120a04fc in PresShell::RecreateFramesFor at nsPresShell.cpp:3468
#37	0x12610cad in nsXBLBindingRequest::DocumentLoaded at nsXBLService.cpp:226
#38	0x1260be12 in nsXBLStreamListener::Load at nsXBLService.cpp:480
#39	0x124544f9 in DispatchToInterface at nsEventListenerManager.cpp:175
#40	0x12455cf8 in nsEventListenerManager::HandleEventInternal at nsEventListenerManager.cpp:1177
#41	0x124872aa in nsEventListenerManager::HandleEvent at nsEventListenerManager.h:144
#42	0x1248746d in nsEventTargetChainItem::HandleEvent at nsEventDispatcher.cpp:212
#43	0x124855ac in nsEventTargetChainItem::HandleEventTargetChain at nsEventDispatcher.cpp:341
#44	0x124864ba in nsEventDispatcher::Dispatch at nsEventDispatcher.cpp:628
#45	0x12564ba7 in nsXMLDocument::EndLoad at nsXMLDocument.cpp:581
#46	0x1255fb61 in nsXMLContentSink::DidBuildModel at nsXMLContentSink.cpp:373
#47	0x14c88c62 in nsParser::DidBuildModel at nsParser.cpp:1589
#48	0x14c8b98c in nsParser::ResumeParse at nsParser.cpp:2368
#49	0x14c88599 in nsParser::OnStopRequest at nsParser.cpp:3001
#50	0x1260a5b1 in nsXBLStreamListener::OnStopRequest at nsXBLService.cpp:377
#51	0x1420a585 in nsBaseChannel::OnStopRequest at nsBaseChannel.cpp:680
#52	0x1421e6fc in nsInputStreamPump::OnStateStop at nsInputStreamPump.cpp:578
#53	0x1421e826 in nsInputStreamPump::OnInputStreamReady at nsInputStreamPump.cpp:403
#54	0x00439b40 in nsInputStreamReadyEvent::Run at nsStreamUtils.cpp:112
#55	0x0046f076 in nsThread::ProcessNextEvent at nsThread.cpp:527
#56	0x003f04cc in NS_ProcessNextEvent_P at nsThreadUtils.cpp:250
#57	0x004701fe in nsThread::Shutdown at nsThread.cpp:468
#58	0x0048ef80 in NS_InvokeByIndex_P at xptcinvoke_unixish_x86.cpp:179
#59	0x0047b2ae in nsProxyObjectCallInfo::Run at nsProxyEvent.cpp:181
#60	0x0046f076 in nsThread::ProcessNextEvent at nsThread.cpp:527
#61	0x003f0601 in NS_ProcessPendingEvents_P at nsThreadUtils.cpp:200
#62	0x14fbd541 in nsBaseAppShell::NativeEventCallback at nsBaseAppShell.cpp:126
#63	0x14f6855b in nsAppShell::ProcessGeckoEvents at nsAppShell.mm:394
#64	0x9416115b in __CFRunLoopDoSources0
#65	0x9415ec1f in __CFRunLoopRun
#66	0x9415e0f4 in CFRunLoopRunSpecific
#67	0x9415df21 in CFRunLoopRunInMode
#68	0x95c0f0fc in RunCurrentEventLoopInMode
#69	0x95c0eeb1 in ReceiveNextEventCommon
#70	0x95c0ed36 in BlockUntilNextEventMatchingListInMode
#71	0x94738135 in _DPSNextEvent
#72	0x94737976 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
#73	0x946f9bef in -[NSApplication run]
#74	0x14f672ad in nsAppShell::Run at nsAppShell.mm:747
#75	0x15c0fe32 in nsAppStartup::Run at nsAppStartup.cpp:184
#76	0x0001258d in XRE_main at nsAppRunner.cpp:3564
#77	0x00002629 in main at nsBrowserApp.cpp:158
#78	0x000022aa in start

I'm not sure what's going wrong layout-wise, but shouldn't we refrain from reloading a URI for retrieving XBL bindings if the bound element is inside the same document?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> shouldn't we refrain from reloading a URI for retrieving XBL bindings if the
> bound element is inside the same document?

No. Why would we?

Comment 7

[(no longer active)]

Attachment: WIP

This patch, applied on top of my patch in bug 336104, fixes all of the assertions.  I ran some of the tests which use contenteditable locally with this patch applied, and it seems that we're not breaking anything here.  I've pushed this to try to make sure that nothing breaks with this.

That said, I don't particularly like this patch, because it breaks the symmetry between binding to the tree and unbinding from it.  Specifically, it will probably break things if UnbindFromTree is called after BindToTree before we've had a chance to run the thread event loop.  I don't know if that's possible to happen, so this might not be an issue, but I don't know how to make the UnbindFromTree operation async as well, because we won't have a document around by the time we reach the event handler.

Asking for feedback from Boris on this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 444782 [details] [diff] [review]
WIP

I don't think this will do the right thing... Consider an HTML element that is bound then unbound within a single update.  It will decrement the count in unbind, then have no document in bind and fail to increment the count.  So the count will end up one lower than it used to be even though the DOM is the same.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

Right.  I think that this patch addresses this issue.  Here, I look at the content editable count inside the runnable code, to protect against increments and decrements happening before we hit the event loop.

The scope of this patch is a bit larger than the previous one, but I think it should be safe.  I've pushed a try server patch as well to see how it goes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 444995 [details] [diff] [review]
Patch (v1)

This seems reasonable to me.  Might be good to have Peter review, though.

Comment 11

[(no longer active)]

Comment on attachment 444995 [details] [diff] [review]
Patch (v1)

This patch doesn't seem to have any adverse effects, as far as my investigations showed.  Requesting review from Peter.

Comment 12

[Damon Sicore (:damons)]

JST should review this as Peterv is out for a couple of weeks.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 444995 [details] [diff] [review]
Patch (v1)

Looks good, r=jst

Comment 14

[Damon Sicore (:damons)]

Blocking 1.9.3 final as it's an sg:crit.

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/855b42fbc47e

I've also marked this to depend on bug 336104 which fixes the rest of the assertions reported here.

Jesse, should we take this for branches as well?

Comment 16

[Jesse Ruderman]

The branch drivers would probably like that.  How high do you estimate the regression risk to be?  (We'll also see whether we find regressions on trunk in the next few days.)

Comment 17

[(no longer active)]

This patch basically changes the way we turn contenteditable on for all elements.  We have quite a bit of tests in our test suite which trigger this code, but I'm not sure if that catches all the possible interactions which might trigger this code, and therefore, I'm hesitant to suggest that it will be regression free.  I think the extent of possible regresssions is acceptable for nightlies, but I personally think that we should revisit this after some time for branch consideration, and in the meanwhile, watch trunk for contenteditable regressions.

Of course, content gurus might actually prove me wrong here.  jst, bz, anyone?

Comment 18

[(no longer active)]

Attachment: Assertion test

Comment 19

[(no longer active)]

Test case landed as http://hg.mozilla.org/mozilla-central/rev/ad511ffa0492.

Comment 20

[Daniel Veditz [:dveditz]]

What branches is this needed on? I get different assertions, but I don't know if that just means we hadn't added the assertions yet or if it's not triggering the bug. I thought we've had the "This is unsafe" assertion for a while, though.

In a 1.9.1.x build I get
###!!! ASSERTION: no frame, see bug #188946: 'frame', file /Users/daniel/dev/ffcentral/moz191/editor/libeditor/base/nsEditor.cpp, line 4143
WARNING: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file /Users/daniel/dev/ffcentral/moz191/layout/base/nsPresContext.h, line 1026
###!!! ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()', file /Users/daniel/dev/ffcentral/moz191/content/base/src/nsContentUtils.cpp, line 4776
###!!! ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()', file /Users/daniel/dev/ffcentral/moz191/content/base/src/nsContentUtils.cpp, line 4776

And pretty much the same on 1.9.2.x except "WARNING: recurring into frame construction" is turned into an ASSERTION (which, come to think of it, is a bad sign).

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to comment #19)
> Test case landed as http://hg.mozilla.org/mozilla-central/rev/ad511ffa0492.

When we think these affect the stable branches please don't check in testcases that reveal the bug until after it's fixed on the branches. Instead set the in-testsuite flag to '?' as a reminder to do it later.

Comment 22

[(no longer active)]

(In reply to comment #20)
> What branches is this needed on?

All of them.  The dangerous part is really recurring into frame construction.  So, we should take this on all branches.  Now that this patch has baked on trunk for some time without any regressions, I guess we can take this in the next dot-release.

(In reply to comment #21)
> (In reply to comment #19)
> > Test case landed as http://hg.mozilla.org/mozilla-central/rev/ad511ffa0492.
> 
> When we think these affect the stable branches please don't check in testcases
> that reveal the bug until after it's fixed on the branches. Instead set the
> in-testsuite flag to '?' as a reminder to do it later.

Oh, sorry about that.  This has always confused me, but I'll remmeber it for the next time.

Comment 23

[LegNeato]

We can't approve this yet as bug 336104 isn't nominated. It looks like they both need to come in together.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 444995 [details] [diff] [review]
Patch (v1)

Approved for 1.9.2.9 and 1.9.1.12, a=dveditz for release-drivers

Comment 25

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ac83b3496c63
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/60472f1c0de9
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7d222e4557a5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f4865914ac56