1149891 - (CVE-2015-2731) crash in CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) from userscript

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[her34her34]

Attachment: Crash.Default.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

Test profile attached.

Install profile.

On bookmark toolbar there is one saved bookmark "firefox crash". Click bookmark.


Actual results:

Firefox will crash as page is loading. If page finishes loading then click bookmark again. Crash usually happens by 3rd try.


Expected results:

Firefox should not crash. Previous versions of firefox do not crash.

Comment 1

[Loic]

No need to provide your profile.

Just give us better steps to reproduce. Which page did you bookmark to reproduce the crash?

Comment 2

[Marcia Knous [:marcia]]

We also need the crash report ID - if there is a crash listed in about:crashes, please paste it here. Thanks!

Comment 3

[her34her34]

Attachment: [e-mail]

Comment 4

[her34her34]

1)create new profile
2)install greasemonkey
3)install userscript [e-mail] 
4)visit url: http://www.freewebarcade.com/game/the-power-of-love/
5)firefox should crash while loading page
6)if page finishes loading, reload page. Repeat reloading until crash.

Comment 5

[Loic]

Confirmed.

CR with FF40:
https://crash-stats.mozilla.com/report/index/44f5f100-163e-4b2e-bb46-b01f52150406

Comment 6

[Xu Zhen]

Attachment: test.html

NoScript、Ghostery can also crash firefox if you open test.html

https://crash-stats.mozilla.com/report/index/7e994356-2556-443b-9b72-b1ede2150411

Comment 7

[Loic]

Xu, thank for the testcase, it crashes FF even without any add-on.

I got a various crash signature with the latest Nightly:
https://crash-stats.mozilla.com/report/index/da13c11f-03cd-43d8-8a08-5696d2150411

Regression range:
good=2014-03-25
bad=2014-03-26
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c0673441fc8&tochange=140ac04d7454

Comment 8

[Alice0775 White]

Using testcase of Comment 6, I get a different regression window:

Firefox35.0.1 do not crash.
bp-af6ce1cb-dc81-4a80-9ce5-504b42150411 on Firefox36.0.4
bp-9a10573c-8741-46c1-b8cb-49a282150411 on Firefox37.0.1
bp-f01f73ea-1e11-490d-8d3a-60c372150411 on Beta38.0b3
bp-85497c28-140a-493c-a57b-321772150411 on Aurora39.0a2
bp-bde93daf-b9dd-4d34-9532-b839d2150411 on Nightly39.0a1 without e10s
bp-6037afd1-f2be-456a-991f-0e0972150411 on Nightly39.0a1 with e10s tab crashed

Pushlog: 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=70a6139f5517&tochange=044d2a98a497

triggered by: Bug 1081038

Comment 9

[xunxun]

In fact, even if no NoScript or Ghostery, run testcase of Comment 6 twice, also can get a crash.

Comment 10

[Loic]

(In reply to Alice0775 White from comment #8)
> Using testcase of Comment 6, I get a different regression window:


Ok, I got the crash history of this bug. A crash in the past has been fixed, and now, it regressed a 2nd time.

1st Regression:
good1=2014-03-25
bad1=2014-03-26
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c0673441fc8&tochange=140ac04d7454

This regression has been fixed in the past:
bad1=2014-08-22
good2=2014-08-23
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b9dd32d1e16&tochange=64c4ec2df3d4

Then it regressed again (this current bug). 2nd regression:
good2=2014-11-14
bad2=2014-11-15
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7f0d92595432&tochange=acbd7b68fa8c
with Bug 1081038 in the range, like you.

Comment 11

[Gabor Krizsanits (INACTIVE)]

I don't think this crash is related to the microtask work I did in the mentioned patch. Why do you guys think that it was the reason for the crash? I see patches in the range those changing code in LoadInfo for example... Anyway, someone should debug this and see what's going wrong. I don't think I will have the time for it this week.

Comment 12

[Alice0775 White]

[Tracking Requested - why for this release]: crash


I got a same range

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=70a6139f5517&tochange=044d2a98a497

This crash is definitely caused by Bug 1081038.

Comment 13

[Alice0775 White]

bp-205c1664-0779-4ec8-a03d-c5def2150503

Comment 14

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 39+ since this is a relatively recent regression - from Firefox 36. 

Christoph, is this something you might have time to look at?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Mhm, nothing obvious goes wrong here, but I can reproduce on Linux in e10s and also non e10s mode by only running the provided testcase from Comment 6. Please find a stacktrace underneath. Interestingly, it doesn't fail within nsCSPService::ShouldLoad() but in nsContentPolicy::ShouldLoad(), but all the arguments seem to be correct from a first glance.

I am not sure at this time, but maybe Bill can help, because shouldLoad is enforcing a "simple-content-policy", which was introduced with bug 1128798 and landed in 38. Given that 37 is also affected (see Comment 12) most likely indicates that Bug 1128798 is not directly related.

Bill, what do you think?


#6  0x00007fffefe529b1 in nsContentPolicy::ShouldLoad (this=0x7fffd9974600, contentType=5, contentLocation=0x7fffd8fc6f00, requestingLocation=0x7fffd99fb000, 
    requestingContext=0x7fffe4bc0800, mimeType=..., extra=0x0, requestPrincipal=0x7fffdb671c80, decision=0x7fffffffb2bc)
    at /home/ckerschb/moz/mc/dom/base/nsContentPolicy.cpp:243
#7  0x00007fffef7f9bd4 in NS_CheckContentLoadPolicy (contentType=5, contentLocation=0x7fffd8fc6f00, originPrincipal=0x7fffdb671c80, context=0x7fffe4bc0800, mimeType=..., 
    extra=0x0, decision=0x7fffffffb2bc, policyService=0x7fffd9974600, aSecMan=0x7fffe26c7290) at ../../dist/include/nsContentPolicyUtils.h:218
#8  0x00007fffefd38f65 in nsObjectLoadingContent::CheckLoadPolicy (this=0x7fffe4bc0890, aContentPolicy=0x7fffffffb2bc)
    at /home/ckerschb/moz/mc/dom/base/nsObjectLoadingContent.cpp:1487
#9  0x00007fffefd35935 in nsObjectLoadingContent::LoadObject (this=0x7fffe4bc0890, aNotify=true, aForceLoad=false, aLoadingChannel=0x0)
    at /home/ckerschb/moz/mc/dom/base/nsObjectLoadingContent.cpp:2131
#10 0x00007fffefd352b3 in nsObjectLoadingContent::LoadObject (this=0x7fffe4bc0890, aNotify=true, aForceLoad=false)
    at /home/ckerschb/moz/mc/dom/base/nsObjectLoadingContent.cpp:2003
#11 0x00007ffff11bca55 in mozilla::dom::HTMLSharedObjectElement::StartObjectLoad (this=0x7fffe4bc0800, aNotify=true)
    at /home/ckerschb/moz/mc/dom/html/HTMLSharedObjectElement.cpp:331
#12 0x00007ffff11d454a in mozilla::dom::HTMLSharedObjectElement::StartObjectLoad (this=0x7fffe4bc0800) at ../../dist/include/mozilla/dom/HTMLSharedObjectElement.h:83
#13 0x00007ffff11d8b23 in nsRunnableMethodArguments<>::apply<mozilla::dom::HTMLSharedObjectElement, void (mozilla::dom::HTMLSharedObjectElement::*)()>(mozilla::dom::HTMLSharedObjectElement*, void (mozilla::dom::HTMLSharedObjectElement::*)()) (this=0x7fffd8facb30, o=0x7fffe4bc0800, 
    m=(void (mozilla::dom::HTMLSharedObjectElement::*)(mozilla::dom::HTMLSharedObjectElement * const)) 0x7ffff11d4530 <mozilla::dom::HTMLSharedObjectElement::StartObjectLoad()>) at ../../dist/include/nsThreadUtils.h:602
#14 0x00007ffff11d895c in nsRunnableMethodImpl<void (mozilla::dom::HTMLSharedObjectElement::*)(), true>::Run (this=0x7fffd8facb00)
    at ../../dist/include/nsThreadUtils.h:809
#15 0x00007fffefc76cce in nsContentUtils::RemoveScriptBlocker () at /home/ckerschb/moz/mc/dom/base/nsContentUtils.cpp:5087
#16 0x00007fffefeb1d50 in nsDocument::EndUpdate (this=0x7fffdd84e000, aUpdateType=1) at /home/ckerschb/moz/mc/dom/base/nsDocument.cpp:5004
#17 0x00007ffff1229019 in nsHTMLDocument::EndUpdate (this=0x7fffdd84e000, aUpdateType=1) at /home/ckerschb/moz/mc/dom/html/nsHTMLDocument.cpp:2537
#18 0x00007fffef82e3c0 in nsHtml5DocumentBuilder::EndDocUpdate (this=0x7fffd8f0d000) at /home/ckerschb/moz/mc/parser/html/nsHtml5DocumentBuilder.h:81
#19 0x00007fffef859ba2 in nsHtml5TreeOpExecutor::DidBuildModel (this=0x7fffd8f0d000, aTerminated=false) at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:120
#20 0x00007fffef87b591 in nsHtml5TreeOperation::Perform (this=0x7fffd8f0fe78, aBuilder=0x7fffd8f0d000, aScriptElement=0x7fffffffbef0)
    at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOperation.cpp:819
#21 0x00007fffef85a746 in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x7fffd8f0d000) at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:448
#22 0x00007fffef877b95 in nsHtml5ExecutorFlusher::Run (this=0x7fffd99ffc80) at /home/ckerschb/moz/mc/parser/html/nsHtml5StreamParser.cpp:127
#23 0x00007fffee689638 in nsThread::ProcessNextEvent (this=0x7fffe4bc5400, aMayWait=false, aResult=0x7fffffffc17e) at /home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:868
#24 0x00007fffee6dbfa7 in NS_ProcessNextEvent (aThread=0x7fffe4bc5400, aMayWait=false) at /home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:265
#25 0x00007fffeeccedeb in mozilla::ipc::MessagePump::Run (this=0x7fffe4b51e70, aDelegate=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95
#26 0x00007fffeeccfc9b in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x7fffe4b51e70, aDelegate=0x7fffffffc5b8)
    at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:289
#27 0x00007fffeec557c5 in MessageLoop::RunInternal (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:233
#28 0x00007fffeec556f5 in MessageLoop::RunHandler (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:226
#29 0x00007fffeec556cd in MessageLoop::Run (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:200
#30 0x00007ffff1c56613 in nsBaseAppShell::Run (this=0x7fffde368800) at /home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:165
#31 0x00007ffff2c1b763 in XRE_RunAppShell () at /home/ckerschb/moz/mc/toolkit/xre/nsEmbedFunctions.cpp:738
#32 0x00007fffeeccfab6 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x7fffe4b51e70, aDelegate=0x7fffffffc5b8)
    at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:259
#33 0x00007fffeec557c5 in MessageLoop::RunInternal (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:233
#34 0x00007fffeec556f5 in MessageLoop::RunHandler (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:226
---Type <return> to continue, or q <return> to quit---
#35 0x00007fffeec556cd in MessageLoop::Run (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:200
#36 0x00007ffff2c1afe2 in XRE_InitChildProcess (aArgc=3, aArgv=0x7fffffffdb98, aGMPLoader=0x0) at /home/ckerschb/moz/mc/toolkit/xre/nsEmbedFunctions.cpp:575
#37 0x0000000000417850 in content_process_main (argc=5, argv=0x7fffffffdb98) at /home/ckerschb/moz/mc/ipc/app/../contentproc/plugin-container.cpp:236
#38 0x0000000000417942 in main (argc=6, argv=0x7fffffffdb98) at /home/ckerschb/moz/mc/ipc/app/MozillaRuntimeMain.cpp:11

Comment 17

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

Well, the problem seems to be that nsObjectLoadingContent::mURI has been freed and now we're using it. If you look at the Greasemonkey script, it installs a mutation observer that immediately removes any <object> element right after it's added to the DOM. It sounds like we have a UAF on some object related to nsObjectLoadingContent.

I guess the question is whether we're running the mutation observer when we shouldn't be (which could be caused by Gabor's bug, since it fiddles with microtasks, and those sound like they might be able to run mutation observers), OR whether we just have a pre-existing UAF in plugin code that was exposed by Gabor's bug.

I'm going to close this bug for now just to be safe. It sounds pretty exploitable.

I'm not sure who should look into this. Maybe Aaron or Bobby will have some ideas.

Comment 18

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

This is definitely a regression from Bug 1081038.

Before that when parser created DOM, MutationObserver was notified at the end of the current task, as expected.
Now it happens whenever there happens to be a JS call, even a C++ to chrome JS one.
And nsIContentPolicy implementations aren't supposed to
modify DOM, but this surprising MutationObserver callback does that now.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicy.idl?rev=9ba87714f5cf&mark=64-79#64


#2  0x00007ffff0389044 in nsObjectLoadingContent::UnloadObject (this=0x7fffcf3b33d0, aResetState=true) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:2584
#3  0x00007ffff0388e0b in nsObjectLoadingContent::UnbindFromTree (this=0x7fffcf3b33d0, aDeep=true, aNullParent=true) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:693
#4  0x00007ffff181a657 in mozilla::dom::HTMLSharedObjectElement::UnbindFromTree (this=0x7fffcf3b3340, aDeep=true, aNullParent=true) at /home/smaug/mozilla/hg/inbound2/dom/html/HTMLSharedObjectElement.cpp:173
#5  0x00007ffff05aabbc in nsINode::doRemoveChildAt (this=0x7fffc3ee9d80, aIndex=1, aNotify=true, aKid=0x7fffcf3b3340, aChildArray=...) at /home/smaug/mozilla/hg/inbound2/dom/base/nsINode.cpp:1643
#6  0x00007ffff0409245 in mozilla::dom::FragmentOrElement::RemoveChildAt (this=0x7fffc3ee9d80, aIndex=1, aNotify=true) at /home/smaug/mozilla/hg/inbound2/dom/base/FragmentOrElement.cpp:1151
#7  0x00007ffff05a72c6 in nsINode::RemoveChild (this=0x7fffc3ee9d80, aOldChild=..., aError=...) at /home/smaug/mozilla/hg/inbound2/dom/base/nsINode.cpp:537
#8  0x00007ffff09d229b in mozilla::dom::NodeBinding::removeChild (cx=0x7fffd18e3e30, obj=..., self=0x7fffc3ee9d80, args=...) at ./NodeBinding.cpp:732
#9  0x00007ffff1477024 in mozilla::dom::GenericBindingMethod (cx=0x7fffd18e3e30, argc=1, vp=0x7fffe0d09270) at /home/smaug/mozilla/hg/inbound2/dom/bindings/BindingUtils.cpp:2609
#10 0x00007ffff4308d3b in js::CallJSNative (cx=0x7fffd18e3e30, native=0x7ffff1476d90 <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/smaug/mozilla/hg/inbound2/js/src/jscntxtinlines.h:235
#11 0x00007ffff429f69f in js::Invoke (cx=0x7fffd18e3e30, args=..., construct=js::NO_CONSTRUCT) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:727
#12 0x00007ffff42ba7cf in Interpret (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:2955
#13 0x00007ffff42acece in js::RunScript (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:677
#14 0x00007ffff429f7bc in js::Invoke (cx=0x7fffd18e3e30, args=..., construct=js::NO_CONSTRUCT) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:746
#15 0x00007ffff42ba7cf in Interpret (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:2955
#16 0x00007ffff42acece in js::RunScript (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:677
#17 0x00007ffff429f7bc in js::Invoke (cx=0x7fffd18e3e30, args=..., construct=js::NO_CONSTRUCT) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:746
#18 0x00007ffff4287724 in js::Invoke (cx=0x7fffd18e3e30, thisv=..., fval=..., argc=2, argv=0x7fffffff9620, rval=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:783
#19 0x00007ffff4978a46 in JS::Call (cx=0x7fffd18e3e30, thisv=..., fval=..., args=..., rval=...) at /home/smaug/mozilla/hg/inbound2/js/src/jsapi.cpp:4401
#20 0x00007ffff0990417 in mozilla::dom::MutationCallback::Call (this=0x7fffcc019850, cx=0x7fffd18e3e30, aThisVal=..., mutations=..., observer=..., aRv=...) at ./MutationObserverBinding.cpp:617
#21 0x00007ffff0532b5b in mozilla::dom::MutationCallback::Call<nsDOMMutationObserver*> (this=0x7fffcc019850, thisVal=@0x7fffffff9948: 0x7fffd56601b0, mutations=..., observer=..., aRv=..., 
    aExecutionReason=0x7ffff4ebe972 <.L.str970> "MutationCallback", aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aCompartment=0x0)
    at ../../dist/include/mozilla/dom/MutationObserverBinding.h:178
#22 0x00007ffff04eb76f in nsDOMMutationObserver::HandleMutation (this=0x7fffd56601b0) at /home/smaug/mozilla/hg/inbound2/dom/base/nsDOMMutationObserver.cpp:778
#23 0x00007ffff04eb93a in nsDOMMutationObserver::HandleMutationsInternal () at /home/smaug/mozilla/hg/inbound2/dom/base/nsDOMMutationObserver.cpp:818
#24 0x00007ffff02de23a in nsDOMMutationObserver::HandleMutations () at /home/smaug/mozilla/hg/inbound2/dom/base/nsDOMMutationObserver.h:527
#25 0x00007ffff02ce07f in nsContentUtils::PerformMainThreadMicroTaskCheckpoint () at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentUtils.cpp:5182
#26 0x00007ffff02ce00b in nsContentUtils::LeaveMicroTask () at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentUtils.cpp:5151
#27 0x00007ffff0452a9d in mozilla::dom::AutoEntryScript::~AutoEntryScript (this=0x7fffffffa508) at /home/smaug/mozilla/hg/inbound2/dom/base/ScriptSettings.cpp:561
#28 0x00007fffefb3a05e in nsXPCWrappedJSClass::CallMethod (this=0x7fffd03e00b0, wrapper=0x7fffd2e62e00, methodIndex=7, info_=0x7fffe304a5f8, nativeParams=0x7fffffffa750)
    at /home/smaug/mozilla/hg/inbound2/js/xpconnect/src/XPCWrappedJSClass.cpp:1423
#29 0x00007fffefb3767b in nsXPCWrappedJS::CallMethod (this=0x7fffd2e62e00, methodIndex=7, info=0x7fffe304a5f8, params=0x7fffffffa750) at /home/smaug/mozilla/hg/inbound2/js/xpconnect/src/XPCWrappedJS.cpp:525
#30 0x00007fffeed04411 in PrepareAndDispatch (self=0x7fffd192cb80, methodIndex=7, args=0x7fffffffa8a0, gpregs=0x7fffffffa820, fpregs=0x7fffffffa850)
    at /home/smaug/mozilla/hg/inbound2/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:122
#31 0x00007fffeed0340b in SharedStub () from /home/smaug/mozilla/hg/inbound2/ff_build/dist/bin/libxul.so
#32 0x00007ffff31c815a in nsBrowserStatusFilter::OnSecurityChange (this=0x7fffd0388f70, aWebProgress=0x7fffd0492028, aRequest=0x0, aState=33)
    at /home/smaug/mozilla/hg/inbound2/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp:264
#33 0x00007ffff31c81ad in non-virtual thunk to nsBrowserStatusFilter::OnSecurityChange(nsIWebProgress*, nsIRequest*, unsigned int) ()
    at /home/smaug/mozilla/hg/inbound2/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp:265
#34 0x00007fffefdc9fff in nsDocLoader::OnSecurityChange (this=0x7fffd0492000, aContext=0x7fffcf3b3340, aState=33) at /home/smaug/mozilla/hg/inbound2/uriloader/base/nsDocLoader.cpp:1452
#35 0x00007ffff2dda8ae in nsDocShell::OnSecurityChange (this=0x7fffd0492000, i_Context=0x7fffcf3b3340, state=33) at /home/smaug/mozilla/hg/inbound2/docshell/base/nsDocShell.h:197
#36 0x00007ffff2ddac15 in non-virtual thunk to nsDocShell::OnSecurityChange(nsISupports*, unsigned int) () at Unified_cpp_docshell_base0.cpp:197
#37 0x00007ffff1c16a3a in nsMixedContentEvent::Run (this=0x7fffcc019eb0) at /home/smaug/mozilla/hg/inbound2/dom/security/nsMixedContentBlocker.cpp:106
#38 0x00007ffff02cc3b1 in nsContentUtils::AddScriptRunner (aRunnable=0x7fffcc019eb0) at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentUtils.cpp:5134
#39 0x00007ffff1c1171d in nsMixedContentBlocker::ShouldLoad (aHadInsecureImageRedirect=false, aContentType=5, aContentLocation=0x7fffcf16ff00, aRequestingLocation=0x7fffcb2da000, 
    aRequestingContext=0x7fffcf3b3340, aMimeGuess=..., aExtra=0x0, aRequestPrincipal=0x7fffcaf6bfc0, aDecision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/security/nsMixedContentBlocker.cpp:766
#40 0x00007ffff1c10149 in nsMixedContentBlocker::ShouldLoad (this=0x7fffd5d05380, aContentType=5, aContentLocation=0x7fffcf16ff00, aRequestingLocation=0x7fffcb2da000, aRequestingContext=0x7fffcf3b3340, 
    aMimeGuess=..., aExtra=0x0, aRequestPrincipal=0x7fffcaf6bfc0, aDecision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/security/nsMixedContentBlocker.cpp:311
#41 0x00007ffff04b6eec in nsContentPolicy::CheckPolicy (this=0x7fffd6fdf290, 
    policyMethod=&virtual nsIContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), 
    simplePolicyMethod=&virtual table offset 24, contentType=5, contentLocation=0x7fffcf16ff00, requestingLocation=0x7fffcb2da000, requestingContext=0x7fffcf3b3340, mimeType=..., extra=0x0, 
    requestPrincipal=0x7fffcaf6bfc0, decision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentPolicy.cpp:127
#42 0x00007ffff04ab668 in nsContentPolicy::ShouldLoad (this=0x7fffd6fdf290, contentType=5, contentLocation=0x7fffcf16ff00, requestingLocation=0x7fffcb2da000, requestingContext=0x7fffcf3b3340, mimeType=..., 
    extra=0x0, requestPrincipal=0x7fffcaf6bfc0, decision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentPolicy.cpp:237
#43 0x00007fffefe503a4 in NS_CheckContentLoadPolicy (contentType=5, contentLocation=0x7fffcf16ff00, originPrincipal=0x7fffcaf6bfc0, context=0x7fffcf3b3340, mimeType=..., extra=0x0, decision=0x7fffffffbb2c, 
    policyService=0x7fffd6fdf290, aSecMan=0x7fffe42809c0) at ../../dist/include/nsContentPolicyUtils.h:218
#44 0x00007ffff0390226 in nsObjectLoadingContent::CheckLoadPolicy (this=0x7fffcf3b33d0, aContentPolicy=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:1491
---Type <return> to continue, or q <return> to quit---
#45 0x00007ffff038cbb5 in nsObjectLoadingContent::LoadObject (this=0x7fffcf3b33d0, aNotify=true, aForceLoad=false, aLoadingChannel=0x0)
    at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:2138
#46 0x00007ffff038c533 in nsObjectLoadingContent::LoadObject (this=0x7fffcf3b33d0, aNotify=true, aForceLoad=false) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:2010

Comment 19

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

gabor, bholley, would you have time to look at this? We effectively don't want to pretend
random xpcom callback/js call as a microtask entry, but limit microtask for web phasing APIs
(good thing here is that everything should be just webidl callbacks, so things will just work).
So, back out bug 1081038, and fix that by making devtools to explicitly enter/leave microtask whenever
executing scripts.

If you don't have time, I'll take this. This is very much critical.

Comment 20

[Gabor Krizsanits (INACTIVE)]

If you could take this that would be great, I just don't have a deep understanding of this area, so I would prefer someone else fixing this to be honest.

Comment 21

[Bobby Holley (:bholley)]

I'm busy with other things, sorry. :-(

Comment 22

[her34her34]

Why is it taking so long to fix a known reproducible crash? The crash is now in 2 versions of firefox release channel and also in ESR. If fixing crashes is such a low priority then mozilla should at least revert the patch that caused this in the first place.

Comment 23

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

sorry, I'll write the patch this week, which includes reverting the patch which caused this.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

(In reply to her34her34 from comment #22)
> Why is it taking so long to fix a known reproducible crash? The crash is now
> in 2 versions of firefox release channel and also in ESR. If fixing crashes
> is such a low priority then mozilla should at least revert the patch that
> caused this in the first place.

Sometimes patches take a little while to fix. Comments like this aren't really helpful to the process. Gently asking developers about it if it is taking too long is more appropriate but patches can take a month or two, easily.

Comment 25

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

Attachment: v1

enter microtask only in case of web/dom related script execution.
MessageManager is considered DOM related, and the worker stuff is very bogus anyway, and not considered "DOM" in this case.
(of some worker runnable might run scripts, it should explicitly use AutoEntryScript. The current setup where all the WorkerRunnables have that is wrong, at least from microtask point of view.)

This regresses bug 1081038. That is something devtools need to deal with explicitly, but I think regressing that case is totally fine, since web components aren't enabled by default. The issue is that devtools use
JS APIs to evaluate scripts, and those know nothing about microtasks.

Comment 26

[Bobby Holley (:bholley)]

Hm, this patch doesn't give any indication of what value the caller should pass. When do we want to enter a microtask, exactly? Can we detect that and do it automatically? Does it depend on whether the target code is chrome or not? Or maybe on whether it's running in a DOM Window?

I'm not happy about adding an extra argument here, especially one without clear guidance of what the caller should pass. I'd probably prefer to just back out bug 1081038 rather than landing this.

Comment 27

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

(In reply to Bobby Holley (:bholley) from comment #26)
> Hm, this patch doesn't give any indication of what value the caller should
> pass. When do we want to enter a microtask, exactly?
When we're doing DOM-y stuff.

> Can we detect that and
> do it automatically?
I don't know how

 Does it depend on whether the target code is chrome or
> not? Or maybe on whether it's running in a DOM Window?
well, if xpconnect stuff happens to run DOM Window code, that would be still wrong.


> 
> I'm not happy about adding an extra argument here, especially one without
> clear guidance of what the caller should pass. I'd probably prefer to just
> back out bug 1081038 rather than landing this.
I can do that too, though we'll need to do something similar to the patch in this bug eventually to have proper microtask handling.

Comment 28

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

Attachment: backout part 1

Comment 29

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

Attachment: backout part 2

Comment 30

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] (review overload. No new review requests before May 24, please) from comment #27)
> (In reply to Bobby Holley (:bholley) from comment #26)
> > Hm, this patch doesn't give any indication of what value the caller should
> > pass. When do we want to enter a microtask, exactly?
> When we're doing DOM-y stuff.

Can we define that better? What's actual thing we care about? That it's in a spec somewhere, and some spec editor is supposed to have thought about all of the ways that running script might cause problems? I don't have a great understanding of microtasks, but my gut feeling is that there needs to be a deeper model for this stuff.

Boris, do you have any thoughts?

Comment 31

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

(In reply to Bobby Holley (:bholley) from comment #30)
> Can we define that better? What's actual thing we care about? That it's in a
> spec somewhere,
It is in HTML spec.

> and some spec editor is supposed to have thought about all
> of the ways that running script might cause problems?
Of course spec authors shouldn't need to care about browser engine internal behavior.
The issue we have here is that bug 1081038 made us run microtask callbacks when there is no microtask from web point of view.
It is all Gecko specific. Sure, we may want to explicitly treat chrome scripts somehow differently.
 


> I don't have a great
> understanding of microtasks, but my gut feeling is that there needs to be a
> deeper model for this stuff.
The model is rather clear from the web point of view.  In general "end-of-microtask is right after the outermost script execution or end of a task"[1]. From the point of view scripts running on the web, bug 1081038 changed that quite a bit.

And ContentPolicy happens to be error prone, since scripts using it shouldn't touch DOM synchronously.



[1] there are some special cases, but not really relevant here.

Comment 32

[Gabor Krizsanits (INACTIVE)]

(In reply to Olli Pettay [:smaug] from comment #25)
> This regresses bug 1081038. That is something devtools need to deal with
> explicitly

I don't think that this problem should be solved by devtools folks. And it's
not scratchpad related. This bug means that webcomponents (and in general
anything that relies on microtasks) will work differently in sandboxes
than in regular web-content. We have sandboxes all over the place
for various things, so we should have a clear story for their scripts as well,
or else this will bite us again. (We should totally back out the patches ASAP
regardless, I just don't think we're _done_ here or that we should just hack it
around)

So how are we going to solve the original problem? Is my original idea to just
simply add an nsAutoMicroTask to evalInSandbox instead of trying to do it
in AutoEntryScript is acceptable?

Comment 33

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #32)
> (In reply to Olli Pettay [:smaug] from comment #25)
> > This regresses bug 1081038. That is something devtools need to deal with
> > explicitly
> 
> I don't think that this problem should be solved by devtools folks.
devtools uses JS APIs to run scripts, and those APIs are missing the web specific behavior of running scripts.

> So how are we going to solve the original problem? Is my original idea to
> just
> simply add an nsAutoMicroTask to evalInSandbox instead of trying to do it
> in AutoEntryScript is acceptable?
that is possibly fine, though, devtools don't use evalInSandbox, right?

Comment 34

[Gabor Krizsanits (INACTIVE)]

(In reply to Olli Pettay [:smaug] from comment #33)
> that is possibly fine, though, devtools don't use evalInSandbox, right?

As far as I know scratchpad is a sandbox, and to run code in it evalInSandbox
is called. (a window is hooked up in the sandbox's prototype, that's how the
DOM is reachable)

Comment 35

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

ok, in that case nsAutoMicroTask around somewhere there sounds the right approach, but I'd prefer it to be enabled explicitly.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Boris, do you have any thoughts?

This is really only an issue when script is invoked via some non-webidl-callback mechanism, since webidl callbacks automatically do microtask stuff, right?

In terms of specs, the only such mechanism I'm aware of is execution of <script> elements.  So if you're not doing that or some moral equivalent of that and you're not calling a web idl callback you should not be doing a microtask.

For the case of scratchpad (and console, and possibly debugger evaluation at a breakpoint, though that one is less clear), I would argue that each evaluation is the moral equivalent of a <script> execution and should be explicitly wrapped in the microtask stuff.

Comment 37

[Bobby Holley (:bholley)]

Comment on attachment 8609736 [details] [diff] [review]
backout part 1

Review of attachment 8609736 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking care of this smaug, sorry for the delay.

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 8609737 [details] [diff] [review]
backout part 2

Review of attachment 8609737 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSUtils.cpp
@@ +218,5 @@
>    // EvaluateString() which calls this function with
>    // aCompileOptions.noScriptRval set to true.
>    aRetValue.setUndefined();
>  
> +  nsAutoMicroTask mt;

Per the discussion in the bug, would it make more sense to hoist this into the callers that actually want it (basically just nsScriptLoader, and possible nsGlobalWindow)?

::: dom/xbl/nsXBLProtoImplField.cpp
@@ +392,5 @@
>    if (IsEmpty()) {
>      return NS_OK;
>    }
>  
> +  nsAutoMicroTask mt;

Seems like we don't actually want a microtask here, right?

::: dom/xbl/nsXBLProtoImplMethod.cpp
@@ +292,5 @@
>    if (!global) {
>      return NS_OK;
>    }
>  
> +  nsAutoMicroTask mt;

And here.

Comment 39

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

I just did the backout here. Either that, or the original approach.

Comment 40

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

(In reply to Bobby Holley (:bholley) from comment #38)
> Per the discussion in the bug, would it make more sense to hoist this into
> the callers that actually want it (basically just nsScriptLoader, and
> possible nsGlobalWindow)?
Don't understand why.


> > +  nsAutoMicroTask mt;
> 
> Seems like we don't actually want a microtask here, right?
That is where we had mt before. (and we probably do want it here)

Comment 41

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

bholley, so I don't now know whether you want changes to the patch 2 (which would mean it wasn't a backout anymore) or keep the patch as is?

Comment 42

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #40)
> (In reply to Bobby Holley (:bholley) from comment #38)
> > Per the discussion in the bug, would it make more sense to hoist this into
> > the callers that actually want it (basically just nsScriptLoader, and
> > possible nsGlobalWindow)?
> Don't understand why.

Because this whole bug is about how we only want to do MicroTask for HTML stuff, and several of the callers of nsJSUtils::EvaluateString are non-HTML.

(In reply to Olli Pettay [:smaug] from comment #41)
> bholley, so I don't now know whether you want changes to the patch 2 (which
> would mean it wasn't a backout anymore) or keep the patch as is?

Either way is fine. If this needs to be backported, pure backout + followup is probably the best way to go.

Comment 43

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

yeah, I was thinking pure backout + followup.

Comment 44

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

Comment on attachment 8609736 [details] [diff] [review]
backout part 1

Approval Request Comment
[Feature/regressing bug #]: bug 1081038
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: this is not very well test-coveraged
[Risks and why]: somewhat risky, changes the time when MutationObserver callbacks are called, but should mostly affect chrome code.
[String/UUID change made/needed]: NA

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 8609736 [details] [diff] [review]
backout part 1

a=dveditz for sec-approval and all the branches

Comment 47

[Daniel Veditz [:dveditz]]

Comment on attachment 8609737 [details] [diff] [review]
backout part 2

Please use a check-in comment about backing out the other bug (as you did in part 1) rather than mention this bug.

a=dveditz for sec-approval and all the branches

Comment 48

[Daniel Veditz [:dveditz]]

When these patches land are we going to re-open bug 1081038 and patch it there, or should we expect another patch on this bug for that? Who is going to do that part?

Comment 49

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

I'll file a new bug...Bug 1169307

Comment 50

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

(and sheriffs, I'll land this to m-i)

Comment 51

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

Comment on attachment 8609736 [details] [diff] [review]
backout part 1

FYI, applying this to esr needs some --fuzz

Comment 52

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8717afeef3c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aad1ac832f7

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9aad1ac832f7
https://hg.mozilla.org/mozilla-central/rev/8717afeef3c6

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4b98b442444
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e16ad0edaa6

https://hg.mozilla.org/releases/mozilla-beta/rev/0bf9b7602fec
https://hg.mozilla.org/releases/mozilla-beta/rev/c4a3e88135e9

https://hg.mozilla.org/releases/mozilla-esr38/rev/2fba50655adf
https://hg.mozilla.org/releases/mozilla-esr38/rev/5614502b587d

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/19005113138d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ed2f6aeb1d81

Comment 56

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial crash using both steps from comment 4 and 6 on Firefox 38 beta 3
bp-5e1f85f9-5f8a-4da8-8faa-cccbe2150602
bp-b3928d54-5f1c-4d94-892c-9bb6c2150602

Verified that using 39 beta 2 across platforms (windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) this issue does not reproduce.

Comment 57

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproduced the crash with Firefox 38 beta 3.
Verified as fixed using Firefox 38.1.0 esr under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5.

Comment 58

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Firefox 40 beta 1 and Aurora 41.0a2 2015-07-02 build under Win 7 x64, Ubuntu 14.04 LTS x64 and Mac OS X 10.9.5.