1164766 - (CVE-2015-4497) use-after-free (& crash) after style flush in CanvasRenderingContext2D (which causes destruction of nsIPresShell)

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[jean-max Reymond]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150511103303

Steps to reproduce:

Open http://www.opendecagnes.com/programme-et-resultats/les-tableaux.html



Actual results:

crash. Crash report:
https://crash-stats.mozilla.com/report/index/63aeedaf-f62e-4cc9-abdb-9c06c2150514


Expected results:

display the document

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: for ESR38
bp-3e29ff07-06a9-407b-9766-655ec2150514
bp-e8a5b520-1820-4af7-a7bd-96fff2150514

Comment 2

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=00a3d59e6e90&tochange=50eddcdf4cbb

Triggered by: Bug 927892

Fixed range pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=01a988be408b&tochange=6e2c2cddd92e

Fixed by: 6e2c2cddd92e	Ryan VanderMeulen — Bug 1148192 - Update pdf.js to version 1.1.24. r=bdahl, r=Mossop

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]: crash on Firefox38.0.5b1

Comment 4

[Markus Stange [:mstange]]

We access the presShell after it's been destroyed.

Stack where the presShell gets destroyed:

> #0  nsAutoRefCnt::operator--() (this=0x2aaad8584970) at ../../dist/include/nsISupportsImpl.h:334
> #1  PresShell::Release() (this=0x2aaad8584800) at /home/mstange/code/mozilla-central/layout/base/nsPresShell.cpp:804
> #2  nsCOMPtr<nsIPresShell>::assign_assuming_AddRef(nsIPresShell*) (this=0x2aaae7df26e0, aNewPtr=0x0) at ../dist/include/nsCOMPtr.h:375
> #3  nsCOMPtr<nsIPresShell>::assign_with_AddRef(nsISupports*) (this=0x2aaae7df26e0, aRawPtr=0x0) at ../../dist/include/nsCOMPtr.h:1030
> #4  nsCOMPtr<nsIPresShell>::operator=(nsIPresShell*) (this=0x2aaae7df26e0, aRhs=0x0) at ../../dist/include/nsCOMPtr.h:546
> #5  nsDocumentViewer::DestroyPresShell() (this=0x2aaae7df2670) at /home/mstange/code/mozilla-central/layout/base/nsDocumentViewer.cpp:4430
> #6  nsDocumentViewer::Hide() (this=0x2aaae7df2670) at /home/mstange/code/mozilla-central/layout/base/nsDocumentViewer.cpp:2126
> #7  nsDocShell::SetVisibility(bool) (this=0x2aaadc835000, aVisibility=false) at /home/mstange/code/mozilla-central/docshell/base/nsDocShell.cpp:6354
> #8  nsFrameLoader::Hide() (this=0x2aaae7b287a0) at /home/mstange/code/mozilla-central/dom/base/nsFrameLoader.cpp:916
> #9  nsHideViewer::Run() (this=0x2aaad15155c0) at /home/mstange/code/mozilla-central/layout/generic/nsSubDocumentFrame.cpp:946
> #10 nsContentUtils::RemoveScriptBlocker() () at /home/mstange/code/mozilla-central/dom/base/nsContentUtils.cpp:5050
> #11 nsAutoScriptBlocker::~nsAutoScriptBlocker() (this=0x7fffffff1c88, __in_chrg=<optimized out>) at /home/mstange/code/mozilla-central/dom/base/nsContentUtils.h:2419
> #12 PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x2aaae12e9800, aFlush=...) at /home/mstange/code/mozilla-central/layout/base/nsPresShell.cpp:4279
> #13 PresShell::FlushPendingNotifications(mozFlushType) (this=0x2aaae12e9800, aType=Flush_Layout) at /home/mstange/code/mozilla-central/layout/base/nsPresShell.cpp:4158
> #14 nsDocument::FlushPendingNotifications(mozFlushType) (this=0x2aaada3a9000, aType=Flush_Layout) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:8263
> #15 nsDocument::FlushPendingNotifications(mozFlushType) (this=0x2aaad75d5800, aType=Flush_Layout) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:8241
> #16 mozilla::dom::Element::GetPrimaryFrame(mozFlushType) (this=0x2aaaeb099560, aType=Flush_Layout) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:2034
> #17 mozilla::dom::Element::GetScrollFrame(nsIFrame**, bool) (this=0x2aaaeb099560, aStyledFrame=0x7fffffff1f50, aFlushLayout=true) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:581
> #18 mozilla::dom::Element::GetClientAreaRect() (this=0x2aaaeb099560) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:888
> #19 mozilla::dom::Element::ClientWidth() (this=0x2aaaeb099560) at ../../dist/include/mozilla/dom/Element.h:776
> #20 mozilla::dom::ElementBinding::get_clientWidth(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitGetterCallArgs) (cx=0x2aaae49c6340, obj=(JSObject * const) 0x2aaaeb480250 [object HTMLDivElement], self=0x2aaaeb099560, args=$jsval((JSObject *) 0x2aaaeaf67600 [object Function "clientWidth"])) at /home/mstange/code/obj-m-debug/dom/bindings/ElementBinding.cpp:2278
> #21 mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) (cx=0x2aaae49c6340, argc=0, vp=0x7fffffff2a88) at /home/mstange/code/mozilla-central/dom/bindings/BindingUtils.cpp:2422
> #22 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (cx=0x2aaae49c6340, native=0x2aaab033cf7d <mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/mstange/code/mozilla-central/js/src/jscntxtinlines.h:235
> #23 js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (cx=0x2aaae49c6340, args=..., construct=js::NO_CONSTRUCT) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:502
> #24 js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, thisv=..., fval=..., argc=0, argv=0x0, rval=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:558
> #25 js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, obj=(JSObject *) 0x2aaaeb480250 [object HTMLDivElement], fval=$jsval((JSObject *) 0x2aaaeaf67600 [object Function "clientWidth"]), argc=0, argv=0x0, rval=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:628
> #26 CallGetter(JSContext*, JS::HandleObject, js::HandleShape, JS::MutableHandleValue) (cx=0x2aaae49c6340, receiver=(JSObject * const) 0x2aaaeb480250 [object HTMLDivElement], shape=0x2aaaeaf64740, vp=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/NativeObject.cpp:1634
> #27 GetExistingProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (cx=0x2aaae49c6340, receiver=(JSObject * const) 0x2aaaeb480250 [object HTMLDivElement], obj=(js::NativeObject * const) 0x2aaaeaf54760 [object ElementPrototype] delegate, shape=0x2aaaeaf64740, vp=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/NativeObject.cpp:1684
> #28 NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (cx=0x2aaae49c6340, obj=(js::NativeObject * const) 0x2aaaeb480250 [object HTMLDivElement], receiver=(JSObject * const) 0x2aaaeb480250 [object HTMLDivElement], id=$jsid("clientWidth"), nameLookup=NotNameLookup, vp=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/NativeObject.cpp:1898
> #29 js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, obj=(js::NativeObject * const) 0x2aaaeb480250 [object HTMLDivElement], receiver=(JSObject * const) 0x2aaaeb480250 [object HTMLDivElement], id=$jsid("clientWidth"), vp=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/NativeObject.cpp:1932
> #30 js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, obj=(JSObject * const) 0x2aaaeb480250 [object HTMLDivElement], receiver=(JSObject * const) 0x2aaaeb480250 [object HTMLDivElement], id=$jsid("clientWidth"), vp=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/NativeObject.h:1439
> #31 GetPropertyOperation(JSContext*, js::InterpreterFrame*, JS::HandleScript, jsbytecode*, JS::MutableHandleValue, JS::MutableHandleValue) (cx=0x2aaae49c6340, fp=0x2aaac9ec34a8, script=0x2aaadc71e380, pc=0x2aaad0fe24c3 "5", lval=JSVAL_VOID, vp=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:260
> #32 Interpret(JSContext*, js::RunState&) (cx=0x2aaae49c6340, state=...) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:2431
> #33 js::RunScript(JSContext*, js::RunState&) (cx=0x2aaae49c6340, state=...) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:452
> #34 js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (cx=0x2aaae49c6340, args=..., construct=js::NO_CONSTRUCT) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:521
> #35 js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, thisv=..., fval=..., argc=1, argv=0x7fffffff4620, rval=$jsval("auto")) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:558
> #36 js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, obj=(JSObject *) 0x2aaaeb493d80 [object Object], fval=$jsval((JSObject *) 0x2aaaeb4ce8c0 [object Function "PDFViewer.prototype.currentScaleValue"]), argc=1, argv=0x7fffffff4620, rval=$jsval("auto")) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:628
> #37 SetExistingProperty(JSContext*, js::HandleNativeObject, JS::HandleObject, JS::HandleId, js::HandleNativeObject, js::HandleShape, JS::MutableHandleValue, JS::ObjectOpResult&) (cx=0x2aaae49c6340, obj=(js::NativeObject * const) 0x2aaaeb493d80 [object Object], receiver=(JSObject * const) 0x2aaaeb493d80 [object Object], id=$jsid("currentScaleValue"), pobj=(js::NativeObject * const) 0x2aaaeb464c40 [object Object] delegate, shape=0x2aaaeb40ee78, vp=$jsval("auto"), result=...) at /home/mstange/code/mozilla-central/js/src/vm/NativeObject.cpp:2242
> #38 js::NativeSetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, js::QualifiedBool, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) (cx=0x2aaae49c6340, obj=(js::NativeObject * const) 0x2aaaeb493d80 [object Object], receiver=(JSObject * const) 0x2aaaeb493d80 [object Object], id=$jsid("currentScaleValue"), qualified=js::Qualified, vp=$jsval("auto"), result=...) at /home/mstange/code/mozilla-central/js/src/vm/NativeObject.cpp:2275
> #39 SetObjectProperty(JSContext*, JSOp, JS::HandleValue, JS::HandleId, JS::MutableHandleValue) (cx=0x2aaae49c6340, op=JSOP_STRICTSETPROP, lval=$jsval((JSObject *) 0x2aaaeb493d80 [object Object]), id=$jsid("currentScaleValue"), rref=$jsval("auto")) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:325
> #40 SetPropertyOperation(JSContext*, JSOp, JS::HandleValue, JS::HandleId, JS::HandleValue) (cx=0x2aaae49c6340, op=JSOP_STRICTSETPROP, lval=$jsval((JSObject *) 0x2aaaeb493d80 [object Object]), id=$jsid("currentScaleValue"), rval=$jsval("auto")) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:361
> #41 Interpret(JSContext*, js::RunState&) (cx=0x2aaae49c6340, state=...) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:2500
> #42 js::RunScript(JSContext*, js::RunState&) (cx=0x2aaae49c6340, state=...) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:452
> #43 js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (cx=0x2aaae49c6340, args=..., construct=js::NO_CONSTRUCT) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:521
> #44 js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, thisv=..., fval=..., argc=1, argv=0x7fffffff5ba0, rval=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/vm/Interpreter.cpp:558
> #45 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (cx=0x2aaae49c6340, thisv=$jsval((JSObject *) 0x2aaae91909c0 [object Proxy]), fval=$jsval((JSObject *) 0x2aaaeb4a30c0 [object Function "webViewerResize"]), args=..., rval=JSVAL_VOID) at /home/mstange/code/mozilla-central/js/src/jsapi.cpp:4337
> #46 mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) (this=0x2aaaeb0792b0, cx=0x2aaae49c6340, aThisVal=$jsval((JSObject *) 0x2aaae91909c0 [object Proxy]), event=..., aRv=...) at /home/mstange/code/obj-m-debug/dom/bindings/EventListenerBinding.cpp:48
> #47 mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) (this=0x2aaaeb0792b0, thisObjPtr=@0x7fffffff5e10: 0x2aaad780cc00, event=..., aRv=..., aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aCompartment=0x0) at ../../dist/include/mozilla/dom/EventListenerBinding.h:54
> #48 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) (this=0x2aaae3b3bbc0, aListener=0x2aaae7ddf970, aDOMEvent=0x2aaacecc4620, aCurrentTarget=0x2aaad780cc00) at /home/mstange/code/mozilla-central/dom/events/EventListenerManager.cpp:950
> #49 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (this=0x2aaae3b3bbc0, aPresContext=0x2aaae103a000, aEvent=0x7fffffff6350, aDOMEvent=0x7fffffff6280, aCurrentTarget=0x2aaad780cc00, aEventStatus=0x7fffffff6288) at /home/mstange/code/mozilla-central/dom/events/EventListenerManager.cpp:1101
> #50 mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (this=0x2aaae3b3bbc0, aPresContext=0x2aaae103a000, aEvent=0x7fffffff6350, aDOMEvent=0x7fffffff6280, aCurrentTarget=0x2aaad780cc00, aEventStatus=0x7fffffff6288) at ../../dist/include/mozilla/EventListenerManager.h:330
> #51 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) (this=0x2aaad0c04008, aVisitor=..., aCd=...) at /home/mstange/code/mozilla-central/dom/events/EventDispatcher.cpp:209
> #52 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (aChain=..., aVisitor=..., aCallback=0x0, aCd=...) at /home/mstange/code/mozilla-central/dom/events/EventDispatcher.cpp:299
> #53 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (aTarget=0x2aaae0026420, aPresContext=0x2aaae103a000, aEvent=0x7fffffff6350, aDOMEvent=0x0, aEventStatus=0x7fffffff633c, aCallback=0x0, aTargets=0x0) at /home/mstange/code/mozilla-central/dom/events/EventDispatcher.cpp:634
> #54 PresShell::FireResizeEvent() (this=0x2aaad8584800) at /home/mstange/code/mozilla-central/layout/base/nsPresShell.cpp:2142
> #55 PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x2aaad8584800, aFlush=...) at /home/mstange/code/mozilla-central/layout/base/nsPresShell.cpp:4225
> #56 PresShell::FlushPendingNotifications(mozFlushType) (this=0x2aaad8584800, aType=Flush_Style) at /home/mstange/code/mozilla-central/layout/base/nsPresShell.cpp:4158
> #57 nsComputedDOMStyle::GetStyleContextForElement(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) (aElement=0x2aaace824bb0, aPseudo=0x0, aPresShell=0x2aaad8584800, aStyleType=nsComputedDOMStyle::eAll) at /home/mstange/code/mozilla-central/layout/style/nsComputedDOMStyle.cpp:409
> #58 mozilla::dom::GetFontParentStyleContext(mozilla::dom::Element*, nsIPresShell*, mozilla::ErrorResult&) (aElement=0x2aaace824bb0, presShell=0x2aaad8584800, error=...) at /home/mstange/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:2099
> #59 mozilla::dom::GetFontStyleContext(mozilla::dom::Element*, nsAString_internal const&, nsIPresShell*, nsAString_internal&, mozilla::ErrorResult&) (aElement=0x2aaace824bb0, aFont=..., presShell=0x2aaad8584800, aOutUsedFont=..., error=...) at /home/mstange/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:2161
> #60 mozilla::dom::CanvasRenderingContext2D::SetFont(nsAString_internal const&, mozilla::ErrorResult&) (this=0x2aaad8985000, font=..., error=...) at /home/mstange/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:2993
> #61 mozilla::dom::CanvasRenderingContext2DBinding::set_font(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitSetterCallArgs) (cx=0x2aaae49c6340, obj=(JSObject * const) 0x2aaadc76b430 [object CanvasRenderingContext2D], self=0x2aaad8985000, args=$jsval("normal bold 16px \"Tahoma\", sans-serif")) at /home/mstange/code/obj-m-debug/dom/bindings/CanvasRenderingContext2DBinding.cpp:5753

Then later in GetFontStyleContext we access the styleSet of the destroyed presShell and crash in nsStyleSet::ResolveStyleForRules:

> #0 nsStyleSet::ResolveStyleForRules(nsStyleContext*, nsTArray<nsCOMPtr<nsIStyleRule> > const&) (this=0x5a5a5a5a5a5a5a5a, aParentContext=0x0, aRules=...)
> #1 mozilla::dom::GetFontStyleContext(mozilla::dom::Element*, nsAString_internal const&, nsIPresShell*, nsAString_internal&, mozilla::ErrorResult&) (aElement=0x2aaace824bb0, aFont=..., presShell=0x2aaad8584800, aOutUsedFont=..., error=...) at /home/mstange/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:2175
> #2 mozilla::dom::CanvasRenderingContext2D::SetFont(nsAString_internal const&, mozilla::ErrorResult&) (this=0x2aaad8985000, font=..., error=...)
> #3 mozilla::dom::CanvasRenderingContext2DBinding::set_font(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitSetterCallArgs) (cx=0x2aaae49c6340, obj=(JSObject * const) 0x2aaadc76b430 [object CanvasRenderingContext2D], self=0x2aaad8985000, args=$jsval("normal bold 16px \"Tahoma\", sans-serif")) at /home/mstange/code/obj-m-debug/dom/bindings/CanvasRenderingContext2DBinding.cpp:5753

Comment 5

[Markus Stange [:mstange]]

I don't really understand why we didn't have this bug before my changes in https://hg.mozilla.org/integration/mozilla-inbound/rev/e91710f9fa78 . The relevant code looks very similar.

Comment 6

[Markus Stange [:mstange]]

Attachment: testcase (WILL CRASH FIREFOX)

The flush that nsComputedDOMStyle::GetStyleContextForElement does is a style flush, so by itself it won't cause the presshell to be destroyed. But style flushes can fire resize events, and those can synchronously destroy presshells, which is what's happening here.

Comment 7

[Mats Palmgren (inactive)]

Bug 1149555 will probably fix this crash.  It might be a good idea to add script
blockers to the entry points that might potentially reach code that does a flush.
There might be other ways besides resize events that can cause script to run.
Adding a nsAutoScriptBlocker in SetFont etc fixes the crash here, right?

Comment 8

[Markus Stange [:mstange]]

A script blocker will prevent the flush from actually flashing anything. So while it will fix the crash, it'll mean that SetFont doesn't resolve relative font units correctly.
I'm just going to keep the presshell alive by using nsCOMPtrs instead of raw pointers for it. Ehsan suggested making that change in an innocent sounding shadow bug that's not security sensitive.

Comment 9

[Markus Stange [:mstange]]

Attachment: similar testcase using fillStyle instead of font (WILL CRASH FIREFOX)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Why are 39+ marked as unaffected?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

This is too late for 38.*

Comment 12

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

This crashes 39+. Marking them as affected; also tracking. We could still possibly get this into 39 but 40 seems more likely.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

The second testcase crashes esr31 as well, so marking everything as affected.

Comment 14

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

Markus, any luck here? I just realized I didn't needinfo you.

Comment 15

[Mats Palmgren (inactive)]

FYI, dholbert is working on this in bug 1175278.

Comment 16

[Markus Stange [:mstange]]

Attachment: wip patch

Comment 18

[Markus Stange [:mstange]]

Attachment: 6-Bug_1164766___Use_nsCOMPtrs_in_more_places__r_mats.diff

Comment 19

[Markus Stange [:mstange]]

Attachment: 7-Bug_1164766___Add_crashtests__r_mats.diff

This patch shouldn't land until we've released the fix.

Comment 20

[Markus Stange [:mstange]]

Attachment: 8-Bug_1164766___Add_reftests_that_ensure_reentrant_ctx_font_ordering_and_flushing__r_mats.diff

These tests justify why we need to do the flush. Landing them can also wait until the fix has been released.

Comment 21

[Daniel Holbert [:dholbert]]

Can you make sure we have tests that cover all the paths I highlighted in the dupe?  It looks like your reftests don't cover the ParseFilter() or ParseColor() issues right now.

We don't necessarily need a reftest; just a crashtest version of my test on the other bug (with a "test-pref" annotation in the crashtest manifest to enable the canvas filters pref) should be fine.

Comment 22

[Markus Stange [:mstange]]

Those are in attachment 8623875 [details] [diff] [review].

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8623875 [details] [diff] [review]
7-Bug_1164766___Add_crashtests__r_mats.diff

Oh, this also adds some comments to the code. I can separate those changes out into a separate patch when I land this.

Comment 24

[Daniel Holbert [:dholbert]]

Ah, right; I just saw the comments in the second patch, and assumed all the tests would be in the final patch. (might be worth rearranging them so all the tests land together; maybe doesn't matter much though)

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 8623874 [details] [diff] [review]
6-Bug_1164766___Use_nsCOMPtrs_in_more_places__r_mats.diff

nsComputedDOMStyle::GetStyleContextForElement returns null if it fails.
GetFontParentStyleContext propagates that value but doesn't set 'error':
http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?rev=536bd9910bc2#2096
GetFontStyleContext calls GetFontParentStyleContext but only checks
error.Failed():
http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?rev=536bd9910bc2#2161
If the PresShell::Destroy has run, the StyleSet has shutdown and
it's probably not a good idea to call styleSet->ResolveStyleForRules
at this point.

GetFontStyleContext is called by SetFont which returns early
without setting 'error' if it gets null:
http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3002
SetFont is called by GetCurrentFontStyle():
http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3828
which only checks 'err.Failed()' which can be false at this
point event though all of the above failed.

GetCurrentFontStyle is called by DrawOrMeasureText:
http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3629
It seems to me the use of 'canvasStyle' is unsafe below
that point.  Besides, GetCurrentFontStyle can clearly return
null so we should bail out if that happens rather than assert.

So I think we should:
1. make GetFontParentStyleContext null-check the result of
   nsComputedDOMStyle::GetStyleContextForElement and do
   "error.Throw(NS_ERROR_FAILURE)" if null.
2. move the "nsCOMPtr<nsIPresShell> presShell" in GetCurrentFontStyle
   to before its SetFont call
3. make DrawOrMeasureText null-check 'currentFontStyle'
   and return early with a failure code if null

I would also like to know more about CanvasFilterChainObserver.
It has a raw CanvasRenderingContext2D* and calls UpdateFilter() on it,
which seems unsafe unless we carefully tear these observers down
before it goes away.  Where do we do that?

In InitializeWithSurface we call SetDimensions (which may flush)
and after that we use the passed in gfxASurface* param - is that
safe?

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 8623875 [details] [diff] [review]
7-Bug_1164766___Add_crashtests__r_mats.diff

The following functions/methods can lead to a flush and are unsafe:
GetFontParentStyleContext
GetFontStyleContext
CanvasRenderingContext2D::GetCurrentFontStyle
CanvasRenderingContext2D::ParseColor
CanvasRenderingContext2D::ClearTarget
CanvasRenderingContext2D::ParseFilter
CanvasRenderingContext2D::UpdateFilter
CanvasRenderingContext2D::SetFont
CanvasRenderingContext2D::DrawOrMeasureText
CanvasRenderingContext2D::DrawWindow
CanvasRenderingContext2D::SetStyleFromString
CanvasRenderingContext2D::SetStyleFromUnion
CanvasRenderingContext2D::SetDimensions
CanvasRenderingContext2D::SetIsOpaque
CanvasRenderingContext2D::SetIsIPC
CanvasRenderingContext2D::InitializeWithSurface
CanvasRenderingContext2D::SetContextOptions
CanvasRenderingContext2D::SetShadowColor
CanvasRenderingContext2D::SetFilter
CanvasRenderingContext2D::FillText
CanvasRenderingContext2D::StrokeText
CanvasRenderingContext2D::MeasureText
CanvasRenderingContext2D::AsyncDrawXULElement
CanvasRenderingContext2D::SetMozTextStyle
CanvasRenderingContext2D::SetStrokeStyle
CanvasRenderingContext2D::SetFillStyle
CanvasRenderingContext2D::GetFont
CanvasRenderingContext2D::GetMozTextStyle

All of these needs to have a scary comment in CanvasRenderingContext2D.h
along the lines of:
/**
 * @note This method may call PresShell::FlushPendingNotifications and
 * thus destroy the world as we know it.  Callers MUST hold a strong
 * ref on the CanvasRenderingContext2D instance the call is made on.
 * (calls from JS are safe though since it does that for you)
 */

We might as well make that a blanket requirement for calls to any
CanvasRenderingContext2D method to avoid having the comments
becoming out-of-date with the code.

We also need to audit all internal (non-JS) calls to any of the
above methods, recursively...
I have only checked CanvasRenderingContext2D.cpp for problems.

Comment 27

[Mats Palmgren (inactive)]

A few of the methods above are protected though, and CanvasRenderingContext2D
is marked final, so only the following public methods can be called from outside:
CanvasRenderingContext2D::SetStrokeStyle
CanvasRenderingContext2D::SetFillStyle
CanvasRenderingContext2D::SetFilter
CanvasRenderingContext2D::FillText
CanvasRenderingContext2D::StrokeText
CanvasRenderingContext2D::MeasureText
CanvasRenderingContext2D::GetFont
CanvasRenderingContext2D::GetMozTextStyle
CanvasRenderingContext2D::SetMozTextStyle
CanvasRenderingContext2D::DrawWindow
CanvasRenderingContext2D::AsyncDrawXULElement
CanvasRenderingContext2D::SetDimensions
CanvasRenderingContext2D::InitializeWithSurface
CanvasRenderingContext2D::SetIsOpaque
CanvasRenderingContext2D::SetContextOptions
CanvasRenderingContext2D::SetShadowColor
CanvasRenderingContext2D::SetIsIPC
CanvasRenderingContext2D::SetFont

Comment 28

[Mats Palmgren (inactive)]

I can't find any C++ callers of:
CanvasRenderingContext2D::SetStrokeStyle
CanvasRenderingContext2D::SetFillStyle
CanvasRenderingContext2D::SetFilter
CanvasRenderingContext2D::FillText
CanvasRenderingContext2D::StrokeText
CanvasRenderingContext2D::GetFont
CanvasRenderingContext2D::GetMozTextStyle
CanvasRenderingContext2D::SetMozTextStyle
CanvasRenderingContext2D::DrawWindow
CanvasRenderingContext2D::AsyncDrawXULElement
CanvasRenderingContext2D::SetShadowColor
CanvasRenderingContext2D::SetFont

and I don't see anything matching this one at first glance:
CanvasRenderingContext2D::MeasureText
  http://mxr.mozilla.org/mozilla-central/search?string=MeasureText%28&find=\.c&findi=\.c&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
  the signature we're looking for is:
    TextMetrics* MeasureText(const nsAString& rawText, mozilla::ErrorResult& error)

CanvasRenderingContext2D::SetIsIPC
  has only one C++ caller - HTMLCanvasElement::MozGetIPCContext:
  http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#862
  and that's only called from here:
  http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.h#118
  and that's only called from JS, afaict.

Comment 29

[Mats Palmgren (inactive)]

CanvasRenderingContext2D::InitializeWithSurface
  Two C++ calls:
  http://mxr.mozilla.org/mozilla-central/source/widget/windows/TaskbarPreview.cpp#70
    looks safe since it AddRefs 'gCtx'
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp#674
    I'm guesssing it gets 'ctx' from here:
    http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#993
    and 'mCurrentContext' is a strong ref.  We should probably make 'ctx'
    a nsCOMPtr here anyway.  And we should probably add a nsWeakFrame check around
    that call too.

Comment 30

[Mats Palmgren (inactive)]

The remaining:
CanvasRenderingContext2D::SetIsOpaque
CanvasRenderingContext2D::SetContextOptions
CanvasRenderingContext2D::SetDimensions

are called from C++ only once - all inside HTMLCanvasElement::UpdateContext:
  http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#885

The only C++ calls to UpdateContext are internally in HTMLCanvasElement:
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#187
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#203
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#821
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#865

If those are safe depends on if the callers of SetAttr, UnsetAttr, GetContext
and MozGetIPCContext.  SetAttr and UnsetAttr themselves looks OK, but it's
impossible to check all consumers of those.  Hopefully they all hold a strong
ref on the element.  MozGetIPCContext traces back to JS so it should be safe.
GetContext is called internally in HTMLCanvasElement in a few places,
CopyInnerTo looks a bit fishy with its use of raw pointers:
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#273
It might be a good idea to sprinkle some strong refs there, I dunno.

For UpdateContext itself:
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#878
It might be a good idea hold a strong ref on the stack for the context
and make the calls on that instead of using mCurrentContext in three
places that may flush.  That is:
 nsCOMPtr<nsICanvasRenderingContextInternal> ctx(mCurrentContext);
 ctx->SetIsOpaque(...)
 ctx->SetContextOptions(...)
 ctx->SetDimensions(...)

Comment 31

[Markus Stange [:mstange]]

(In reply to Mats Palmgren (:mats) from comment #28)
> and I don't see anything matching this one at first glance:
> CanvasRenderingContext2D::MeasureText

Yeah, this is also only called from JavaScript, through https://hg.mozilla.org/mozilla-central/annotate/c9a79ec16280/dom/webidl/CanvasRenderingContext2D.webidl#l114

Thanks for investigating this so thoroughly!

Comment 32

[Markus Stange [:mstange]]

(In reply to Mats Palmgren (:mats) from comment #25)
> If the PresShell::Destroy has run, the StyleSet has shutdown and
> it's probably not a good idea to call styleSet->ResolveStyleForRules
> at this point.

Should we check presShell->IsDestroying() before accessing the StyleSet?

> GetFontStyleContext is called by SetFont which returns early
> without setting 'error' if it gets null:
> http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3002

I think it does this in order to follow the canvas spec that says that invalid values shouldn't cause the ctx.font setter to throw an exception.

> SetFont is called by GetCurrentFontStyle():
> http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3828
> which only checks 'err.Failed()' which can be false at this
> point event though all of the above failed.

Yes, but in that case, SetFont won't have modified CurrentState().fontGroup, so GetCurrentFontStyle() will return the font that was set before SetFont was called.

> GetCurrentFontStyle is called by DrawOrMeasureText:
> http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3629
> It seems to me the use of 'canvasStyle' is unsafe below
> that point.

Why? Because the styleSet might have shut down?

> Besides, GetCurrentFontStyle can clearly return
> null

But only in the case where we hit the NS_ERROR in GetCurrentFontStyle(), right?

> so we should bail out if that happens rather than assert.

> So I think we should:
> 1. make GetFontParentStyleContext null-check the result of
>    nsComputedDOMStyle::GetStyleContextForElement and do
>    "error.Throw(NS_ERROR_FAILURE)" if null.

Ok.

> 2. move the "nsCOMPtr<nsIPresShell> presShell" in GetCurrentFontStyle
>    to before its SetFont call

What would this achieve?

> 3. make DrawOrMeasureText null-check 'currentFontStyle'
>    and return early with a failure code if null

Ok.

> I would also like to know more about CanvasFilterChainObserver.
> It has a raw CanvasRenderingContext2D* and calls UpdateFilter() on it,
> which seems unsafe unless we carefully tear these observers down
> before it goes away.  Where do we do that?

This does look sketchy. I'm going to take a closer look at this later.

> In InitializeWithSurface we call SetDimensions (which may flush)
> and after that we use the passed in gfxASurface* param - is that
> safe?

Yes, gfxASurfaces aren't affected by flushing. Pointers to them should always be coming from a refpointer that's still somewhere on the stack.

(In reply to Mats Palmgren (:mats) from comment #30)
> The only C++ calls to UpdateContext are internally in HTMLCanvasElement:
> http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.
> cpp#187
> http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.
> cpp#203
> http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.
> cpp#821
> http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.
> cpp#865
> 
> If those are safe depends on if the callers of SetAttr, UnsetAttr, GetContext
> and MozGetIPCContext.

Can flushing destroy the HTMLCanvasElement?

Comment 33

[Daniel Holbert [:dholbert]]

(In reply to Markus Stange [:mstange] from comment #32)
> > If those are safe depends on if the callers of SetAttr, UnsetAttr, GetContext
> > and MozGetIPCContext.
> 
> Can flushing destroy the HTMLCanvasElement?

(I think the answer to this question is "no", based on bug 1175278 comment 17; looks like the CanvasRenderingContext2D holds a [CC-aware] reference to its HTMLCanvasElement & keeps it alive until they've both been orphaned after the JS has finished running.)

Comment 34

[Mats Palmgren (inactive)]

(In reply to Markus Stange [:mstange] from comment #32)
> (In reply to Mats Palmgren (:mats) from comment #25)
> > If the PresShell::Destroy has run, the StyleSet has shutdown and
> > it's probably not a good idea to call styleSet->ResolveStyleForRules
> > at this point.
> 
> Should we check presShell->IsDestroying() before accessing the StyleSet?

If we make GetFontParentStyleContext set 'error' when it fails
(returns null) then it shouldn't be necessary since we will
return early in that case.  I guess we could add a
MOZ_ASSERT(!presShell->IsDestroying()) to make that assumption
explicit.

> > GetFontStyleContext is called by SetFont which returns early
> > without setting 'error' if it gets null:
> > http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> > CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3002
> 
> I think it does this in order to follow the canvas spec that says that
> invalid values shouldn't cause the ctx.font setter to throw an exception.

Does invalid values really make GetFontStyleContext return null?
I don't think it does.  It seems like it should return a valid object,
it's just that the provided (invalid) font value have been ignored
and isn't reflected in the returned nsStyleContext.

(FWIW, I tried this JS ''ctx.font = "{"'' with a breakpoint in SetFont
 and I got a non-null result from GetFontStyleContext.)

> > SetFont is called by GetCurrentFontStyle():
> > http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> > CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3828
> > which only checks 'err.Failed()' which can be false at this
> > point event though all of the above failed.
> 
> Yes, but in that case, SetFont won't have modified CurrentState().fontGroup,
> so GetCurrentFontStyle() will return the font that was set before SetFont
> was called.

Yes, but it seems the intention of the code is to fallback to
"eFamily_sans_serif" in case of failure.  Granted, it doesn't
matter much.  Given the "if (!CurrentState().fontGroup)" check
at the start though, we /might/ return null in this case.

> > GetCurrentFontStyle is called by DrawOrMeasureText:
> > http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> > CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3629
> > It seems to me the use of 'canvasStyle' is unsafe below
> > that point.
> 
> Why? Because the styleSet might have shut down?

Yeah, I wouldn't trust any layout or style data to be alive
after PresShell::Destroy.

> > Besides, GetCurrentFontStyle can clearly return
> > null
> 
> But only in the case where we hit the NS_ERROR in GetCurrentFontStyle(),
> right?

No, if it's null on entry and SetFont returns early because
it gets null from GetFontStyleContext then err.Failed() in
GetCurrentFontStyle() will still be false in some cases
so it doesn't try to setup the "eFamily_sans_serif" fallback
and just returns the current value, which is null.

If we make GetFontParentStyleContext set the ErrorResult to Failed
then it can only happen in the NS_ERROR case.

> > 2. move the "nsCOMPtr<nsIPresShell> presShell" in GetCurrentFontStyle
> >    to before its SetFont call
> 
> What would this achieve?

It would prevent the shell from being deleted after the SetFont
call.  I was worried the CreateFontGroup stuff that follows
might depend on it, but perhaps it doesn't. Feel free to ignore
this bit.  As long as the subsequent GetPresShell() call returns
null in a safe way I think we're OK.

We might want to change the if-condition on line 3840 though, to:
  if (presShell && !presShell->IsDestroying()) {

> > If those are safe depends on if the callers of SetAttr, UnsetAttr, GetContext
> > and MozGetIPCContext.
> 
> Can flushing destroy the HTMLCanvasElement?

I suspect it can, unless the caller holds a strong ref for the
duration of the call.  HTMLCanvasElement has a strong ref on the
CanvasRenderingContext2D which in turn has a strong ref back on
the element, but I suspect our cycle collector can detect such
cycles and destroy both objects if there are no other strong refs.
And if 'mCurrentContext' is assigned null (or a new object) then
the last ref to the element might go away, which is why I suggested
we hold a strong ref on the stack in HTMLCanvasElement::UpdateContext
as a precaution.

Comment 35

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #33)
> (I think the answer to this question is "no", based on bug 1175278 comment
> 17; looks like the CanvasRenderingContext2D holds a [CC-aware] reference to
> its HTMLCanvasElement & keeps it alive until they've both been orphaned
> after the JS has finished running.)

Yes, if the call comes from JS we're safe because it holds an external
strong ref that keeps it alive.  What we're discussing at this point
is internal C++ calls.

Comment 36

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

Too late for 39, we have to wontfix this again.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

We're not going to chemspill esr31 over this, so marking it as wontfix too.

Comment 38

[Daniel Holbert [:dholbert]]

(Correcting bug summary, since this isn't actually limited to the PDF viewer)

Comment 40

[Daniel Holbert [:dholbert]]

Markus, do you have cycles to shift this to your front-burner and finish this off?  It's been a few weeks since the last update here, and we don't want to drop the ball on this -- as dveditz notes over in dupe-bug 1175278, ZDI reported this independently (in that bug) and there's a disclosure deadline associated with their report.

(Mats is on vacation for the immediate future, so he can't review a new patch *right now* I expect, but it'd be great if we had a patch waiting for him when he gets back, & can fix this on all branches ASAP at that point. [Alternately, I could also step in to review before he's back, but I'd be slightly afraid that I'd miss one of the concerns that he raised or had in mind].)

Comment 41

[Markus Stange [:mstange]]

Thanks for the reminder, Daniel. I'll try to finish this today.

Comment 42

[Markus Stange [:mstange]]

(In reply to Mats Palmgren (on vacation) (:mats) from comment #34)
> > > GetFontStyleContext is called by SetFont which returns early
> > > without setting 'error' if it gets null:
> > > http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> > > CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3002
> > 
> > I think it does this in order to follow the canvas spec that says that
> > invalid values shouldn't cause the ctx.font setter to throw an exception.
> 
> Does invalid values really make GetFontStyleContext return null?
> I don't think it does.  It seems like it should return a valid object,
> it's just that the provided (invalid) font value have been ignored
> and isn't reflected in the returned nsStyleContext.
> 
> (FWIW, I tried this JS ''ctx.font = "{"'' with a breakpoint in SetFont
>  and I got a non-null result from GetFontStyleContext.)

I can't confirm that. We return null from GetFontStyleContext if !fontParsedSuccessfully: http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?rev=536bd9910bc2#2143
And I hit that case when I execute ctx.font = "{".

Comment 43

[Markus Stange [:mstange]]

(In reply to Mats Palmgren (on vacation) (:mats) from comment #25)
> GetFontStyleContext is called by SetFont which returns early
> without setting 'error' if it gets null:
> http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3002
> SetFont is called by GetCurrentFontStyle():
> http://mxr.mozilla.org/mozilla-central/source/dom/canvas/
> CanvasRenderingContext2D.cpp?rev=536bd9910bc2#3828
> which only checks 'err.Failed()' which can be false at this
> point event though all of the above failed.

OK, so basically we want a way for SetFont to tell the caller that it didn't update the font in a certain case, without triggering an exception for JS callers. I'm going to add a method called SetFontInternal which returns a bool (whether it updated the font) in addition to the error code. SetFont will just call SetFontInteral and ignore the returned value. But GetCurrentFontStyle will call SetFontInternal and check the return value, and then also execute the fallback code if SetFontInternal returned false.

Comment 44

[Markus Stange [:mstange]]

Attachment: part 1: patch

Comment 45

[Markus Stange [:mstange]]

Attachment: part 2: tests

Comment 46

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

This bug is about to miss the Firefox 40 and ESR 38.2 releases. Our last Beta is being built tomorrow morning (Thursday). Should I mark this as "won't fix" for those releases?

Comment 47

[Markus Stange [:mstange]]

Yes please. Mats is without doubt the best reviewer for this fix, and he's on PTO for a few more days, as far as I know.

Comment 48

[Mats Palmgren (inactive)]

Comment on attachment 8633755 [details] [diff] [review]
part 1: patch

Looks good.  We should probably land it without the @note comments to avoid
revealing to much about which parts were unsafe.  We can add those after
the bug is public.

(In reply to Markus Stange [:mstange] from comment #32)
> (In reply to Mats Palmgren (:mats) from comment #25)
> > I would also like to know more about CanvasFilterChainObserver.
> > It has a raw CanvasRenderingContext2D* and calls UpdateFilter() on it,
> > which seems unsafe unless we carefully tear these observers down
> > before it goes away.  Where do we do that?
> 
> This does look sketchy. I'm going to take a closer look at this later.

Please file a (hidden) follow-up bug on this.

Comment 49

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

Should we land this for 41 and 42 now?

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Probably not since it'll need to land on ESR38+3 as well.

Comment 51

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

This needs sec-approval to land.

Comment 52

[Markus Stange [:mstange]]

Attachment: part 1a: patch without the comments

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. Comments and tests will land later.

Which older supported branches are affected by this flaw?
38+

If not all supported branches, which bug introduced the flaw?
Unclear; it fixes many problems, some of which have been introduced before Firefox 4.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply easily (or with minor merging) on older branches.

How likely is this patch to cause regressions; how much testing does it need?
Not very likely. It's mostly just adding error checks and nsRefPtrs. The only potential problems I can see is that valid content triggers one of those error checks, but I think that's very unlikely since things would have been broken in those cases even before this patch.

Comment 53

[Markus Stange [:mstange]]

Attachment: part 1b: Add comments (do not land yet)

Comment 54

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

Given where we are in the product cycle, if I give sec-approval+, I'd normally instruct folks to not check this in until August 25 (two weeks into the next cycle) to reduce the exposure window. Is there a reason we'd need to check this in sooner?

Comment 55

[Markus Stange [:mstange]]

The only urgency on this bug is imposed by the October disclosure deadline from 1175278. I don't really have a good picture of how that impacts the decision about when we need to check it in.

Comment 56

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

That's fine. It will still be in 41 (since we ship 40 on Tuesday). This is just when to check in.

I'll give sec-approval+ for checkin on 8/25 or after (around then please though).

We'll want this on Aurora, Beta, and ESR38 then so it goes out in 41 and in the matching ESR38 release. If this patch applies cleanly, please nominate it for those branches. Otherwise, please make appropriate patches and nominate them.

Comment 57

[Daniel Veditz [:dveditz]]

(In reply to Mats Palmgren (:mats) from comment #48)
> Please file a (hidden) follow-up bug on this.

Was that bug filed?

Comment 58

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

Tracking this for 43 on the assumption it's likely affected.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

cf comment #56

Comment 60

[Markus Stange [:mstange]]

(In reply to Daniel Veditz [:dveditz] from comment #57)
> (In reply to Mats Palmgren (:mats) from comment #48)
> > Please file a (hidden) follow-up bug on this.
> 
> Was that bug filed?

I filed bug 1195968. It's not urgent to fix because canvas filters are disabled by default.

Comment 61

[Daniel Veditz [:dveditz]]

Comment on attachment 8643268 [details] [diff] [review]
part 1a: patch without the comments

Markus is on vacation and we'd like this one checked-in for testing before uplift. Thanks!

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/942cacbdaef2

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/942cacbdaef2

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta/esr38 uplift approval on this when you get a chance.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

And release, sorry.

Comment 68

[Daniel Holbert [:dholbert]]

Comment on attachment 8643268 [details] [diff] [review]
part 1a: patch without the comments

I think Markus is on vacation for a few more days, so I'll trigger the approval requests on his behalf.

Approval Request Comment
[Feature/regressing bug #]: bug 927892, based on dupe bug 1175278 comment 10

[User impact if declined]: exposure to potentially-exploitable bug

[Describe test coverage new/current, TreeHerder]: We have automated tests for canvas APIs. (We don't have regression tests checked in for this bug yet, because we don't want to reveal how to trigger the bug, since it's potentially exploitable)

[Risks and why]: See last section of comment 52 -- not very likely to cause regressions. Mostly adding owning pointers and error checks. In any real-world case where behavior changes, the new behavior should be safer than the old behavior, so that's a good thing. (Note that there is one new MOZ_RELEASE_ASSERT in the patch, but we're expecting our error-handling code to prevent that from being triggered.)

[String/UUID change made/needed]: None.

Comment 69

[Daniel Holbert [:dholbert]]

Comment on attachment 8643268 [details] [diff] [review]
part 1a: patch without the comments

(Requesting esr38 separately since it uses a different approval-request-comment format for some reason)
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
 This is a sec-crit bug.

User impact if declined:
 Exposure to potentially exploitable bug.
 
Fix Landed on Version:
 Landed on 43 so far.  (and I've requested backport approval requested for 42, 41, and 40)

Risk to taking this patch (and alternatives if risky): See risk assessment in previous comment. No real alternatives.

String or UUID changes made by this patch: None.

Comment 70

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8643268 [details] [diff] [review]
part 1a: patch without the comments

Sure, let's take it everywhere

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a516c5d06a21
https://hg.mozilla.org/releases/mozilla-beta/rev/df08fae1a435
https://hg.mozilla.org/releases/mozilla-release/rev/12564185b77d
https://hg.mozilla.org/releases/mozilla-esr38/rev/5b7897acfc87 (ESR 38.2.x)
https://hg.mozilla.org/releases/mozilla-esr38/rev/5e8b87a87c8b (ESR 38.3.x)

Sorry for accidentally resetting the esr38 tracking flag.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1effc4cb6414
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/1effc4cb6414

Needs rebasing for b2g34_v2_1s.

Comment 73

[Nick Thomas [:nthomas] (UTC+12)]

Transplanted onto the Firefox relbranch for esr38 (GECKO3820esr_2015080613_RELBRANCH):
http://hg.mozilla.org/releases/mozilla-esr38/rev/a4b88bfecdfe

Comment 74

[Ryan VanderMeulen [:RyanVM]]

Was https://hg.mozilla.org/releases/mozilla-esr38/rev/5b7897acfc87 insufficient for some reason?

Comment 75

[Ryan VanderMeulen [:RyanVM]]

*sigh* dammit, TB

Comment 76

[Sylvestre Ledru [:Sylvestre]]

Just for confirmation, fennec is not affected, right?

Comment 77

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

n-i Al and Margaret for comment 76.

Comment 78

[Daniel Holbert [:dholbert]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #76)
> Just for confirmation, fennec is not affected, right?

I don't think that's correct -- this is a Gecko rendering bug, so I'd expect it to affect all Gecko-based products, including Fennec.

Comment 79

[Kevin Brosnan [Ex-Mozilla]]

I'll take a look into reproducible on Firefox for Android. Though I agree with dholbert this should affect us.

Comment 80

[Daniel Holbert [:dholbert]]

Even if hypothetically the testcases here don't obviously cause problems on Android, I'd still consider Android affected & just in need of a more sneaky testcase. There's no platform / UI dependency here at all.

If there's action we need to take if this affects Fennec, then we should proceed with that action.

Comment 81

[Markus Stange [:mstange]]

Attachment: part 1a, rebased for b2g34_v2_1s

Comment 82

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks everyone for the confirmation. Sylvestre, does that mean we need to do a 40.0.3 equivalent for Fennec too?

Comment 83

[Kevin Brosnan [Ex-Mozilla]]

I can reproduce this on v40 for Android and that it is fixed on Nightly v43 for provided test cases with the following note. 

I was not able to reproduce the crash on https://bugzilla.mozilla.org/attachment.cgi?id=8623278 which is the ZDI provided POC and https://bugzilla.mozilla.org/attachment.cgi?id=8650061 which is from the dupe 1196400.

Comment 84

[Kevin Brosnan [Ex-Mozilla]]

Yes we will need Firefox for Android builds.

Comment 85

[Sylvestre Ledru [:Sylvestre]]

OK, build started then!

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/b4135ecf90a3

Comment 87

[Catalin Suciu [:csuciu]]

I was able to reproduce the crash on Firefox for Android 40 and can confirm that it is fixed on 40.0.3

Comment 88

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I managed to crash Firefox 40.0.2 using all testcases from this bug and bug 1175278, except for the URL in comment 0 and testcase 5 from bug 1175278. That makes 6 testcases with which Firefox 40.0.2 crashed on Mac, Ubuntu and Windows.

I could no longer reproduce the crashes on Windows 7 x64, Mac OS X 10.9.5, and Ubuntu 12.04 x86, using any of the test cases from this bug or bug 1175278.

Comment 89

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Verification above was performed using:
- Firefox 40.0.3 build 1 - BuildID: 20150826023504
- ESR 38.2.1 build 2 - BuildID: 20150826044534

Comment 90

[Catalin Suciu [:csuciu]]

I'm not able to reproduce this crash on Firefox for Android 41 Beta 4 using any of the test cases from bug #1175278 or from this bug.

Comment 91

[Daniel Holbert [:dholbert]]

In addition to comment 88, I just double-checked that the attached testcases all load without any crashes or assertions in a mozilla-inbound debug build from today.  I tested duplicate-bug 1175278's testcases as well.

--> Marking as VERIFIED.

Does this bug still need to be hidden for any reason? Looks like it's fixed on all supported branches all the way back to ESR38, as of 6 months ago. (August 2015)