Closed Bug 1319668 Opened 4 years ago Closed 4 years ago

Crash in gfxContext::CurrentState


(Core :: Canvas: 2D, defect, P3)

Windows 10



Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed


(Reporter: ting, Assigned: jfkthame)



(Keywords: crash, Whiteboard: [gfx-noted])

Crash Data


(1 file)

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.
Most of the crashes have this stack, with the crash address being a small
offset from zero (0x50 / 0x60):

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&)
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)

Jonathan, maybe we need to null check 'thebes' here?
Flags: needinfo?(jfkthame)
(In reply to Mats Palmgren (:mats) from comment #1)
> Jonathan, maybe we need to null check 'thebes' here?
> 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".)
Flags: needinfo?(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.
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.
Attachment #8816667 - Flags: review?(mats)
Attachment #8816667 - Flags: review?(mats) → review+
Bug 1319668 - Check for null context before trying to draw canvas text. r=mats
Marking leave-open for now; let's see what crashstats has to say about this.
Keywords: leave-open
Priority: -- → P3
Whiteboard: [gfx-noted]
(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?
Flags: needinfo?(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.

Closed: 4 years ago
Flags: needinfo?(jfkthame)
Resolution: --- → FIXED
Please nominate this for Aurora approval when you get a chance.
Assignee: nobody → jfkthame
Flags: needinfo?(jfkthame)
Keywords: leave-open
Target Milestone: --- → mozilla53
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
Flags: needinfo?(jfkthame)
Attachment #8816667 - Flags: approval-mozilla-aurora?
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
Attachment #8816667 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Setting qe-verify- per Comment 11.
Flags: qe-verify-
Blocks: 1335359
You need to log in before you can comment on or make changes to this bug.