1313884 - Crash in mozilla::dom::CanvasRenderingContext2D::DrawImage

Status: RESOLVED DUPLICATE of bug 1318283

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-2c5b1fbf-d548-417a-acbe-f86d52161029.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, double, double, double, double, double, double, unsigned char, mozilla::ErrorResult&) 	dom/canvas/CanvasRenderingContext2D.cpp:4847
1 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, mozilla::ErrorResult&) 	obj-firefox/dist/include/mozilla/dom/CanvasRenderingContext2D.h:218
2 	xul.dll 	mozilla::dom::CanvasRenderingContext2DBinding::drawImage 	obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:4176
3 	xul.dll 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp:2770
4 		@0x3e560ca7 	
5 		@0xffffff8b

this is a cross-platform crash, which has been around for a while, but is regressing in numbers since firefox 50 beta builds.
overall it's still at a rather low volume though (0.05% of browser crashes on 50.0b last week).

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

In this place, we're assuming AdjustTarget::mTarget is not null, though it can be (and we check elsewhere in the code).  May as well check.
I would like to understand why this spiked up though.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: check-mtarget.patch

This patch mirrors other places in CanvasRenderingContext2D that check IsTargetValid() before using mTarget. This should make sure that EnsureTarget() succeeded and mTarget is valid.

There are quite a few other places that don't do this, and I'm not sure if that is a problem.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8806594 [details] [diff] [review]
check-mtarget.patch

Review of attachment 8806594 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4720,5 @@
>  
>    Element* element = nullptr;
>  
>    EnsureTarget();
> +  if (!IsTargetValid()) {

If we crash today when the target is not valid, let's put a gfxCriticalError() here, so that we get an assert or a note in the crash report.

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: canvas-target.patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ae634cfdf2
Check if mTarget is null in CanvasRenderingContext2D::DrawImage

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c7ae634cfdf2

Comment 7

[[:philipp]]

hi, this looks like a simple null-check - do you think we can uplift this to 50/51 in terms of scope and risk?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

We don't know what caused the crash to spike, so I will check after the weekend if this has done the job, or just moved the crash elsewhere, and request the uplift as appropriate.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

This patch didn't quite stop the crash: https://crash-stats.mozilla.com/report/index/1388d08f-bd97-4528-97d0-429c82161104

Comment 10

[Ryan Hunt [:rhunt]]

I am not sure what is going on here. My best guess is that somehow IsValidTarget() is returning true even when mTarget is null.

Looking at the code for IsValidTarget() this is technically possible if sErrorTarget == null && mBufferProvider != null. But I can't find a case where this could happen.

Milan, would it make sense to put a release assert in AdjustedTarget and other locations to see if that case is happening and when? 

If it isn't that mTarget is null, then something else is and I can't identify another good possibility.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Check for null pointer, diagnostic crash in nightly+aurora. r=gw280

A quick patch with what I had in mind - check for the null pointer, get some more information and crash in the cases where bad things happen, but only in nightly+aurora.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4879b87c17
Part 2: Additional nullptr test and nightly/aurora crash if bad things happen. r=gw280

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/056085427582d32c24ae55d1c5248c31fd6a5fc1 - crashing when bad things happen is one thing, but saying that "bad things have happened" every time we run reftests, https://treeherder.mozilla.org/logviewer.html#?job_id=39033633&repo=mozilla-inbound, or crashtests, https://treeherder.mozilla.org/logviewer.html#?job_id=39033719&repo=mozilla-inbound, or mochitests, https://treeherder.mozilla.org/logviewer.html#?job_id=39035519&repo=mozilla-inbound, or, or, or, or, is another thing entirely. Even I'm not so pessimistic as to say that bad things happen almost every time we run a test.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks for backing this out.  While getting all these failures is great, I will remain ashamed for another 24 hours for not pushing this to try in the first place.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Check for null pointer, diagnostic crash in nightly+aurora. r=jrmuizel

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09175243e5029b4e9f7ea6a840157fe99a40eac7

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #15)
> Created attachment 8809875 [details] [diff] [review]
> Check for null pointer, diagnostic crash in nightly+aurora. r=jrmuizel
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=09175243e5029b4e9f7ea6a840157fe99a40eac7

Based on Jeff's suggestion as to how to work around the lifetime problem in the previous patch.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #15)
> Created attachment 8809875 [details] [diff] [review]
> Check for null pointer, diagnostic crash in nightly+aurora. r=jrmuizel
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=09175243e5029b4e9f7ea6a840157fe99a40eac7

Green.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

this had a regression and when backed out an improvement for windows xp svg opacity:
https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,1,1%5D&series=%5Bautoland,271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,1,1%5D&zoom=1478775369376.7124,1478890356458.904,523.3382906435147,557.0185880412843&selected=%5Bmozilla-inbound,271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,143829,39033623,1%5D

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Joel Maher ( :jmaher) from comment #18)
> this had a regression and when backed out an improvement for windows xp svg
> opacity:
> https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,
> 271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,1,1%5D&series=%5Bautoland,
> 271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,1,1%5D&zoom=1478775369376.7124,
> 1478890356458.904,523.3382906435147,557.0185880412843&selected=%5Bmozilla-
> inbound,271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,143829,39033623,1%5D

Interesting - it was a UAF scenario, so let's see what the "real" patch does.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/741ff57b2407
Part 2. Additional nullptr test and nightly/aurora crash if bad things happen. r=gw280

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Pulsebot from comment #20)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/741ff57b2407
> Part 2. Additional nullptr test and nightly/aurora crash if bad things
> happen. r=gw280

Looks like I lied in the comments as to who reviewed this.

Comment 22

[Marcia Knous [:marcia]]

http://bit.ly/2fwvPQm seems to have also spiked, not sure if it needs it's own bug or is covered under this one.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

I imagine the same underlying cause, and we can make sure we fix it all the same way.  Once the second patch lands we may get more information and understand what the cause is.

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/741ff57b2407

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Crashes confirming mTarget is null:
https://crash-stats.mozilla.com/report/index/f277849c-d189-4cce-81dc-e4c122161122
https://crash-stats.mozilla.com/report/index/570ff6e5-3ad6-4220-9acd-eb3842161122

The action and analysis for this crash is in bug 1318283.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Seems to be related to filters.

Comment 27

[Gerry Chang [:gchang]]

Since bug 1318283 was fixed, mark 51/52/53 fixed.

Comment 28

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.