1319668 - Crash in gfxContext::CurrentState

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-c03b7ffc-9aa3-4302-bb34-9d0e52161122.
=============================================================
#15 top crash of Nightly 20161120030205 on Windows, 8 crashes from 2 installations. There are 2198 crashes with this signature in the past 6 months.

Comment 1

[Mats Palmgren (inactive)]

Most of the crashes have this stack, with the crash address being a small
offset from zero (0x50 / 0x60):

gfxContext::CurrentState()
gfxContext::SetColor(mozilla::gfx::Color const&)
mozilla::dom::CanvasBidiProcessor::DrawText(int, int)
nsBidiPresUtils::ProcessText(char16_t const*, int, unsigned char, nsPresContext*, nsBidiPresUtils::BidiProcessor&, nsBidiPresUtils::Mode, nsBidiPositionResolve*, int, int*, nsBidi*)
mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsAString_internal const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, float*)
mozilla::dom::CanvasRenderingContext2D::FillText(nsAString_internal const&, double, double, mozilla::dom::Optional<double> const&, mozilla::ErrorResult&)
mozilla::dom::CanvasRenderingContext2DBinding::fillText
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
...

Jonathan, maybe we need to null check 'thebes' here?
https://hg.mozilla.org/releases/mozilla-release/annotate/7356baae8e73/dom/canvas/CanvasRenderingContext2D.cpp#l3619

Comment 2

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #1)
> Jonathan, maybe we need to null check 'thebes' here?
> https://hg.mozilla.org/releases/mozilla-release/annotate/7356baae8e73/dom/
> canvas/CanvasRenderingContext2D.cpp#l3619

Yes, looks like it; that should prevent this crash, at least, though it'll presumably leave us with incomplete drawing.

(Seems like if we're getting null here, that suggests something has gone badly in gfx-land; sure enough, looking at the report from comment 0, there are a bunch of GraphicsCriticalError notes about "DrawSurface with bad surface".)

Comment 3

[Jonathan Kew [:jfkthame]]

Actually, at least in bp-c03b7ffc-9aa3-4302-bb34-9d0e52161122, I _don't_ see the gfxCriticalNote << "Invalid target..." note that gfxContext::CreatePreservingTransformOrNull should have issued if it was going to return null. So while I agree we should have a null check here, I'm not sure that'll prove to be the whole story.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Check for null context before trying to draw canvas text

In view of the notes above, I think we should do this, but leave this bug open for a while until we see if the crashes actually stop.

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5144592dd9180a21472159e4d813a7f61c90484e
Bug 1319668 - Check for null context before trying to draw canvas text. r=mats

Comment 6

[Jonathan Kew [:jfkthame]]

Marking leave-open for now; let's see what crashstats has to say about this.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5144592dd918

Comment 8

[Vincent Liu[:vliu]]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Marking leave-open for now; let's see what crashstats has to say about this.

Hi Jonathan,
Will you take this bug for the following tracking?

Comment 9

[Jonathan Kew [:jfkthame]]

AFAICS from looking at crash reports[1] there are no reports for this signature on FF 53.0a1 builds since the patch here landed. So I think we can close this.

[1] https://crash-stats.mozilla.com/signature/?product=Firefox&version=53.0a1&signature=gfxContext%3A%3ACurrentState&date=%3E%3D2016-10-09T17%3A12%3A47.000Z&date=%3C2017-01-09T17%3A12%3A47.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-build_id&_sort=-date&page=1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora approval when you get a chance.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8816667 [details] [diff] [review]
Check for null context before trying to draw canvas text

Approval Request Comment
[Feature/Bug causing the regression]: long-standing crasher

[User impact if declined]: potential crash during canvas drawing

[Is this code covered by automated tests?]: no, AFAIK we don't have STR, this is based on crash reports from the wild

[Has the fix been verified in Nightly?]: indirectly, in that crash reports stopped coming in after the fix was landed

[Needs manual test from QE? If yes, steps to reproduce]: no

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: just adds a null-check/early return to avoid crashing in a gfx failure case

[String changes made/needed]: none

Comment 12

[Julien Cristau [:jcristau] (back on 2026-01-06)]

Comment on attachment 8816667 [details] [diff] [review]
Check for null context before trying to draw canvas text

crash fix for beta52, should be in b2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c44d7b72386

Comment 14

[Andrei Vaida [:avaida]]

Setting qe-verify- per Comment 11.