Closed Bug 1319668 Opened 4 years ago Closed 4 years ago
Crash in gfx
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
(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".)
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)
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.
(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?
AFAICS from looking at crash reports there are no reports for this signature on FF 53.0a1 builds since the patch here landed. So I think we can close this.  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: 4 years ago
Resolution: --- → FIXED
Please nominate this for Aurora approval when you get a chance.
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
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.
4 years ago
See Also: → 1339762
You need to log in before you can comment on or make changes to this bug.