623484 - nsMsgContentPolicy::ShouldAcceptContentForPotentialMsg should not call out-of-module synchronously (to nsIMsgHeaderSink::OnMsgHasRemoteContent) [Want to fire mutation events, but it's not safe]

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

I am seeing an angry assertion:
###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file /home/visbrero/rev_control/hg/comm-central/mozilla/content/base/src/nsContentUtils.cpp, line 3625

While the direct causation is protz's monkeypatch for window.messageHeaderSink.onMsgHasRemoteContent in Thunderbird Conversations, the root cause is that nsMsgContentPolicy::ShouldAcceptContentForPotentialMsg is invoking nsIMsgHeaderSink::OnMsgHasRemoteContent synchronously while there is all kinds of serious stuff on the stack.

The correct behaviour is for us to post a runnable to trigger the notification.

It's worth noting that even without protz's extension, the code Thunderbird code does appear to get up to some DOM mutation.

The stack looks like this:
  0 RealBreak
  1 Break
  2 NS_DebugBreak_P
  3 nsContentUtils::HasMutationListeners
  4 nsGenericElement::SetAttr
  5 nsGenericElement::SetAttribute
  6 nsXULElement::SetAttribute
  7 nsIDOMElement_SetAttribute
  8 js::CallJSNative
  9 js::Interpret
 10 js::RunScript
 11 js::Invoke
 12 js::ExternalInvoke
 13 js::ExternalInvoke
 14 js::ExternalGetOrSet
 15 js::Shape::set
 16 js_SetPropertyHelper
 17 js::Interpret
 18 js::RunScript
 19 js::Invoke
 20 js::ExternalInvoke
 21 js::ExternalInvoke
 22 JS_CallFunctionValue
 23 nsXPCWrappedJSClass::CallMethod
 24 nsXPCWrappedJS::CallMethod
 25 PrepareAndDispatch
 26 SharedStub
 27 nsMsgContentPolicy::ShouldAcceptContentForPotentialMsg
 28 nsMsgContentPolicy::ShouldLoad
 29 nsContentPolicy::CheckPolicy
 30 nsContentPolicy::ShouldLoad
 31 NS_CheckContentLoadPolicy
 32 nsContentUtils::CanLoadImage
 33 nsCSSValue::Image::Image
 34 nsGenericHTMLElement::MapBackgroundInto
 35 nsGenericHTMLElement::MapBackgroundAttributesInto
 36 MapAttributesIntoRule
 37 nsMappedAttributes::MapRuleInfoInto
 38 nsRuleNode::WalkRuleTree
 39 nsRuleNode::GetBackgroundData
 40 nsRuleNode::GetStyleBackground
 41 nsStyleContext::DoGetStyleBackground
 42 nsStyleContext::GetStyleBackground
 43 nsCSSFrameConstructor::ConstructFramesFromItem
 44 nsCSSFrameConstructor::ConstructFramesFromItemList
 45 nsCSSFrameConstructor::ProcessChildren
 46 nsCSSFrameConstructor::ConstructTableRow
 47 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 48 nsCSSFrameConstructor::ConstructFramesFromItem
 49 nsCSSFrameConstructor::ConstructFramesFromItemList
 50 nsCSSFrameConstructor::ProcessChildren
 51 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 52 nsCSSFrameConstructor::ConstructFramesFromItem
 53 nsCSSFrameConstructor::ConstructFramesFromItemList
 54 nsCSSFrameConstructor::ProcessChildren
 55 nsCSSFrameConstructor::ConstructTable
 56 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 57 nsCSSFrameConstructor::ConstructFramesFromItem
 58 nsCSSFrameConstructor::ConstructFramesFromItemList
 59 nsCSSFrameConstructor::ProcessChildren
 60 nsCSSFrameConstructor::ConstructTableCell
 61 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 62 nsCSSFrameConstructor::ConstructFramesFromItem
 63 nsCSSFrameConstructor::ConstructFramesFromItemList
 64 nsCSSFrameConstructor::ProcessChildren
 65 nsCSSFrameConstructor::ConstructTableRow
 66 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 67 nsCSSFrameConstructor::ConstructFramesFromItem
 68 nsCSSFrameConstructor::ConstructFramesFromItemList
 69 nsCSSFrameConstructor::ProcessChildren
 70 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 71 nsCSSFrameConstructor::ConstructFramesFromItem
 72 nsCSSFrameConstructor::ConstructFramesFromItemList
 73 nsCSSFrameConstructor::ProcessChildren
 74 nsCSSFrameConstructor::ConstructTable
 75 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 76 nsCSSFrameConstructor::ConstructFramesFromItem
 77 nsCSSFrameConstructor::ConstructFramesFromItemList
 78 nsCSSFrameConstructor::ProcessChildren
 79 nsCSSFrameConstructor::ConstructBlock
 80 nsCSSFrameConstructor::ConstructNonScrollableBlock
 81 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 82 nsCSSFrameConstructor::ConstructFramesFromItem
 83 nsCSSFrameConstructor::ConstructFramesFromItemList
 84 nsCSSFrameConstructor::ProcessChildren
 85 nsCSSFrameConstructor::ConstructBlock
 86 nsCSSFrameConstructor::ConstructNonScrollableBlock
 87 nsCSSFrameConstructor::ConstructFrameFromItemInternal
 88 nsCSSFrameConstructor::ConstructFramesFromItem
 89 nsCSSFrameConstructor::ConstructFramesFromItemList
 90 nsCSSFrameConstructor::ProcessChildren
 91 nsCSSFrameConstructor::ConstructBlock
 92 nsCSSFrameConstructor::ConstructDocElementFrame
 93 nsCSSFrameConstructor::ContentRangeInserted
 94 nsCSSFrameConstructor::ContentInserted
 95 PresShell::InitialReflow
 96 nsContentSink::StartLayout
 97 nsContentSink::StyleSheetLoaded
 98 mozilla::css::Loader::SheetComplete
 99 mozilla::css::Loader::ParseSheet
100 mozilla::css::SheetLoadData::OnStreamComplete
101 nsUnicharStreamLoader::OnStopRequest
102 nsBaseChannel::OnStopRequest
103 nsInputStreamPump::OnStateStop
104 nsInputStreamPump::OnInputStreamReady
105 nsInputStreamReadyEvent::Run
106 nsThread::ProcessNextEvent
107 NS_ProcessNextEvent_P
108 nsBaseAppShell::Run
109 nsAppStartup::Run
110 XRE_main
111 main

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Whoops, my bad, although protz's extension is on the stack, it is in fact the core Thunderbird code that's up to the badness actually:

(gdb) call DumpJSStack()
0 set_value(val = "Always load remote content from ELIDED") ["chrome://global/content/bindings/text.xml":0]
    this = [object XULElement @ 0x7fffdf0f0a20 (native @ 0x7fffe4d52c00)]
1 anonymous(aMsgHdr = [xpconnect wrapped nsIMsgDBHdr @ 0x7fffc8a81320 (native @ 0x7fffc64fa270)]) ["chrome://messenger/content/mailWindowOverlay.js":2559]
    emailAddress = "ELIDED"
    headerParser = [xpconnect wrapped nsIMsgHeaderParser @ 0x7fffc34fda50 (native @ 0x7fffe4c51160)]
    this = [object Object]
2 anonymous(aMsgHdr = [xpconnect wrapped nsIMsgDBHdr @ 0x7fffc8a81320 (native @ 0x7fffc64fa270)]) ["chrome://messenger/content/msgHdrViewOverlay.js":654]
    this = [object ChromeWindow @ 0x7fffe6369940 (native @ 0x7fffe6347c78)]
3 _onMsgHasRemoteContent_patched(aMsgHdr = [xpconnect wrapped nsIMsgDBHdr @ 0x7fffc8a81320 (native @ 0x7fffc64fa270)]) ["resource://conversations/monkeypatch.js":649]
    messageId = "ELIDED"
    msgListeners = [object Object]
    this = [object Object]

Comment 2

[David :Bienvenu]

Attachment: proposed fix

hoping to get rid of some assertions...

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 524339 [details] [diff] [review]
proposed fix

Hmm, it may be worth running the content-policy parts of the mozmill tests a few times and check we haven't broken them or introducing a random orange.

iirc we expect the remote content button to be available once message loading is complete. I'm not sure that it would still be guaranteed after this patch.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 524339 [details] [diff] [review]
proposed fix

woo! It will be awesome to have this gone.

Standard8 is right, this could change some timing and should ideally get some try server runs.  If that proves to be the case, adding a controller.sleep(0) after any more clever event-driven approach on the URI may be the ideal solution since it guarantees that the event, if it exists, will have run.

Comment 5

[David :Bienvenu]

yeah, I was a bit worried about that but the tests passed here. I'll do a try server run.

Comment 6

[David :Bienvenu]

try server builds will be here - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-34d1a8166bb9

Comment 7

[David :Bienvenu]

try server mozmill tests seem to have passed...I'll try the tinderbox mozmill tests next, i.e., I'll land this.

Comment 8

[David :Bienvenu]

changeset - http://hg.mozilla.org/comm-central/rev/0c36ce76850c