Crash in gfxContext::CurrentState

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ting, Assigned: jfkthame)

Tracking

({crash})

Trunk
mozilla53
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

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

Attachments

(1 attachment)

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::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
Flags: needinfo?(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".)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5144592dd9180a21472159e4d813a7f61c90484e
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.

[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
Status: NEW → RESOLVED
Closed: 3 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.